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

[Jeff Xu]

Hi Aruna

On Thu, Oct 3, 2024 at 4:29 PM Aruna Ramakrishna
<aruna.ramakrishna@xxxxxxxxxx> wrote:
>
>
>
> > On Aug 26, 2024, at 5:53 PM, Jeff Xu <jeffxu@xxxxxxxxxxxx> wrote:
> >
> > Hi Aruna
> >
> > On Fri, Aug 9, 2024 at 10:30 AM Jeff Xu <jeffxu@xxxxxxxxxxxx> wrote:
> >>
> >> On Thu, Aug 1, 2024 at 11:13 PM Aruna Ramakrishna
> >> <aruna.ramakrishna@xxxxxxxxxx> 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@xxxxxxxxxx>
> >>> ---
> >>> 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
> >
>
> Hi Jeff,
>
> I apologize for the delay in my response. I haven’t had a chance to optimize
> this patchset or try new test cases.
>
Not a problem, I understand.

> I agree with your first suggestion that we can set pkru to 0 only when
> entering_altstack = true, as that is the intention of this flow anyway.
>
> I’m not so sure if SS_PKEYALTSTACK is really necessary - theoretically it seems
> logical to not do this for applications that do not use pkeys but use altstack, but
> it adds extra code/checks for possibly insignificant performance gain. I have not
> tested this yet.
>
One of the reasons that I'm thinking about is that signaling handling
has real-time requirements for some real-time systems, and applying
PKRU=0 without checking will increase cost to those systems and might
be viewed as a regression.

Thanks
-Jeff

> I’ll try to retest and send out a new patchset on top of the previous ones that have
> been merged to 6.12.
>
> Thank you for your feedback, and your attention to detail - I appreciate it.
>
> Thanks,
> Aruna
>
>