[PATCH v3 1/5] arm64: signal: Improve POR_EL0 handling to avoid uaccess failures

[Jeff Xu]

On Thu, Oct 31, 2024 at 1:45 AM Kevin Brodsky <kevin.brodsky at arm.com> wrote:
>
> On 30/10/2024 23:01, Jeff Xu wrote:
> >> -static int restore_poe_context(struct user_ctxs *user)
> >> +static int restore_poe_context(struct user_ctxs *user,
> >> +                              struct user_access_state *ua_state)
> >>  {
> >>         u64 por_el0;
> >>         int err = 0;
> >> @@ -282,7 +338,7 @@ static int restore_poe_context(struct user_ctxs *user)
> >>
> >>         __get_user_error(por_el0, &(user->poe->por_el0), err);
> >>         if (!err)
> >> -               write_sysreg_s(por_el0, SYS_POR_EL0);
> >> +               ua_state->por_el0 = por_el0;
> >>
> >>         return err;
> >>  }
> >> @@ -850,7 +906,8 @@ static int parse_user_sigframe(struct user_ctxs *user,
> >>  }
> >>
> >>  static int restore_sigframe(struct pt_regs *regs,
> >> -                           struct rt_sigframe __user *sf)
> >> +                           struct rt_sigframe __user *sf,
> >> +                           struct user_access_state *ua_state)
> >>  {
> >>         sigset_t set;
> >>         int i, err;
> >> @@ -899,7 +956,7 @@ static int restore_sigframe(struct pt_regs *regs,
> >>                 err = restore_zt_context(&user);
> >>
> >>         if (err == 0 && system_supports_poe() && user.poe)
> >> -               err = restore_poe_context(&user);
> >> +               err = restore_poe_context(&user, ua_state);
> >>
> >>         return err;
> >>  }
> >> @@ -908,6 +965,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;
> >> @@ -924,12 +982,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;
> >>
> > Do you need to move restore_altstack ahead of restore_sigframe?
>
> This is not necessary because restore_sigframe() no longer writes to
> POR_EL0. restore_poe_context() (above) now saves the original POR_EL0
> value into ua_state, and it is restore_user_access_state() (called below
> just before returning to userspace) that actually writes to POR_EL0,
> after all uaccess is completed.
>
Got it, thanks for the explanation.

-Jeff

> Having said that, I somehow missed the call to restore_altstack() when
> writing the commit message, so these changes in sys_rt_sigreturn are in
> fact necessary. Good catch! At least the patch itself should be doing
> the right thing.
>
> - Kevin
>
> > similar as x86 change [1],
> > the discussion for this  happened  in [2] [3]
> >
> > [1] https://lore.kernel.org/lkml/20240802061318.2140081-5-aruna.ramakrishna@oracle.com/
> > [2] https://lore.kernel.org/lkml/20240425210540.3265342-1-jeffxu@chromium.org/
> > [3] https://lore.kernel.org/lkml/d0162c76c25bc8e1c876aebe8e243ff2e6862359.camel@intel.com/
> >
> > Thanks
> > -Jeff
> >
> >
> >> +       restore_user_access_state(&ua_state);
> >> +
> >>         return regs->regs[0];
>