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

Enabling SMEP and SMAP #473

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Enabling SMEP and SMAP #473

wants to merge 3 commits into from

Conversation

p4zuu
Copy link
Collaborator

@p4zuu p4zuu commented Sep 30, 2024

Enabling SMEP and SMAP if supported by the CPU.

To enable these, smep and smap features have to be specifically enabled, but this could be changed in the future since I think all modern CPUs support SMEP and SMAP.

I stole the way to give the CFG_SMAP bool to assembly from #455. This will likely cause a small conflict in kernel/src/cpu/idt/svsm.rs:265 when one of the two PR is merged.
This also conflict with the userspace PR since SMAP has to be temporary disabled to fetch user-provided pointers in syscalls arguments.

Finally, SMAP requires RFLAGS.AC to be unset to be enabled so I added a clac instruction in different entries, including #HV entry for which I don't really know if this is required, and if this could break something in the future. @msft-jlange could you please take a look?

@msft-jlange
Copy link
Contributor

Finally, SMAP requires RFLAGS.AC to be unset to be enabled so I added a clac instruction in different entries, including #HV entry for which I don't really know if this is required, and if this could break something in the future. @msft-jlange could you please take a look?

CLAC should be cleared in every exception handler so that any event handler that is invoked while executing in kernel mode can transition to EFLAGS.AC=0 while it is executing. The #HV handler is no exception. Every event handler that wants to access user-mode code should explicitly be required to execute STAC when user-mode access is expected, and the #HV handler is no different. You have put CLAC in the right place.

@msft-jlange
Copy link
Contributor

Why are the smap and smep features optional? Is there a reason we wouldn't want them enabled for all builds? I worry that making them optional is going to lead to regressions pretty quickly as most testing will occur with SMAP disabled, and that will mask important bugs that are only observed with SMAP enabled.

@p4zuu
Copy link
Collaborator Author

p4zuu commented Oct 1, 2024

CLAC should be cleared in every exception handler so that any event handler that is invoked while executing in kernel mode can transition to EFLAGS.AC=0 while it is executing. The #HV handler is no exception. Every event handler that wants to access user-mode code should explicitly be required to execute STAC when user-mode access is expected, and the #HV handler is no different. You have put CLAC in the right place.

Perfect, thanks!

@p4zuu
Copy link
Collaborator Author

p4zuu commented Oct 1, 2024

Why are the smap and smep features optional? Is there a reason we wouldn't want them enabled for all builds? I worry that making them optional is going to lead to regressions pretty quickly as most testing will occur with SMAP disabled, and that will mask important bugs that are only observed with SMAP enabled.

I was more looking for feedback here. I had the feeling that it was worth enabling smap and smep by default since all modern CPUs have it, but was a bit unsure. The argument of testing without them rings a bell now and very makes sense to me. I'm switching to enable them by default.

kernel/src/cpu/x86/smap.S Outdated Show resolved Hide resolved
@p4zuu p4zuu force-pushed the smep_smap branch 4 times, most recently from fe09d11 to f7b9d74 Compare October 1, 2024 10:08
kernel/src/cpu/x86/smap.rs Outdated Show resolved Hide resolved
@joergroedel
Copy link
Member

I was more looking for feedback here. I had the feeling that it was worth enabling smap and smep by default since all modern CPUs have it, but was a bit unsure. The argument of testing without them rings a bell now and very makes sense to me. I'm switching to enable them by default.

Yes, please enable the features by default.

@joergroedel joergroedel added the in-review PR is under active review and not yet approved label Oct 2, 2024
kernel/src/cpu/idt/common.rs Outdated Show resolved Hide resolved
We could use PageFaultError in different places, instead of
a hardcoded const.

Signed-off-by: Thomas Leroy <[email protected]>
SMEP can be enable by setting bit 20 of CR4.
A #PF will be raised if the CPU tries to fetch an instruction from
a user page while running in CPL < 3.

Signed-off-by: Thomas Leroy <[email protected]>
Enable SMAP if supported.

If supported, SMAP is enabled if CR4.SMAP bit is set, and if RFLAGS.AC is
unset.
It means that we need to clear RFLAGS.AC in entries, to have the kernel
running with RFLAGS.AC = 0.

Two asm macros (asm_clac and asn_stac) have been created to respectively
clear and set RFLAGS.AC from assembly. Two Rust functions have also been
created to perform exactly the same but from Rust code. This is still unused
but it will will be useful when we'll have CPL3 support, to be able to
read/write in userspace (eg. in syscall handlers, or even with user #VC).

Signed-off-by: Thomas Leroy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-review PR is under active review and not yet approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants