On Wed, Oct 23, 2024 at 04:05:09PM +0100, Kevin Brodsky wrote:
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index f5fb48dabebe..d2e4e50977ae 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -66,9 +66,63 @@ struct rt_sigframe_user_layout { unsigned long end_offset; }; +/*
- Holds any EL0-controlled state that influences unprivileged memory accesses.
- This includes both accesses done in userspace and uaccess done in the kernel.
- This state needs to be carefully managed to ensure that it doesn't cause
- uaccess to fail when setting up the signal frame, and the signal handler
- itself also expects a well-defined state when entered.
- */
+struct user_access_state {
- u64 por_el0;
+};
#define TERMINATOR_SIZE round_up(sizeof(struct _aarch64_ctx), 16) #define EXTRA_CONTEXT_SIZE round_up(sizeof(struct extra_context), 16) +/*
- 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.
@@ -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.
Either way:
Reviewed-by: Catalin Marinas catalin.marinas@arm.com