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

[Aruna Ramakrishna]

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

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.

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