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