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

From: Aruna Ramakrishna
Date: Tue May 07 2024 - 13:16:15 EST



> 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