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
> 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
> >
