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

Translated stolen register app value lost on signal/synchall control transfer #4495

Closed
derekbruening opened this issue Oct 27, 2020 · 2 comments · Fixed by #4526
Closed

Translated stolen register app value lost on signal/synchall control transfer #4495

derekbruening opened this issue Oct 27, 2020 · 2 comments · Fixed by #4526
Assignees

Comments

@derekbruening
Copy link
Contributor

When translating a thread context for synchall or on a signal, we invoke the client fault handler and arrive at the app's proper value for the stolen register in our local mcontext. We then use the same mcontext to get back into DR through an exit stub, but for that transition we need to put DR's TLS pointer back into the stolen register. That clobbers the translated value. If all client handlers did not touch the value, no harm is done since the TLS slot value is taken as the app value; but if a client were to need to restore a value there, we would get the wrong value in the end. This should be rare, since drreg avoids using the stolen register.

@derekbruening
Copy link
Contributor Author

Taking this on as it is looking like it may be related to #4460 in that both seem to involve the TLS stolen reg slot across signals and translations.

@derekbruening
Copy link
Contributor Author

For the signal path, it seems to only affect the handler itself: a longjmp out of there should restore properly to the setjmp value, and a re-execute will work since the sigcontext has the right value. It seems less likely that the handler will assume the value from the faulting context, but it is possible.

For the synchall path, this is a more serious issue and less of a corner case.

derekbruening added a commit that referenced this issue Nov 11, 2020
Currently, a stolen register value that is translated by a client is
discarded both in a synchall and a synchronous signal.  We fix both
paths here.

Adds a new test client.stolen-reg which tests both a synchronous fault
signal translation path as well as a synchall translation path via a
synchall flush.  The test was confirmed to fail without either of the
fixes in place.

Ensures the new test passes on 32-bit ARM.  This exercised the
translate_from_synchall_to_dispatch() path and found some ARM bugs:
the reset exit stub is A32 mode, so we need to change the mode for the
suspended sigcontext; yet thread_set_mcontext() did not support that.
That is all fixed now and verified.

Fixes #4495
derekbruening added a commit that referenced this issue Nov 11, 2020
Currently, a stolen register value that is translated by a client is
discarded both in a synchall and a synchronous signal.  We fix both
paths here.

Adds a new test client.stolen-reg which tests both a synchronous fault
signal translation path as well as a synchall translation path via a
synchall flush.  The test was confirmed to fail without either of the
fixes in place.

Ensures the new test passes on 32-bit ARM.  This exercised the
translate_from_synchall_to_dispatch() path and found some ARM bugs:
the reset exit stub is A32 mode, so we need to change the mode for the
suspended sigcontext; yet thread_set_mcontext() did not support that.
That is all fixed now and verified.

Fixes #4495
gregcawthorne pushed a commit that referenced this issue Nov 28, 2020
Currently, a stolen register value that is translated by a client is
discarded both in a synchall and a synchronous signal.  We fix both
paths here.

Adds a new test client.stolen-reg which tests both a synchronous fault
signal translation path as well as a synchall translation path via a
synchall flush.  The test was confirmed to fail without either of the
fixes in place.

Ensures the new test passes on 32-bit ARM.  This exercised the
translate_from_synchall_to_dispatch() path and found some ARM bugs:
the reset exit stub is A32 mode, so we need to change the mode for the
suspended sigcontext; yet thread_set_mcontext() did not support that.
That is all fixed now and verified.

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

Successfully merging a pull request may close this issue.

1 participant