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.

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

PC discontinuity reported as incorrect signal handler return point by invariant checker #5912

Closed
abhinav92003 opened this issue Mar 14, 2023 · 2 comments · Fixed by #5965
Closed
Assignees

Comments

@abhinav92003
Copy link
Contributor

abhinav92003 commented Mar 14, 2023

Today we do not check PC discontinuities between an instr followed by a "kernel xfer" marker. If there is a discontinuity, it is reported as a "Signal handler return point incorrect" invariant error later when the signal returns. E.g. the following

    28156078    20615075:       79758 ifetch       5 byte(s) @ 0x000000001a593862 66 45 89 0c fc       data16 mov    %r9w -> (%r12,%rdi,8)[2byte]
    28156079    20615075:       79758 write        2 byte(s) @ 0x0000177219ac0060 by PC 0x000000001a593862

// PC discontinuity in the kernel xfer marker address and prev instr. Not reported by invariant checker as such.
    28156080    20615075:       79758 <marker: kernel xfer from 0x1a59386d to handler>
...
    28156767    20615515:       79758 <marker: syscall xfer from 0x7ff30d21c1c9>

// Invariant error reported here due to mismatch with the pre_kernel_xfer instr (even though it is same as the kernel xfer marker address).
    28156770    20615516:       79758 ifetch       3 byte(s) @ 0x000000001a59386d 48 31 c0             xor    %rax %rax -> %rax

Such errors should be reported as what they are to avoid confusion.

@derekbruening
Copy link
Contributor

Offline discussion concluded we want to do this:

A) Add our regular PC transition check to the pre-signal instruction fetch transition to the signal marker value for the interrupted PC

B) Change the signal check to compare the handler return point to the marker value, not to the pre-signal instruction fetch

derekbruening added a commit that referenced this issue Jul 21, 2023
The drmemtrace record type TRACE_TYPE_INSTR_CONDITIONAL_JUMP is now
deprecated in offline traces where it is replaced by
TRACE_TYPE_INSTR_TAKEN_JUMP and TRACE_TYPE_INSTR_UNTAKEN_JUMP.  The
version number is bumped to accomplish this.

Indirect branches in drmemtrace traces now contain a marker holding
the actual target (TRACE_MARKER_TYPE_BRANCH_TARGET), which immediately
precedes the branch record.

These changes are implemented inside raw2trace and in PT ir2trace.
raw2trace tests are updated and new tests added.

Special cases with new logic and tests include:
+ Rseq side exits must specially set whether taken or untaken
+ Rseq abort rollbacks to a branch are tested as this is the case
  where it was not easy to identify whether a branch was taken
  in the past (it required particular inferences).
+ Branches prior to signals.
+ Trace-final branches: we just delete these.

Adds view support:

```
     2212815     1648444:     1249326 ifetch       6 byte(s) @ 0x00007f3406720707 48 3d 01 f0 ff ff    cmp    %rax, $0xfffff001
     2212816     1648445:     1249326 ifetch       2 byte(s) @ 0x00007f340672070d 73 01                jnb    $0x00007f3406720710 (untaken)
     2212817     1648445:     1249326 <marker: indirect branch target 0x7f34066a8b37>
     2212818     1648446:     1249326 ifetch       1 byte(s) @ 0x00007f340672070f c3                   ret
     2212819     1648446:     1249326 read         8 byte(s) @ 0x00007ffd91e24fa8 by PC 0x00007f340672070f
     2212820     1648447:     1249326 ifetch       5 byte(s) @ 0x00007f34066a8b37 4c 8b 54 24 48       mov    0x48(%rsp), %r10
```

Adds several new invariant checks and augments the existing PC
continuity checks.  This required a little refactoring to check
branches before signals which is part of #5912.  Unit tests for each
cases are added.

Updates the documentation and the changelist.

Issue: #5490, #6213, #5912
Fixes #6213
Fixes #5490
derekbruening added a commit that referenced this issue Jul 25, 2023
+ Remove documented guarantee that branches are delayed
  (but the impl remains; added a comment about waiting
  for users to update before changing that, if we do)
