Re: [PATCH v8 3/5] x86/pkeys: Update PKRU to enable all pkeys before XSAVE

[Jeff Xu <jeffxu@chromium.org>]

Hi Aruna

On Fri, Aug 9, 2024 at 10:30 AM Jeff Xu <jeffxu@chromium.org> wrote:
>
> On Thu, Aug 1, 2024 at 11:13 PM Aruna Ramakrishna
> <aruna.ramakrishna@oracle.com> wrote:
> >
> > If the alternate signal stack is protected by a different pkey than the
> > current execution stack, copying xsave data to the sigaltstack will fail
> > if its pkey is not enabled in the PKRU register.
> >
> > We do not know which pkey was used by the application for the altstack,
> > so enable all pkeys before xsave.
> >
> > But this updated PKRU value is also pushed onto the sigframe, which
> > means the register value restored from sigcontext will be different from
> > the user-defined one, which is unexpected. Fix that by overwriting the
> > PKRU value on the sigframe with the original, user-defined PKRU.
> >
> > Signed-off-by: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>
> > ---
> >  arch/x86/kernel/fpu/signal.c | 11 +++++++++--
> >  arch/x86/kernel/signal.c     | 12 ++++++++++--
> >  2 files changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> > index 931c5469d7f3..1065ab995305 100644
> > --- a/arch/x86/kernel/fpu/signal.c
> > +++ b/arch/x86/kernel/fpu/signal.c
> > @@ -168,8 +168,15 @@ static inline bool save_xstate_epilog(void __user *buf, int ia32_frame,
> >
> >  static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf, u32 pkru)
> >  {
> > -       if (use_xsave())
> > -               return xsave_to_user_sigframe(buf);
> > +       int err = 0;
> > +
> > +       if (use_xsave()) {
> > +               err = xsave_to_user_sigframe(buf);
> > +               if (!err)
> > +                       err = update_pkru_in_sigframe(buf, pkru);
> > +               return err;
> > +       }
> > +
> >         if (use_fxsr())
> >                 return fxsave_to_user_sigframe((struct fxregs_state __user *) buf);
> >         else
> > diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> > index 9dc77ad03a0e..5f441039b572 100644
> > --- a/arch/x86/kernel/signal.c
> > +++ b/arch/x86/kernel/signal.c
> > @@ -102,7 +102,7 @@ get_sigframe(struct ksignal *ksig, struct pt_regs *regs, size_t frame_size,
> >         unsigned long math_size = 0;
> >         unsigned long sp = regs->sp;
> >         unsigned long buf_fx = 0;
> > -       u32 pkru = read_pkru();
> > +       u32 pkru;
> >
> >         /* redzone */
> >         if (!ia32_frame)
> > @@ -157,9 +157,17 @@ get_sigframe(struct ksignal *ksig, struct pt_regs *regs, size_t frame_size,
> >                 return (void __user *)-1L;
> >         }
> >
> > +       /* Update PKRU to enable access to the alternate signal stack. */
> > +       pkru = sig_prepare_pkru();
> I think, the better place to call sig_prepare_pkru() at begging of the
> get_sigframe:
>
> get_sigframe()
> {
>    /* ... */
>     if (ka->sa.sa_flags & SA_ONSTACK) {
>          if (sas_ss_flags(sp) == 0) {
>                 // set pkru = 0 <--- set pkru = 0 here.
>                 entering_altstack = true;
>          }
> }
> For two reasons:
> - We probably don't want all signaling handling going through "PKRU=0"
> , this includes the regular stack and nested signaling handling. The
> best place is when "entering the altstack". IIUC, this feature only
> enabled when sigaltstack() is used.
>  - The thread might not have read-access to the altstack, so we will
> want to make sure that pkru=0 is set before any read to the altstack.
> And IIRC,  fpu__alloc_mathframe needs read-access to the altstack.
> To help with testing, I will send a test case to demo the usage.
Apologies, I remembered it wrong, the fpu__alloc_mathframe doesn't
need read-access to altstack.

But I think the best place to set pkru=0 is still when the stack
entering altstack, as shown above. the reason is: The signal stack can
be nested, i.e. trigger another signaling inside signal handler and we
don't need to set pkru=0 multiple times (only the first time enter
sigaltstack)

> (please give me sometime to organize the test code, I'm hoping to send
> out before the end of next week)
>
the test code is placed at [1]
[1] https://github.com/peaktocreek/pkeytest

> Also, could you please consider adding a new flag SS_PKEYALTSTACK (see
> SS_AUTODISARM for example). Most existing apps that use sigaltstack()
> don't use PKEY and  don't care about setting PKRU=0, and don't need to
> overwrite the sig frame after XSAVE.  The flag will limit the impact
> of this patch.
>
Thanks
-Jeff

> Thanks
> Best regards,
> -Jeff
>
>
>
>
>
> >         /* save i387 and extended state */
> > -       if (!copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_size, pkru))
> > +       if (!copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_size, pkru)) {
> > +               /*
> > +                * Restore PKRU to the original, user-defined value; disable
> > +                * extra pkeys enabled for the alternate signal stack, if any.
> > +                */
> > +               write_pkru(pkru);
> >                 return (void __user *)-1L;
> > +       }
> >
> >         return (void __user *)sp;
> >  }
> > --
> > 2.39.3
> >