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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 7 May 2024 17:15:41 +0000
From: Aruna Ramakrishna <aruna.ramakrishna@...cle.com>
To: Thomas Gleixner <tglx@...utronix.de>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "dave.hansen@...ux.intel.com"
	<dave.hansen@...ux.intel.com>,
        Ingo Molnar <mingo@...nel.org>, Keith Lucas
	<keith.lucas@...cle.com>
Subject: Re: [PATCH v3 2/4] x86/pkeys: Add helper functions to update PKRU on
 sigframe


> On May 7, 2024, at 9:16 AM, Thomas Gleixner <tglx@...utronix.de> 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