Re: [RFC PATCH] x86/pkeys: update PKRU to enable pkey 0 before XSAVE

[Aruna Ramakrishna]

> On Mar 15, 2024, at 4:05 PM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> 
> On Fri, Mar 15 2024 at 18:43, Aruna Ramakrishna wrote:
>>> On Mar 15, 2024, at 10:36 AM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>>>> But I’m not sure there is a “clean” way to do this. If there is, I’m
>>>> happy to redo the patch.
>>> 
>>> If it turns out to be required, desired whatever then the obvious clean
>>> solution is to hand the PKRU value down:
>>> 
>>>        setup_rt_frame()
>>>          xxx_setup_rt_frame()
>>>            get_sigframe()
>>>              copy_fpstate_to_sigframe()
>>> 
>>> copy_fpstate_to_sigframe() has the user fpstate pointer already so none
>>> of the __update_pkru_in_sigframe() monstrosities are required. No?
>>> 
>> 
>> Are you suggesting modifying all these functions down the chain from
>> handle_signal() to take in an additional parameter?
> 
> Yes.
> 
>> Wouldn’t that break kABI?
> 
> If so who cares?
> 
> kABI is an out of tree madness maintained by distros, but upstream has
> never supported it and never will. Aside of that kABI is a driver
> interface which hardly has anything to do with the low level
> architecture specific signal delivery code.
> 
> The kernel has no stable ABI, never had and never will have one, unless
> hell freezes over.
> 
>> In this approach too, the snippet where the value is modified on the
>> sigframe after xsave will remain unchanged, because we need the value
>> before xsave to match the register contents.
>> 
>> I guess what I’m saying is, half of __update_pkru_in_sigframe() will
>> remain unchanged - it would just be invoked from
>> copy_fpstate_to_sigframe() instead of handle_signal().
> 
> Yes, but that's the necessary and sane part of it.
> 
>> If there’s a way to do this without overwriting PKRU on the sigframe
>> after xsave, I'd like to understand that flow.
> 
> There is none for obvious reasons unless you figure out how to resolve a
> double circular hen and egg problem.
> 
>> Or if it’s just a matter of not needing to extract fpstate pointer in
>> handle_signal(), I can restructure it that way too.
> 
> It's not only the pointer retrieval. Updating xstate is obviously a FPU
> specific problem and the general signal handling code simply should not
> care. C does not provide encapsulation, but it does not prevent
> respecting it either.
> 
> Ideally we'd hide all of this in the FPU code, which is anyway the first
> one writing to the signal stack. The problem is the error case when any
> of the writes after saving the FPU frame terminaly faults or any other
> condition makes the signal delivery fail.
> 
> So handle_signal() should look like this:
> 
> /* Ensure that the signal stack is writeable */
>        pkru = sig_prepare_pkru();
> 
> failed = setup_rt_frame(ksig, regs, pkru);
> if (!failed) {
> /*
> * Clear the direction flag as per the ABI for function entry.
> *
> * Clear RF when entering the signal handler, because
> * it might disable possible debug exception from the
> * signal handler.
> *
> * Clear TF for the case when it wasn't set by debugger to
> * avoid the recursive send_sigtrap() in SIGTRAP handler.
> */
> regs->flags &= ~(X86_EFLAGS_DF|X86_EFLAGS_RF|X86_EFLAGS_TF);
> /*
> * Ensure the signal handler starts with the new fpu state.
> * This also ensures that the PKRU state is set to the
> * initial state.
> */
> fpu__clear_user_states(fpu);
> } else {
>         /* Restore the previous PKRU state */
>                sig_restore_pkru(pkru);
>        }
> 
> and then you'd have:
> 
> static inline bool copy_fpregs_to_sigframe(struct xregs_state __user *buf, u32 pkru)
> {
> if (use_xsave()) {
> if (!xsave_to_user_sigframe(buf))
>                 return false;
>                if (cpu_feature_enabled(X86_FEATURE_OSPKE))
>                 return !__put_user(pkru_address(buf), pkru);
>                return true;
>        }
> if (use_fxsr())
> return fxsave_to_user_sigframe((struct fxregs_state __user *) buf);
> else
> return fnsave_to_user_sigframe((struct fregs_state __user *) buf);
> }
> 
> And yes, I deliberately changed the return value of setup_rt_frame() to
> bool in this mockup because nothing cares about it. The only relevant
> information is whether if failed or not. That want's to be a separate
> patch obivously.
> 
> Thanks,
> 
>        tglx
> 
> 
> 

I’ll update the patch based on your feedback and send out a v2.

Thank you to both of you for your time - appreciate it!

Thanks,
Aruna