+ Added comments about some issues in signal PC continuity
  checks that #5912 should address
+ Added missing clear of last_indirect_target_
+ Use last_insr_in_cur_context_ instead of prev_instr_
+ Multiple comment wording and variable name and test parameter updates
+ Added an untaken-before-signal raw2trace unit test
+ Added an assert on no non-branch instrs being delayed
derekbruening added a commit that referenced this issue Jul 25, 2023
…ev_instr_decoded_; #5912 covers changing both sides to the prior in the cur context
derekbruening added a commit that referenced this issue Jul 25, 2023
The drmemtrace record type TRACE_TYPE_INSTR_CONDITIONAL_JUMP is now
deprecated in offline traces where it is replaced by
TRACE_TYPE_INSTR_TAKEN_JUMP and TRACE_TYPE_INSTR_UNTAKEN_JUMP. The
version number is bumped to accomplish this.

Indirect branches in drmemtrace traces now contain a marker holding the
actual target (TRACE_MARKER_TYPE_BRANCH_TARGET), which immediately
precedes the branch record.

These changes are implemented inside raw2trace and in PT ir2trace.
raw2trace tests are updated and new tests added.

Special cases with new logic and tests include:
+ Rseq side exits must specially set whether taken or untaken
+ Rseq abort rollbacks to a branch are tested as this is the case where
it was not easy to identify whether a branch was taken in the past (it
required particular inferences).
+ Branches prior to signals.
+ Trace-final and window-final branches: we just delete these.

Adds view support:

```
     2212815     1648444:     1249326 ifetch       6 byte(s) @ 0x00007f3406720707 48 3d 01 f0 ff ff    cmp    %rax, $0xfffff001
     2212816     1648445:     1249326 ifetch       2 byte(s) @ 0x00007f340672070d 73 01                jnb    $0x00007f3406720710 (untaken)
     2212817     1648445:     1249326 <marker: indirect branch target 0x7f34066a8b37>
     2212818     1648446:     1249326 ifetch       1 byte(s) @ 0x00007f340672070f c3                   ret
     2212819     1648446:     1249326 read         8 byte(s) @ 0x00007ffd91e24fa8 by PC 0x00007f340672070f
     2212820     1648447:     1249326 ifetch       5 byte(s) @ 0x00007f34066a8b37 4c 8b 54 24 48       mov    0x48(%rsp), %r10
```

Adds several new invariant checks and augments the existing PC
continuity checks. This required a little refactoring to check branches
before signals which is part of #5912. Unit tests for each case are
added.  Adds a couple of comments on issues that #5912 should
address.

Updates the documentation to remove the documented guarantee
that branches are delayed.

Updates the changelist.

Issue: #5490, #6213, #5912
Fixes #6213
Fixes #5490
ivankyluk pushed a commit to ivankyluk/dynamorio that referenced this issue Jul 28, 2023
The drmemtrace record type TRACE_TYPE_INSTR_CONDITIONAL_JUMP is now
deprecated in offline traces where it is replaced by
TRACE_TYPE_INSTR_TAKEN_JUMP and TRACE_TYPE_INSTR_UNTAKEN_JUMP. The
version number is bumped to accomplish this.

Indirect branches in drmemtrace traces now contain a marker holding the
actual target (TRACE_MARKER_TYPE_BRANCH_TARGET), which immediately
precedes the branch record.

These changes are implemented inside raw2trace and in PT ir2trace.
raw2trace tests are updated and new tests added.

Special cases with new logic and tests include:
+ Rseq side exits must specially set whether taken or untaken
+ Rseq abort rollbacks to a branch are tested as this is the case where
it was not easy to identify whether a branch was taken in the past (it
required particular inferences).
+ Branches prior to signals.
+ Trace-final and window-final branches: we just delete these.

Adds view support:

