<snip> + Jesse, he is working on the glibc side + CC a few others on
that thread as submitted on the glibc side from sifive. Sorry for the
delay, I was away on vacation.
unsigned long locked = 0x2; // Kernel Task State -> LOCK WRITE
unsigned long cur_val = 0x3; // Kernel Task State -> WRITE and SHADOWSTACK
ENABLED
unsigned long new_val = 0x0; // Userspace Feature Change via syscall ->
DISABLE
| x86-64 | risc-v | arm64 |
| ---------- | -------- | --------- |
| Works | Fails | Fails |
I would not have expected that combination to work at all with the
prctl() (as opposed to arch_prctl()) interface TBH, if you've locked
write on you shouldn't be able to disable it. The reason that works on
x86 at the minute is that for x86 you can only change one bit at a time
so the new value when disabling is effectively 0x2, not 0x0.
Yes, this is exactly what I am pointing out. Implementation aside, is that
the behavior we want?
I am proposing and have questions over the following:
1. What should the behavior be if you had write locked and disable the
shadow stack?
- I can argue both ways here, -EPERM or success. I think I and most arches
lead to failure.
Given that RISC-V doesn't support control of writes it's moot there
at the minute, and x86 currently uses arch_prctl() so will need an
additional API, it seems the path of least resistance is to allow it.
This also avoids locking writes (or pushes, for arm64) on effectively
also locking enable which seems neater.
2. riscv should check that the low bit is set in locking not just that its
0, it should be 1
I think for ABI compatibility RISC-V will have to continue accepting 0
as being equivalent to locking PR_SHADOW_STACK_ENABLE (or everything,
but it only supports that one bit right now).
TL;DR - No users, lets fix it before risc-v lands the userspace side IIUC
I can't find any libc's that support this for risc-v. It appears the glibc
patches were not merged and I commented on those patches just now
that the interface is wrong:
-
https://inbox.sourceware.org/libc-alpha/[email protected]/
Additionally, stress-ng does it "generically", and it would be broken on
a riscv system:
-
https://sources.debian.org/src/stress-ng/0.22.00-2/stress-prctl.c?hl=1144#L1144
This is a bug and never followed the convention to begin with. So risc-v is
non-compliant to the spec and this effectively prevents the a true
unification
of a generic prctl interface. The behavior on riscv doesn't adhere to
there own docs:
-
https://cdn.kernel.org/doc/html/latest/arch/riscv/zicfiss.html#prctl-enabling
3. riscv should return -EPERM vs -EINVAL
If you mean for arch_lock_shadow_stack_status() I think -EINVAL is a
sensible error code when the system or task does not support shadow
stacks, I'm not sure we should return -EPERM at all. On arm64 we
support locking any bit, not just the ones that we currently know about.
This is for future proofing, userspace can lock unknown flags.
No, I mean when setting a locked bit via prctl
and PR_SET_SHADOW_STACK_STATUS.
Currently, the error codes for changing a locked bit:
x86: EPERM
arm64: EBUSY (Which we discussed offline about changing to EPERM)
risc-v:
If we can all agree on item 1, that locked bits check can be refactored and
shared in one of
two ways:
1. within prctl itself, before the arch hook is called, we would need
helpers per-arch to extract the thread features and lock bits
2. as a helper where folks just pass the unsigned long of the bits to get
the result
For me option 1 seems a bit nicer, and moves more of the implementation
into generic code which is something we should really be doing in
general with the shadow stack support - there's a lot of cross arch
duplication at the minute. It has been on my list to look at this
repitition at some point, it had been held up by the clone3() stuff but
that seems to have died a death for now.
Agreed, it's more work, but I like it better.
