Message-ID: <20240425210540.3265342-1-jeffxu@chromium.org>
Date: Thu, 25 Apr 2024 21:05:40 +0000
From: jeffxu@...omium.org
To: aruna.ramakrishna@...cle.com
Cc: andrew.brownsword@...cle.com,
	dave.hansen@...ux.intel.com,
	linux-kernel@...r.kernel.org,
	matthias.neugschwandtner@...cle.com,
	tglx@...utronix.de,
	jeffxu@...omium.org,
	jeffxu@...gle.com,
	jannh@...gle.com,
	sroettger@...gle.com,
	x86@...nel.org
Subject: Re: [RFC PATCH v2 1/1] x86/pkeys: update PKRU to enable pkey 0 before XSAVE

From: Jeff Xu <jeffxu@...omium.org>

On 3/21/24 14:56, Aruna Ramakrishna wrote:
>Enabling both the non-zero pkey (for the thread) and pkey zero (in
>userspace) will not work for us. We cannot have the alt stack writeable
>by all - the rationale here is that the code running in that thread
>(using a non-zero pkey) is untrusted and should not have access to the
>alternate signal stack (that uses pkey zero), to prevent the return
>address of a function from being changed. The expectation is that kernel
>should be able to set up the alternate signal stack and deliver the
>signal to the application even if pkey zero is explicitly disabled by
>the application. The signal handler accessibility should not be dictated
>by the PKRU value that the thread sets up.
>
We have a similar threat model that we don't want "untrusted threads" to
access altstack. I think this patch need not be restricted to the
use case of zero pkey for altstack, i.e. application can also set
non-zero pkey to altstack and expect the same.

>Solution:
>The PKRU register is managed by XSAVE, which means the sigframe contents
>must match the register contents - which is not the case here. We want
>the sigframe to contain the user-defined PKRU value (so that it is
>restored correctly from sigcontext) but the actual register must be
>reset to init_pkru so that the alt stack is accessible and the signal
>can be delivered to the application. It seems that the proper fix here
>would be to remove PKRU from the XSAVE framework and manage it
>separately, which is quite complicated. As a workaround, this patch does
>something like this:
>
>        orig_pkru = rdpkru();
>        wrpkru(init_pkru & orig_pkru);
>        xsave_to_user_sigframe();
>        put_user(pkru_sigframe_addr, orig_pkru)
>
The default PKRU of thread [1] is set as 01 (disable access) for each PKEY
from 1 to 15, and 00 (RW) for PKEY 0.

Let's use pkey 1 as an example:
The init_pkru is 01, if the thread has PKRU (orig_pkru) as 10 (disable write
but have read) then new_pkru from (init_pkru & orig_pkru) is 00, which gives
RW access to the pkey 1.

When the thread has orig_pkru as 01 (disable access) or 00 (RW), new_pkru is
unchanged from orig_pkru.

Now take pkey 0:
the init_pkru is 00, regardless what threads has, new_pkru will always be 00.

This seems to work out well for pkey 1 to 15, i.e. signal handing code in
kernel only give write access when the thread alrady has read access to the
PKEY that is used by the altstack. The threat model interesting here is to
prevent untrusted threads from writing to altstack, and read is probably less
of a problem.

Does this meet what you want? (Note the pkey 0 is different than 1-15)

Suppose someone also like to disable all access to altstack, then there is one
more place to mind: in sigreturn(), it calls restore_altstack(), and requires
read access to altstack. However, at the time, PKRU is already restored from
sigframe, so SEGV will raise (the value in sigframe doesn't have read access
to the PKEY).

Without changing sigreturn, using wrpkru(0) here might not be necessary:
the dispatch to user space works fine, only to crash at sigreturn step.

[1] defined by init_pkru_value in pkeys.c

Best regards,
-Jeff

Powered by blists - more mailing lists