Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[x86] drreg: aflags not preserved on fault if xax dead #4933

Closed
abhinav92003 opened this issue Jun 1, 2021 · 0 comments · Fixed by #4944
Closed

[x86] drreg: aflags not preserved on fault if xax dead #4933

abhinav92003 opened this issue Jun 1, 2021 · 0 comments · Fixed by #4944
Labels
Component-Extension OpSys-x86

Comments

@abhinav92003
Copy link
Contributor

On x86, the aflags restore logic seems broken if xax is dead and doesn't need to be spilled.

The current condition for detecting aflags_in_xax needs xax to be spilled prior to the lahf.

dynamorio/ext/drreg/drreg.c

Line 1912 in e8fc651

else if (prev_xax_spill && instr_get_opcode(&inst) == OP_lahf && spill)

PR #4932 demonstrates the issue. drreg-test fails:

196: drreg-test running
196: ERROR: spilled flags value was not preserved in test #17!
196: drreg-test finished

Before:

TAG  0x00007fe0ffe9bd67
+0    L3 @0x00007fdebffeefc8  48 c7 c2 17 f1 00 00 mov    $0x000000000000f117 -> %rdx
+7    L3 @0x00007fdebffe1348  48 c7 c2 17 f1 00 00 mov    $0x000000000000f117 -> %rdx
+14   L3 @0x00007fdebff71ec0  b4 d7                mov    $0xd7 -> %ah
+16   L3 @0x00007fdebffdc928  9e                   sahf   %ah
+17   L3 @0x00007fdebff77a90  90                   nop
+18   L3 @0x00007fdebff71120  48 c7 c1 00 00 00 00 mov    $0x0000000000000000 -> %rcx
+25   L3 @0x00007fdebffdd1b0  48 8b 09             mov    (%rcx)[8byte] -> %rcx
+28   L3 @0x00007fdebff74b10  48 c7 c0 34 12 00 00 mov    $0x0000000000001234 -> %rax // rax dead
+35   L3 @0x00007fdebffde9c0  eb 00                jmp    $0x00007fe0ffe9bd8c
END 0x00007fe0ffe9bd67

After:

TAG  0x00007fe0ffe9bd67
+0    L3 @0x00007fdebffeefc8  48 c7 c2 17 f1 00 00 mov    $0x000000000000f117 -> %rdx
+7    L3 @0x00007fdebffe1348  48 c7 c2 17 f1 00 00 mov    $0x000000000000f117 -> %rdx
+14   m4 @0x00007fdebff77178                       <label>
+14   L3 @0x00007fdebff71ec0  b4 d7                mov    $0xd7 -> %ah
+16   L3 @0x00007fdebffdc928  9e                   sahf   %ah
+17   m4 @0x00007fdebffdd7b8  9f                   lahf    -> %ah // aflags spill
+18   m4 @0x00007fdebffea2d0  0f 90 c0             seto    -> %al
+21   m4 @0x00007fdebffdcfb0  48 3b c1             cmp    %rax %rcx // overwrite aflags
+24   L3 @0x00007fdebff77a90  90                   nop
+25   L3 @0x00007fdebff71120  48 c7 c1 00 00 00 00 mov    $0x0000000000000000 -> %rcx
+32   L3 @0x00007fdebffdd1b0  48 8b 09             mov    (%rcx)[8byte] -> %rcx // fault
+35   m4 @0x00007fdebffde940  65 48 a3 e0 00 00 00 mov    %rax -> %gs:0x000000e0[8byte]
                              00 00 00 00
+46   m4 @0x00007fdebffdcec8                       <label>
+46   L3 @0x00007fdebff74b10  48 c7 c0 34 12 00 00 mov    $0x0000000000001234 -> %rax // rax dead so no need to spill xax to slot
+53   m4 @0x00007fdebff763f0  65 48 a3 e8 00 00 00 mov    %rax -> %gs:0x000000e8[8byte]
                              00 00 00 00
+64   m4 @0x00007fdebff76af8                       <label>
+64   m4 @0x00007fdebff77a10  65 48 a1 e0 00 00 00 mov    %gs:0x000000e0[8byte] -> %rax
                              00 00 00 00
+75   m4 @0x00007fdebff53e40  3c 81                cmp    %al $0x81
+77   m4 @0x00007fdebff50a50  9e                   sahf   %ah
+78   m4 @0x00007fdebfff1480  65 48 a1 e8 00 00 00 mov    %gs:0x000000e8[8byte] -> %rax
                              00 00 00 00
+89   m4 @0x00007fdebffefea8                       <label>
+89   L3 @0x00007fdebffde9c0  eb 00                jmp    $0x00007fe0ffe9bd8c
END 0x00007fe0ffe9bd67

As expected, if I remove the mov $0x0000000000001234 -> %rax app instr that's causing rax to be dead, drreg-test passes. This is because then xax is spilled to a slot prior to the lahf.

TAG  0x00007f96f3e6fd67
+0    L3 @0x00007f94b3fb29c0  48 c7 c2 17 f1 00 00 mov    $0x000000000000f117 -> %rdx
+7    L3 @0x00007f94b3f45120  48 c7 c2 17 f1 00 00 mov    $0x000000000000f117 -> %rdx
+14   m4 @0x00007f94b3fb17b8                       <label>
+14   L3 @0x00007f94b3fb11b0  b4 d7                mov    $0xd7 -> %ah
+16   L3 @0x00007f94b3f48b10  9e                   sahf   %ah
+17   m4 @0x00007f94b3fb0ec8  65 48 a3 e8 00 00 00 mov    %rax -> %gs:0x000000e8[8byte] // xax spilled to slot as it's not dead
                              00 00 00 00
