linux-kernel - Re: [PATCH v3 3/4] x86/pkeys: Update PKRU to enable all pkeys before XSAVE

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: <FF998E58-D109-45B1-9BD8-FEF873E2FA7A@oracle.com>
Date: Tue, 7 May 2024 17:34:16 +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 3/4] x86/pkeys: Update PKRU to enable all pkeys before
 XSAVE


> On May 7, 2024, at 9:47 AM, Thomas Gleixner <tglx@...utronix.de> wrote:
> 
> On Thu, Apr 25 2024 at 18:05, Aruna Ramakrishna wrote:
> 
>> If the alternate signal stack is protected by a different pkey than the
>> current execution stack, copying xsave data to the altsigstack will fail
>> if its pkey is not enabled. This commit enables all pkeys before
>> xsave,
> 
> This commit (patch) ....
> 
> Also this lacks any justification why this enables all pkeys and how
> that is the right thing to do instead of using init_pkru_value which
> is what is set by fpu__clear_user_states() before going back to user
> space. For signal handling this can be the only valid PKEY state unless
> I'm missing something here.


If the alt sig stack is protected by a different pkey (other than pkey 0), then
this flow would need to enable that, along with the pkey for the thread’s 
stack. Since the code has no way of knowing what pkey the altstack needs,
it enables all for this brief window.

> 
>> static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf,
>>  u32 pkru)
>> {
>> - if (use_xsave())
>> - return xsave_to_user_sigframe(buf);
>> + int err = 0;
>> +
>> + if (use_xsave()) {
>> + err = xsave_to_user_sigframe(buf);
>> + if (!err && cpu_feature_enabled(X86_FEATURE_OSPKE))
> 
> The CPU feature check really wants to be in update_pkru_in_sigframe()
> 
>> @@ -278,6 +278,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
>> if (stepping)
>> user_disable_single_step(current);
>> 
>> + pkru = sig_prepare_pkru();
> 
> pkru is defined in the first patch:
> 
>> +       u32 pkru = read_pkru();
> 
> Why do we need a read and then another read in sig_prepare_pkru()?

sig_prepare_pkru() is where the pkru value is updated to make sure the alt stack is
accessible for XSAVE, while capturing the user-defined orig_pkru so that it can be
correctly restored later. Two reads are obviously redundant - I will remove the first one
(introduced in patch 1) here. 

Thanks,
Aruna