On 24/10/2024 12:59, Catalin Marinas wrote:
On Wed, Oct 23, 2024 at 04:05:09PM +0100, Kevin Brodsky wrote:
+/*
- Save the unpriv access state into ua_state and reset it to disable any
- restrictions.
- */
+static void save_reset_user_access_state(struct user_access_state *ua_state) +{
- if (system_supports_poe()) {
/** Enable all permissions in all 8 keys* (inspired by REPEAT_BYTE())*/u64 por_enable_all = (~0u / POE_MASK) * POE_RXW;I think this should be ~0ul.
It is ~0u on purpose, because unlike in REPEAT_BYTE(), I only wanted the lower 32 bits to be filled with POE_RXW (we only have 8 keys, the top 32 bits are RES0). That said, given that D128 has 4-bit pkeys, we could anticipate and fill the top 32 bits too (should make no difference on D64).
@@ -907,6 +964,7 @@ SYSCALL_DEFINE0(rt_sigreturn) { struct pt_regs *regs = current_pt_regs(); struct rt_sigframe __user *frame;
- struct user_access_state ua_state;
/* Always make any pending restarted system calls return -EINTR */ current->restart_block.fn = do_no_restart_syscall; @@ -923,12 +981,14 @@ SYSCALL_DEFINE0(rt_sigreturn) if (!access_ok(frame, sizeof (*frame))) goto badframe;
- if (restore_sigframe(regs, frame))
- if (restore_sigframe(regs, frame, &ua_state)) goto badframe;
if (restore_altstack(&frame->uc.uc_stack)) goto badframe;
- restore_user_access_state(&ua_state);
- return regs->regs[0];
badframe:
The saving part I'm fine with. For restoring, I was wondering whether we can get a more privileged POR_EL0 if reading the frame somehow failed. This is largely theoretical, there are other ways to attack like writing POR_EL0 directly than unmapping/remapping the signal stack.
What I'd change here is always restore_user_access_state() to POR_EL0_INIT. Maybe just initialise ua_state above and add the function call after the badframe label.
I'm not sure I understand. When we enter this function, POR_EL0 is set to whatever the signal handler set it to (POR_EL0_INIT by default). There are then two cases: 1) Everything succeeds, including reading the saved POR_EL0 from the frame. We then call restore_user_access_state(), setting POR_EL0 to the value we've read, and return to userspace. 2) Any uaccess fails (for instance reading POR_EL0). In that case we leave POR_EL0 unchanged and deliver SIGSEGV.
In case 2 POR_EL0 is most likely already set to POR_EL0_INIT, or whatever the signal handler set it to. It's not clear to me that forcing it to POR_EL0_INIT helps much. Either way it's doubtful that the SIGSEGV handler will be able to recover, since the new signal frame we will create for it may be a mix of interrupted state and signal handler state (depending on exactly where we fail).
Kevin