+28   m4 @0x00007f94b3f4a3f0                       <label>
+28   m4 @0x00007f94b3f4aaf8  9f                   lahf    -> %ah
+29   m4 @0x00007f94b3f24a50  0f 90 c0             seto    -> %al
+32   m4 @0x00007f94b3f4ba10  48 3b c1             cmp    %rax %rcx
+35   L3 @0x00007f94b3f4b178  90                   nop
+36   L3 @0x00007f94b3fbe2d0  48 c7 c1 00 00 00 00 mov    $0x0000000000000000 -> %rcx
+43   L3 @0x00007f94b3fb0fb0  48 8b 09             mov    (%rcx)[8byte] -> %rcx
+46   m4 @0x00007f94b3f27e40  3c 81                cmp    %al $0x81
+48   m4 @0x00007f94b3fc2fc8  9e                   sahf   %ah
+49   m4 @0x00007f94b3fc5480  65 48 a1 e8 00 00 00 mov    %gs:0x000000e8[8byte] -> %rax
                              00 00 00 00
+60   m4 @0x00007f94b3fc3ea8                       <label>
+60   L3 @0x00007f94b3fb2940  eb 00                jmp    $0x00007f96f3e6fd85
END 0x00007f96f3e6fd67
@abhinav92003 abhinav92003 mentioned this issue Jun 1, 2021
Closed
abhinav92003 added a commit that referenced this issue Jun 9, 2021
@abhinav92003
i#4939,4933: Make drreg state restoration more robust.
0779ea4 
Revamps the drreg state restoration logic in drreg_event_restore_state to correctly
handle various corner cases like multiple restores (i#4939), aflags in reg (i#4933)
and multi-phase use (i#3823).

Makes the reconstructed ilist available to drreg_event_restore_state. This is needed
for determining whether some instr is app or tool.

The new logic looks at the complete ilist to track the movement of the app value of
gpr/aflags across spills and restores.
We do not forget a spill slot on seeing one restore (which caused i#4939), but
remember that the app value can still be found in the spill slot, in addition to
the gpr itself. Instead, a spill slot is forgetten when it is used to spill some
other reg.
We also track whether gprs/aflags contain their native app value or not. This helps
in handling cases of overlapping or nested spill regions arising out of multi-phase
use of drreg. Tool writes (except restores) to gprs/aflags clobbers the native value.
App writes to gprs/aflags installs a native value and clobbers the value in the
reg's spill slot.
We also track movement of aflags to registers more robustly (to fix i#4933).

Adds tests for the i#4939 and i#4933 cases. Test for i#3823 will be added in a
separate PR.

Issue: #4933, #4939, #3823
Fixes: #4933
Fixes: #4939
@abhinav92003 abhinav92003 mentioned this issue Jun 9, 2021
Merged
abhinav92003 added a commit that referenced this issue Jun 14, 2021
@abhinav92003
i#3801,#3823: Make drreg state restoration more robust (#4944)
f62441b 
Revamps the drreg state restoration logic in drreg_event_restore_state to
correctly handle various corner cases like multiple restores (i#4939), multiple
spills, aflags in reg (i#4933), multi-phase use (i#3823) leading to nested
or otherwise overlapping spill regions, and app instrs that look like drreg
spill/restore instrs.

Makes the reconstructed ilist available to drreg_event_restore_state (i#3801).
The additional metadata that is used by the new logic is only whether an instr
is an app or meta instr. For cases when the reconstructed ilist is not
available, we continue to use the old best-effort logic. Highlighted its caveats
in comments.

The new logic looks at the complete ilist to track the movement of the app value
of gprs/aflags across spills and restores. The native value can be in the
gpr/aflags itself or one or more spill slots. We do not forget a spill slot on
seeing one restore (which caused i#4939), but remember that the app value can
still be found in the spill slot, in addition to the gpr itself. Instead, a
spill slot is forgotten when it is used to spill some other reg or if there's an
app instr that writes to the spilled gpr/aflags.

We also track whether gprs/aflags contain their native app value or not. This
helps in handling cases of overlapping or nested spill regions arising out of
multi-phase use of drreg. Tool writes (except restores) to gprs/aflags clobber
the native value. App writes to gprs/aflags install a native value and
invalidate the value in the gpr's/aflags' spill slot. Restores from spill slots
also install a native value.

Adds tests for multi-phase gpr reservation cases (nested/overlapping spill
regions), multiple spills for gpr and multiple restores for gpr. Tests for these
scenarios for aflags will be added when multi-phase drreg support is added for
aflags.

Tracks movement of aflags to registers more robustly to fix i#4933. Also adds a
test for this case.

Avoids confusing app instrs that write using the stolen reg on AArch64 with
drreg spill or restore instrs. Also adds a test that verifies this fix.

Adds documentation about faulting fragment's reconstructed ilist being available
in restore state event callbacks.

Issue: #4933, #4939, #3823, #3801
Fixes: #4933
Fixes: #4939
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Component-Extension OpSys-x86
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

i#3801,#3823: Make drreg state restoration more robust DynamoRIO/dynamorio
1 participant
@abhinav92003