Date: Thu, 20 Jun 2024 12:23:05 -0700
From: Jeff Xu <jeffxu@...omium.org>
To: Aruna Ramakrishna <aruna.ramakrishna@...cle.com>
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

Hi Aruna

On Mon, Jun 17, 2024 at 9:43 AM Aruna Ramakrishna
<aruna.ramakrishna@...cle.com> wrote:
>
>
>
> > 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.
>
Consider below case:
1> let init_pkur be 0x55555554(disable all for key 1-15, allow access to pkey 0
2> let the current pkur 0xcccccc1 (disable write for key  0 and 2-15,
allow access to pkey 1)
The new pkru will be 0x0. It gives  access to all pkeys, IIUC,
your scenario just needs to enable all access to pkey 0, right ?

I give this example to show the algorithm is difficult for dev to
reasable about.

> 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?
>
Yes, along that line of thinking.

Since the kernel (handle_signal) now needs to modify PKRU to have access to
sigaltstack, which is a behavior change, and altstack can be protected
by any PKEYs,
setting the PKRU to zero is the simplest solution.

I agree with Thomos that this will open up more access, we could let
applications
explicitly ask this by setting a new ss_flag when calling sigaltstack() call.
e.g.
stack_t altstack;
altstack.ss_sp = ptr;
altstack.ss_flags = 0;
altstack.ss_flags |= SS_PKEYALTSTACK;  <----
int ret = sigaltstack(&altstack, NULL);


> 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.
>
User space can write assembly code to change PKRU before the rest of the calling
conversation, e.g. push local to stack.
Try below code:

/*
 * change PKRU before calling the inner.
 *  disclaim: not expect in ASM, so this code might have bug
*/
void asm_handler(int signum, siginfo_t *si, void *vucontext);
__asm__(
".global asm_handle\n"
"asm_handler:\n"
" mov %rdx, %r8\n"
" xor %eax, %eax\n"
" xor %ecx, %ecx\n"
" xor %edx, %edx\n"
" wrpkru\n"
" mov %r8, %rdx\n"
" call inner\n"
" ret\n"
);

void inner(int signum, siginfo_t *si, void *ptr)
{
...
}


struct sigaction sa;
 ...
sa.sa_sigaction = asm_handler;
sa.sa_flags = SA_ONSTACK | SA_SIGINFO;
ret = sigaction(SIGUSR1, &sa, NULL);

Thanks
-Jeff

> Thanks,
> Aruna
>

Powered by blists - more mailing lists