linux-kernel - Re: [RFC PATCH] x86/pkeys: update PKRU to enable pkey 0 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: <871q8bl7nf.ffs@tglx>
Date: Fri, 15 Mar 2024 18:36:04 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Aruna Ramakrishna <aruna.ramakrishna@...cle.com>, Dave Hansen
 <dave.hansen@...el.com>
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>
Subject: Re: [RFC PATCH] x86/pkeys: update PKRU to enable pkey 0 before XSAVE

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