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

[Aruna Ramakrishna]

> On Nov 6, 2024, at 11:27 AM, Dave Hansen <dave.hansen@xxxxxxxxx> 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.