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.mari...@arm.com>
