lkml.org 
[lkml]   [2024]   [May]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v3 3/4] x86/pkeys: Update PKRU to enable all pkeys before XSAVE
Date

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

\
 
 \ /
  Last update: 2024-05-27 18:19    [W:0.132 / U:0.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site