Page MenuHomeFreeBSD

Check for invalid sstatus values in set_mcontext().
ClosedPublic

Authored by jhb on Jan 16 2020, 10:52 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jan 4, 9:48 PM
Unknown Object (File)
Dec 10 2024, 6:06 PM
Unknown Object (File)
Dec 7 2024, 11:44 AM
Unknown Object (File)
Oct 12 2024, 8:46 AM
Unknown Object (File)
Sep 24 2024, 7:22 AM
Unknown Object (File)
Sep 24 2024, 12:03 AM
Unknown Object (File)
Sep 23 2024, 2:42 PM
Unknown Object (File)
Sep 23 2024, 4:27 AM
Subscribers

Details

Summary

Previously, this check was only in sys_sigreturn() which meant that
user applications could write invalid values to the register via
setcontext() or swapcontext().

Test Plan
  • booted riscv64 in spike

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 28712
Build 26727: arc lint + arc unit

Event Timeline

sys/riscv/riscv/machdep.c
375

I don't think this comment accurately describes what is being checked here. The SPP bit in sstatus is used to indicate the privilege mode in which the hart was executing before taking a trap. In this case we would expect to be coming from user mode, so we check that SPP == 0.

To check that interrupts haven't been tampered with, we would expect to see that the SPIE bit is set, indicating that supervisor interrupts were enabled before the trap was taken, which as the comment state we expect to always be true.

I don't think there would be any harm in checking both of these conditions.

sys/riscv/riscv/machdep.c
375

I had merely preserved the comment in this case, but I think you are saying to just add an SPIE check explicitly and then it will match the comment? It's probably best to do that as a followup I think if so.

mhorne added inline comments.
sys/riscv/riscv/machdep.c
375

Yes, that is what I meant. Doing it as a follow-up would be great.

This revision is now accepted and ready to land.Jan 17 2020, 5:17 PM
This revision was automatically updated to reflect the committed changes.
sys/riscv/riscv/machdep.c
375

Actually, I wonder if we should instead be treating this field a bit differently and either ignoring it altogether (so it's effectively a read-only field for userland) or only allowing userland to set the bits in ustatus and preserving all non-ustatus bits in tf_sstatus.