Re: [PATCH v3 2/4] x86/pkeys: Add helper functions to update PKRU on sigframe

[Aruna Ramakrishna]

> On May 7, 2024, at 9:16 AM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> 
> On Thu, Apr 25 2024 at 18:05, Aruna Ramakrishna wrote:
>> This patch adds helper functions that will update PKRU value on the
> 
> git grep 'This patch' Documentation/process/
> 
> Also please explain WHY this is needed and not just what.


Patch 1/4 has the “why”. Should I expand more in that commit message?
Or are you specifically asking about why the helper functions are needed;
I can certainly elaborate on that here.

Thanks very much for your detailed review. I will make the required corrections.

Thanks,
Aruna
 
> 
>> sigframe after XSAVE.
> 
> ...
> 
>> +/*
>> + * Update the value of PKRU register that was already pushed
>> + * onto the signal frame.
>> + */
>> +static inline int
>> +__update_pkru_in_sigframe(struct xregs_state __user *buf, u32 pkru)
> 
> No line break and why does this need two underscores in the function name?
> 
>> +{
>> + int err = -EFAULT;
>> + struct _fpx_sw_bytes fx_sw;
>> + struct pkru_state *pk = NULL;
> 
> Why assign NULL to pk?
> 
> Also this wants to have a
> 
> if (unlikely(!cpu_feature_enabled(X86_FEATURE_OSPKE)))
>      return 0;
> 
> Instead of doing it at the call site.
> 
>> + if (unlikely(!check_xstate_in_sigframe((void __user *) buf, &fx_sw)))
> 
> What is this check for?
> 
> More interesting: How is this check supposed to succeed at all?
> 
> copy_fpstate_to_sigframe()
>  ....
>  copy_fpregs_to_sigframe()
>    xsave_to_user_sigframe();
>    __update_pkru_in_sigframe();
>  save_xstate_epilog();
> 
> check_xstate_in_sigframe() validates the full frame including what
> save_xstate_epilog() writes afterwards. So this clearly cannot work.
> 
>> + goto out;
> 
> What's wrong with 'return -EFAULT;'?
> 
>> + pk = get_xsave_addr_user(buf, XFEATURE_PKRU);
>> + if (!pk || !user_write_access_begin(buf, sizeof(struct xregs_state)))
>> + goto out;
> 
> Why user_write_access_begin()?
> 
>    1) The access to the FPU frame on the stack has been validated
>       already in copy_fpstate_to_sigframe() _before_
>       copy_fpregs_to_sigframe() is invoked.
> 
>    2) This does not require the nospec_barrier() as this is not a user
>       controlled potentially malicious access.
> 
>> + unsafe_put_user(pkru, (unsigned int __user *) pk, uaccess_end);
> 
> This type case would need __force to be valid for make C=1.
> 
> But that's not required at all because get_xsave_addr_user() should
> return a user pointer in the first place.
> 
>> +
>> + err = 0;
>> +uaccess_end:
>> + user_access_end();
>> +out:
>> + return err;
> 
> So none of the above voodoo is required at all.
> 
>       return __put_user(pkru, get_xsave_addr_user(buf, XFEATURE_PKRU));
> 
> Is all what's needed, no?
> 
>> +/*
>> + * Given an xstate feature nr, calculate where in the xsave
>> + * buffer the state is. The xsave buffer should be in standard
>> + * format, not compacted (e.g. user mode signal frames).
>> + */
>> +void *get_xsave_addr_user(struct xregs_state __user *xsave, int xfeature_nr)
> 
> void __user *
> 
>> +{
>> + if (WARN_ON_ONCE(!xfeature_enabled(xfeature_nr)))
>> + return NULL;
>> +
>> + return (void *)xsave + xstate_offsets[xfeature_nr];
> 
>  return (void __user *)....
> 
> Thanks,
> 
>        tglx