Re: [RFC PATCH] x86/pkeys: update PKRU to enable pkey 0 before XSAVE

[Aruna Ramakrishna]

> On Mar 15, 2024, at 10:36 AM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> 
> On Thu, Mar 14 2024 at 18:14, Aruna Ramakrishna wrote:
>>> On Mar 14, 2024, at 10:54 AM, Dave Hansen <dave.hansen@xxxxxxxxx> wrote:
>>> The need for this new feature is highly dependent on the threat model
>>> that it supports.  I'm highly dubious that there's a true need to
>>> protect against an attacker with arbitrary write access in the same
>>> address space.  We need to have a lot more information there.
>> 
>> I thought the PKRU value being reset in the signal handler was
>> supposed to be the default behavior. In which case, this is a bug.
>> 
>> "Signal Handler Behavior
>> Each time a signal handler is invoked (including nested signals),
>> the thread is temporarily given a new, default set of protection
>> key rights that override the rights from the interrupted context.”
>> 
>> (Ref: https://man7.org/linux/man-pages/man7/pkeys.7.html)
>> 
>> I'm not very familiar with protection keys (before I started looking
>> into this issue), so I apologize for misunderstanding.
>> 
>> fpu__clear_user_states() does reset PKRU, but that happens much later
>> in the flow. Before that, the kernel tries to save registers on to the
>> alternate signal stack in setup_rt_frame(), and that fails if the
>> application has explicitly disabled pkey 0 (and the alt stack is
>> protected by pkey 0). This patch attempts to move that reset a little
>> earlier in the flow, so that setup_rt_frame() can succeed.
>> 
>>> I haven't even more than glanced at the code.  It looks pretty
>>> unspeakably ugly even at a glance.
>> 
>> I agree with you - no argument there.
> 
> It's a horrible hack.
> 
>> But I’m not sure there is a “clean” way to do this. If there is, I’m
>> happy to redo the patch.
> 
> If it turns out to be required, desired whatever then the obvious clean
> solution is to hand the PKRU value down:
> 
>         setup_rt_frame()
>           xxx_setup_rt_frame()
>             get_sigframe()
>               copy_fpstate_to_sigframe()
> 
> copy_fpstate_to_sigframe() has the user fpstate pointer already so none
> of the __update_pkru_in_sigframe() monstrosities are required. No?
> 

(Resending with some edits; I’m not sure why my previous message did not make it to the mailing list.)

I’m not sure I fully understand.

Are you suggesting modifying all these functions down the chain from handle_signal() to take in an additional parameter? Wouldn’t that break kABI?

In this approach too, the snippet where the value is modified on the sigframe after xsave will remain unchanged, because we need the value before xsave to match the register contents.

I guess what I’m saying is, half of __update_pkru_in_sigframe() will remain unchanged - it would just be invoked from copy_fpstate_to_sigframe() instead of handle_signal().

       pk = get_xsave_standard_addr((struct xregs_state __user *) buf,
                       XFEATURE_PKRU);
       if (!pk || !user_write_access_begin((struct xregs_state __user *) buf,
                               sizeof(struct xregs_state)))
               goto out;
       unsafe_put_user(new_pkru, (unsigned int __user *) pk, uaccess_end);

Right?

If I’ve misunderstood something, I apologize. If there’s a way to do this without overwriting PKRU on the sigframe after xsave, I'd like to understand that flow. Or if it’s just a matter of not needing to extract fpstate pointer in handle_signal(), I can restructure it that way too.

Thanks,
Aruna

> Thanks,
> 
>        tglx