```
     2212815     1648444:     1249326 ifetch       6 byte(s) @ 0x00007f3406720707 48 3d 01 f0 ff ff    cmp    %rax, $0xfffff001
     2212816     1648445:     1249326 ifetch       2 byte(s) @ 0x00007f340672070d 73 01                jnb    $0x00007f3406720710 (untaken)
     2212817     1648445:     1249326 <marker: indirect branch target 0x7f34066a8b37>
     2212818     1648446:     1249326 ifetch       1 byte(s) @ 0x00007f340672070f c3                   ret
     2212819     1648446:     1249326 read         8 byte(s) @ 0x00007ffd91e24fa8 by PC 0x00007f340672070f
     2212820     1648447:     1249326 ifetch       5 byte(s) @ 0x00007f34066a8b37 4c 8b 54 24 48       mov    0x48(%rsp), %r10
```

Adds several new invariant checks and augments the existing PC
continuity checks. This required a little refactoring to check branches
before signals which is part of DynamoRIO#5912. Unit tests for each case are
added.  Adds a couple of comments on issues that DynamoRIO#5912 should
address.

Updates the documentation to remove the documented guarantee
that branches are delayed.

Updates the changelist.

Issue: DynamoRIO#5490, DynamoRIO#6213, DynamoRIO#5912
Fixes DynamoRIO#6213
Fixes DynamoRIO#5490
ivankyluk pushed a commit to ivankyluk/dynamorio that referenced this issue Jul 29, 2023
The drmemtrace record type TRACE_TYPE_INSTR_CONDITIONAL_JUMP is now
deprecated in offline traces where it is replaced by
TRACE_TYPE_INSTR_TAKEN_JUMP and TRACE_TYPE_INSTR_UNTAKEN_JUMP. The
version number is bumped to accomplish this.

Indirect branches in drmemtrace traces now contain a marker holding the
actual target (TRACE_MARKER_TYPE_BRANCH_TARGET), which immediately
precedes the branch record.

These changes are implemented inside raw2trace and in PT ir2trace.
raw2trace tests are updated and new tests added.

Special cases with new logic and tests include:
+ Rseq side exits must specially set whether taken or untaken
+ Rseq abort rollbacks to a branch are tested as this is the case where
it was not easy to identify whether a branch was taken in the past (it
required particular inferences).
+ Branches prior to signals.
+ Trace-final and window-final branches: we just delete these.

Adds view support:

```
     2212815     1648444:     1249326 ifetch       6 byte(s) @ 0x00007f3406720707 48 3d 01 f0 ff ff    cmp    %rax, $0xfffff001
     2212816     1648445:     1249326 ifetch       2 byte(s) @ 0x00007f340672070d 73 01                jnb    $0x00007f3406720710 (untaken)
     2212817     1648445:     1249326 <marker: indirect branch target 0x7f34066a8b37>
     2212818     1648446:     1249326 ifetch       1 byte(s) @ 0x00007f340672070f c3                   ret
     2212819     1648446:     1249326 read         8 byte(s) @ 0x00007ffd91e24fa8 by PC 0x00007f340672070f
     2212820     1648447:     1249326 ifetch       5 byte(s) @ 0x00007f34066a8b37 4c 8b 54 24 48       mov    0x48(%rsp), %r10
```

Adds several new invariant checks and augments the existing PC
continuity checks. This required a little refactoring to check branches
before signals which is part of DynamoRIO#5912. Unit tests for each case are
added.  Adds a couple of comments on issues that DynamoRIO#5912 should
address.

Updates the documentation to remove the documented guarantee
that branches are delayed.

Updates the changelist.

Issue: DynamoRIO#5490, DynamoRIO#6213, DynamoRIO#5912
Fixes DynamoRIO#6213
Fixes DynamoRIO#5490
@lihasgupta
Copy link
Contributor

#5965 (comment)

lihasgupta added a commit that referenced this issue Sep 24, 2023
Removes redundant `pre_signal_flow_continuity` check in the
invariant_checker.

Adds unit tests to check for PC discontinuities in the transitions
between instructions and kernel xfer markers

Adds caching around the decoding, to prevent repeated calls to the
expensive `decode_from_copy()` function.

Refactors the invariant_checker to use `instr_info_t` structs which
group together memref's and their corresponding decodings.

Fixes: #5912
Fixes: #6006
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants