Don't Inline zero_page Function #87
Conversation
There is a current crash in patina on x86 when zero_page gets inlined in a certain way, small changes to the code generation cause no crash to occur. However, when zero_page gets inlined on x86, rdi in the outer function can get clobbered and not restored, which causes undefined behavior. In the example seen, the root page of the page table is supposed to be 0x50000, stored in rdi, but rdi gets used to track incrementing the zeroing writes to this page and ends up at 0x51000, which gets stored as the page table root, causing a write because there is a mismatch when this gets used later. There is a greater investigation needed here in two parts: - Whether this function is needed to be written in assembly. It was written so to avoid the compiler from optimizing out the memory write that it thinks may not be used, but after discussion with the compiler team, this can be avoided another way. However, that move is considered risky, as if the compiler decides to throw away a zeroing of the page table, we will have garbage entries, causing undefined behavior. - Whether this function should be written in a separate asm file as a global_asm import. Doing this avoids the use of inline asm and puts all register usage in our control. However, it also removes the niceties of inline_asm, namely only doing asm for the parts required to be so and doing the rest in Rust. Also, this is a larger change and so considered risky. In order to mitigate the current crash, the simplest change is made first: don't allow the zero_page functions to be inlined. This was seen to boot and under a debugger the proper push/pop of rdi was occurring. This is done for AARCH64 as well, even though it does not use explicit registers (x86 must in order to use the rep instruction) in order to keep the implementation similar and avoid similar potential problems. Other AARCH64 asm was reviewed and determined to not use explicit general purpose registers, and so left alone. Finally, the x86 function was incorrectly declaring that it preserves_flags, when it in fact explicitly clears the direction flag, so that is removed. There is a task filed in patina to review all usage of inline asm in patina and associated repos because there is a high chance that it has been done incorrectly; there are many common pitfalls that the Rust compiler can take when special care is not given to inline asm.
|
1.88 introduced naked functions which looks like another option for this scenario. |
|
Ever considered just move this to its own function into |
@kuqin12 , yeah, that’s what I was saying in the PR message, that that is an option, but for the immediate mitigation, I wanted to get the smallest change in to fix it, since the currently released version is broken. Moving to its own file works, but for the arm64 version we are actually blending Rust and asm, so decided let’s just fix this now, then get the opinion of the compiler team and come in with that fix. |
Yeah, I considered naked functions as well, but, as Kun was saying, I think global_asm may be more appropriate here. But, speculation aside, I have a question out to the compiler team on what they would expect the right move here would be and I’ll make a change based on their recommendation after mitigating the issue. |
Description
There is a current crash in patina on x86 when zero_page gets inlined in a certain way, small changes to the code generation cause no crash to occur. However, when zero_page gets inlined on x86, rdi in the outer function can get clobbered and not restored, which causes undefined behavior. In the example seen, the root page of the page table is supposed to be 0x50000, stored in rdi, but rdi gets used to track incrementing the zeroing writes to this page and ends up at 0x51000, which gets stored as the page table root, causing a write because there is a mismatch when this gets used later.
There is a greater investigation needed here in two parts:
In order to mitigate the current crash, the simplest change is made first: don't allow the zero_page functions to be inlined. This was seen to boot and under a debugger the proper push/pop of rdi was occurring. This is done for AARCH64 as well, even though it does not use explicit registers (x86 must in order to use the rep instruction) in order to keep the implementation similar and avoid similar potential problems. Other AARCH64 asm was reviewed and determined to not use explicit general purpose registers, and so left alone.
Finally, the x86 function was incorrectly declaring that it preserves_flags, when it in fact explicitly clears the direction flag, so that is removed.
There is a task filed in patina to review all usage of inline asm in patina and associated repos because there is a high chance that it has been done incorrectly; there are many common pitfalls that the Rust compiler can take when special care is not given to inline asm.
How This Was Tested
Tested booting Q35 and SBSA to Windows. Because this bug only repro'd on certain builds of the published crate, the real test will be releasing this and consuming it view the feed. However, disallowing inlining did work when viewed under the debugger.
Integration Instructions
N/A.