Date: Mon, 17 Jun 2024 16:43:50 +0000
From: Aruna Ramakrishna <aruna.ramakrishna@...cle.com>
To: Jeff Xu <jeffxu@...omium.org>
CC: "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        Keith Lucas
	<keith.lucas@...cle.com>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>, Thomas
 Gleixner <tglx@...utronix.de>,
        "x86@...nel.org" <x86@...nel.org>,
        Andrew
 Brownsword <andrew.brownsword@...cle.com>,
        Matthias Neugschwandtner
	<matthias.neugschwandtner@...cle.com>,
        "jeffxu@...gle.com"
	<jeffxu@...gle.com>,
        "jannh@...gle.com" <jannh@...gle.com>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "sroettger@...gle.com"
	<sroettger@...gle.com>,
        "jorgelo@...omium.org" <jorgelo@...omium.org>,
        "rick.p.edgecombe@...el.com" <rick.p.edgecombe@...el.com>
Subject: Re: Re [PATCH v5 2/5] x86/pkeys: Add helper functions to update PKRU
 on sigframe



> On Jun 11, 2024, at 3:13 PM, Jeff Xu <jeffxu@...omium.org> wrote:
> 
> On Tue, Jun 11, 2024 at 7:05 AM Aruna Ramakrishna
> <aruna.ramakrishna@...cle.com> wrote:
>> 
>> 
>> 
>>> On Jun 10, 2024, at 2:39 PM, jeffxu@...omium.org wrote:
>>> 
>>> The orig_pkru & init_pkru_value is quite difficult to understand.
>>> 
>>> case 1> init_pkru: 00 (allow all)
>>> orig_pkru all cases  => allow all
>>> 
>>> case 2> init_pkru: 01 (disable all)
>>> Orig_pkru:
>>> allow all 00 => 00 allow all.
>>> disable all 01 => 01 disable all.
>>> disable write 10 => 00 allow all <--- *** odd ***
>>> disable all 11 => 01 disable all
>>> 
>>> case 3> init pkru: 10 (disable write)
>>> allow all 00 => 00 allow all.
>>> disable all 01 => 00 (allow all) <----*** odd ***
>>> disable write 10 => 10 allow all
>>> disable all 11 => 10 disable write <--- *** odd ***
>>> 
>>> case 4> init pkru: 11 (disable all)
>>> orig_pkru all cases => unchanged.
>>> 
>>> set PKRU(0) seems to be better, easy to understand.
>>> 
>> 
>> I’m not sure I follow.
>> 
>> The default init_pkru is 0x55555554 (enable only pkey 0). Let’s assume the application
>> sets up PKRU = 0x55555545 (i.e. enable only pkey 2). We want to set up the PKRU
>> to enable both pkey 0 and pkey 2, before the XSAVE, so that both the current stack as
>> well as the alternate signal stack are writable.
>> 
>> So with
>> write_pkru(orig_pkru & pkru_get_init_value());
>> 
>> It changes PKRU to 0x55555544 - enabling both pkey 0 and pkey 2.
>> 
> Consider below examples:
> 
> 1>
> The default init_pkru is 0x55555554 (pkey 0 has all access, 1-15 disable all)
> and thread has PKRU of 0xaaaaaaa8 (pkey 0 has all access, 1-15 disable write)
> init_pkru & curr_pkru will have 0x0
> If altstack is protected by pkey 1, your code will change PKRU to 0,
> so the kernel is able to read/write to altstack.
> 
> 2>
> However  when the thread's PKRU disable all access to 1-15:
> The default init_pkru is 0x55555554 (pkey 0 has all access, 1-15 disable all)
> and thread has PKRU of 0x5555554 (pkey 0 has all access, 1-15 disable all)
> init_pkru & curr_pkru will have 0x55555554
> If altstack is protected by pkey 1, kernel doesn't change PKRU, so
> still not able
> to access altstack.
> 
> 3> This algorithm is stranger if inti_pkru is configured differently:
> The init_pkru is 0xaaaaaaa8 (pkey 0 has all access, and 1-15 disables write.)
> and thread has PKRU of 0x55555554 (pkey 0 has all access, 1-15 disable all)
> init_pkru & curr_pkru will have 0x0 (0-15 has all access).
> 
> Overall I think this is a confusing algorithm to decide the new PKRU to use.

I understand your point. But the patch makes the assumption that the sigaltstack is
accessible with init_pkru, which is not the case in these 3 examples. I’m not saying
that assumption is right - just that, that’s the use case which we’re trying to fix here.

But you’re saying that the altstack could be using any pkey, and handle_signal()
does not know which one it is, so it should just enable all pkeys - is that right?

Thomas,
Can you please review/comment? 
 
> 
>> After the XSAVE, it calls update_pkru_in_sigframe(), which overwrites this (new)
>> PKRU saved on the sigframe with orig_pkru, which is 0x55555545 in this example.
>> 
>> Setting PKRU to 0 would be simpler, it would enable all pkeys - 0 through 15 - which,
>> as Thomas pointed out, seems unnecessary. The application needs the pkey it
>> enabled for access to its own stack, and we need to enable pkey 0 under the hood
>> to enable access to the alternate signal stack.
>> 
> 
> I think you are referring to Thomas's comments in V3, copy here for
> ease of response:
> 
>> User space resumes with the default PKRU value and the first thing user
>> space does when entering the signal handler is to push stuff on the
>> signal stack.
> ...
>> If user space protects the task stack or the sigalt stack with a key
>> which is not in init_pkru_value then it does not matter at all whether
>> it dies in handle_signal() or later when returning to user space, no?
> 
> The userspace could register a custom handler (written in assembly) to
> change PKRU and allow access to the stack, this could be written in such
> that it is before pushing stuff to the stack. So all it requires is
> that the kernel
> doesn't SIGSEGV when preparing the signal frame in sigaltstack, this is
> where PKRU=0 inside the kernel  path helps.
> 
> Even today, without patch, one can already do following:
> 1> use PKEY 1 to protect sigaltstack
> 3> let the thread have all access to PKEY 1
> 3> send a signal to the thread, kernel will save PKRU to the altstack correctly.
> 4> kernel set init_pkur before hands over control to userspace
> 5> userspace set PKRU to allow access to PKEY 1 as the first thing to do.
> 6> on sig_return, threads have PKRU restored correctly from the value
> in sigframe.
> 

In this example above, pkey 1 grants access to the altstack, which is enabled
through out this flow. I think perhaps this use case is more interesting:

1. Use pkey 1 to protect sigaltstack.
2. Use pkey 2 to protect thread’s stack.
3. Disable all but pkey 2 (= orig_pkru).
4. Send a signal to the thread (kernel patch needed; we'd have to enable all
pkeys in handle_signal() - i.e. pkru = 0).
5. Kernel sets pkru=init_pkru before handing over control to userspace (i.e.
only pkey 0 is enabled).
6. Signal handler should enable pkey 1 as the first thing.
7. On sigreturn, orig_pkru is restored correctly.

I’m not sure if (6) will work or if it’ll crash with a SIGSEGV - I haven’t tested
this flow.

Thanks,
Aruna

Powered by blists - more mailing lists