by Jeff Xu

[permalink] [raw]

Subject: Re: Re [PATCH v5 2/5] x86/pkeys: Add helper functions to update PKRU on sigframe

On Tue, Jun 11, 2024 at 7:05 AM Aruna Ramakrishna
<[email protected]> wrote:
>
>
>
> > On Jun 10, 2024, at 2:39 PM, [email protected] 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.

> 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.

That is why I consider "let kernel to modify PKRU to give access to
altstack" is
an ABI change, i.e. compare with current behavior that kernel deny
such access.
It might be good to consider adding a new flag in ss_flags.

Thanks.
-Jeff

2024-06-11 22:17:13

by Jeff Xu

[permalink] [raw]

Subject: Re: Re [PATCH v5 4/5] x86/pkeys: Restore altstack before sigcontext

On Tue, Jun 11, 2024 at 7:08 AM Aruna Ramakrishna
<[email protected]> wrote:
>
>
>
> > On Jun 10, 2024, at 2:44 PM, [email protected] wrote:
> >
> > Can we we move this patch to the first of the series? this can be an
> > independent patch, the problem not only affect PKRU, but also other
> > scenarios, as Rick pointed out in [1]
> >
> > [1] https://lore.kernel.org/lkml/[email protected]/
> >
> > -Jeff
>
> For this patch set, the issue with rt_sigreturn() is only exposed after patch 3/5 - i.e., when
> copy_fpregs_to_sigframe() calls update_pkru_in_sigframe() to update the PKRU value to
> user-specified PKRU that might disable pkey 0 access, thus breaking restore_altstack().
> So it seemed logical to me to have this fix as patch 4/5 of this series. I’m not strongly
> opposed to moving this to patch 1/5, but this ordering is easier to understand (I think).
> But if this patch needs to be broken out of this patchset and submitted independently
> (for the other scenarios you mentioned) - I can do that.
>
My main thought is that Rick mentioned this can be a cross-arch changes [1]
and we can start with x86/64 and other arches might follow.

[1] https://lore.kernel.org/lkml/[email protected]/

Thanks
-Jeff

> Thanks,
> Aruna