Received-SPF: pass (google.com: domain of linux-kernel+bounces-104733-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Thomas Gleixner <tglx@linutronix.de>
To: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>, Dave Hansen
 <dave.hansen@intel.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
 "x86@kernel.org" <x86@kernel.org>, "dave.hansen@linux.intel.com"
 <dave.hansen@linux.intel.com>
Subject: Re: [RFC PATCH] x86/pkeys: update PKRU to enable pkey 0 before XSAVE
In-Reply-To: <349322E5-5E6C-48D5-BA12-1CF47CE82930@oracle.com>
References: <20240314172920.2708810-1-aruna.ramakrishna@oracle.com>
 <4bd2aea0-3cea-4ef8-9607-40447cd531e5@intel.com>
 <349322E5-5E6C-48D5-BA12-1CF47CE82930@oracle.com>
Date: Fri, 15 Mar 2024 18:36:04 +0100
Message-ID: <871q8bl7nf.ffs@tglx>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

On Thu, Mar 14 2024 at 18:14, Aruna Ramakrishna wrote:
>> On Mar 14, 2024, at 10:54=E2=80=AFAM, 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.=E2=80=9D
>
> (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=E2=80=99m not sure there is a =E2=80=9Cclean=E2=80=9D way to do thi=
s. If there is, I=E2=80=99m
> 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