linux-kernel - Re: [RFC] Restore PKRU to user-defined value after signal handling

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]

Message-ID: <9BA465A2-905D-4D0E-87A6-AB89C28A7B4F@oracle.com>
Date: Wed, 6 Nov 2024 19:40:17 +0000
From: Aruna Ramakrishna <aruna.ramakrishna@...cle.com>
To: Dave Hansen <dave.hansen@...el.com>
CC: Thomas Gleixner <tglx@...utronix.de>,
        "mingo@...hat.com"
	<mingo@...hat.com>,
        "dave.hansen@...ux.intel.com"
	<dave.hansen@...ux.intel.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Rudi Horn
	<rudi.horn@...cle.com>, Joe Jin <joe.jin@...cle.com>,
        Jeff Xu
	<jeffxu@...omium.org>
Subject: Re: [RFC] Restore PKRU to user-defined value after signal handling


> On Nov 6, 2024, at 11:27 AM, Dave Hansen <dave.hansen@...el.com> wrote:
> 
> On 11/6/24 10:33, Aruna Ramakrishna wrote:
>> static inline int update_pkru_in_sigframe(struct xregs_state __user *buf, u32 pkru)
>> {
>> +       int err = 0;
>> +
>>        if (unlikely(!cpu_feature_enabled(X86_FEATURE_OSPKE)))
>>                return 0;
>> -       return __put_user(pkru, (unsigned int __user *)get_xsave_addr_user(buf, XFEATURE_PKRU));
> 
> Let me try to summarize that whole email:
> 
> The existing code updates the PKRU value in the XSAVE buffer.  But it
> does not update ->xfeatures[PKRU].  If ->xfeatures[PKRU]==0, then XRSTOR
> will ignore the data that __put_user() put in place.
> 
> How does ->xfeatures[PKRU] end up set to 0?  On AMD, a WRPKRU(0) sets
> PKRU=0 *and* XINUSE[PKRU]=0.  Intel doesn't do that.  Either behavior is
> architecturally permitted.
> 
> Did I miss anything?

Nope, this is correct.

> 
> But the suggested fix is just beyond hideous.  Can't we just use the
> mask that xsave_to_user_sigframe() generated instead of reading it back
> out of userspace three seconds after it is written?
> 
> static inline int update_pkru_in_sigframe(..., u32 mask)
> {
> u32 xinuse;
> int err;
> 
>        if (unlikely(!cpu_feature_enabled(X86_FEATURE_OSPKE)))
>                return 0;
> 
> /* Ensure XRSTOR picks up the new PKRU value from the buffer: */
> xinuse = (mask & xfeatures_in_use()) | XFEATURE_MASK_PKRU;
> 
> err =  __put_user(xinuse, &buf->header.xfeatures);
> if (err)
> return err;
> 
>        return ... existing code here;
> }

Ah, I missed xfeatures_in_use(). This is a better implementation.

> 
> This probably means moving update_pkru_in_sigframe() to the end of
> xsave_to_user_sigframe() instead of calling it after, though.
> 

I do not understand why it has to be moved. Would you mind explaining?

Thank you for your feedback, I’ll redo the patch and test again.

Thanks,
Aruna

> But either way, this is all horrific.  It's yet another reason that the
> XSAVE architecture complexity hurts more than it helps.  We want PKRU
> written out here, dammit.  We shouldn't have to ask the hardware to
> write it out, and _then_ go back and do it ourselves.