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

[NativeAOT] When reconciling shadow stack after catch, use more precise way to figure how much to pop. #104652

Merged
merged 4 commits into from
Jul 17, 2024

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Jul 10, 2024

Fixes: #104628

After turning AVs in managed code into managed exceptions, we will not see the faulting/throwing IP on the shadow stack as redirecting vectored exception context is not a call thus will not push the faulting location to the shadow stack.
Also, there are other issues with throwing IP potentially present on the shadow stack multiple times, where the lowermost occurrence is not necessarily the correct one to unwind to.

To handle such cases we track the SSP unwinds in exception handling.
This is the same strategy as in #104820

Copy link
Contributor

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

@VSadov VSadov requested a review from janvorli July 10, 2024 02:29
@MichalStrehovsky
Copy link
Member

I've just run into this locally too. Do we understand why none of the CI testing caught this? Is there a 9.0 issue tracking CET testing?

@VSadov
Copy link
Member Author

VSadov commented Jul 10, 2024

Do we understand why none of the CI testing caught this?

Maybe typical lab runs use older hardware?

Yes, there are plans to have regular lab runs that have CET enabled. Also with CFG enabled as well.

@VSadov VSadov changed the title [NativeAOT] When reconciling shadow stack after catch, look for RhpThrowHwEx2 as well. [NativeAOT] When reconciling shadow stack after catch, use more precise way to figure how much to pop. Jul 13, 2024
@VSadov
Copy link
Member Author

VSadov commented Jul 13, 2024

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@VSadov
Copy link
Member Author

VSadov commented Jul 13, 2024

@janvorli - Switched to the "SSP on REGDISPLAY" approach.

PCODE IP;
PCODE IP;

#ifdef TARGET_AMD64
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to make this new member Windows only? The ifdefs around places that work with this are somehow inconsistent - some of them check for windows too and some don't.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to make it less platform-dependent initially, but as we moved through approaches it ended up being strictly wintel. Every piece working with this field is windows-only.
I will make the field windows-only as well.

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@VSadov
Copy link
Member Author

VSadov commented Jul 17, 2024

Thanks!!

@VSadov
Copy link
Member Author

VSadov commented Jul 17, 2024

I do not think Linux timeouts are related to this win-x64-CET specific change.

@VSadov VSadov merged commit f9016e3 into dotnet:main Jul 17, 2024
89 of 93 checks passed
@VSadov VSadov deleted the Fix104628 branch July 17, 2024 03:34
@github-actions github-actions bot locked and limited conversation to collaborators Aug 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Access violation while finding shadow stack pointer in nativeaot/SmokeTests/UnitTests
4 participants