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

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, Mar 14 2024 at 18:14, Aruna Ramakrishna wrote:
>> On Mar 14, 2024, at 10:54 AM, Dave Hansen <dave.hansen@intel.com> 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?

Thanks,

        tglx