[PATCH 0/5] Improve arm64 pkeys handling in signal delivery

Tue Oct 22 03:40:45 PDT 2024

On Tue, Oct 22, 2024 at 11:31 AM Pierre Langlois
<pierre.langlois at arm.com> wrote:
>
> Dave Martin <Dave.Martin at arm.com> writes:
>
> > On Mon, Oct 21, 2024 at 12:06:25PM +0200, Kevin Brodsky wrote:
> >> On 17/10/2024 17:48, Dave Martin wrote:
> >> > On Thu, Oct 17, 2024 at 02:39:04PM +0100, Kevin Brodsky wrote:
> >> >> This series is a follow-up to Joey's Permission Overlay Extension (POE)
> >> >> series [1] that recently landed on mainline. The goal is to improve the
> >> >> way we handle the register that governs which pkeys/POIndex are
> >> >> accessible (POR_EL0) during signal delivery. As things stand, we may
> >> >> unexpectedly fail to write the signal frame on the stack because POR_EL0
> >> >> is not reset before the uaccess operations. See patch 3 for more details
> >> >> and the main changes this series brings.
> >> >>
> >> >> A similar series landed recently for x86/MPK [2]; the present series
> >> >> aims at aligning arm64 with x86. Worth noting: once the signal frame is
> >> >> written, POR_EL0 is still set to POR_EL0_INIT, granting access to pkey 0
> >> >> only. This means that a program that sets up an alternate signal stack
> >> >> with a non-zero pkey will need some assembly trampoline to set POR_EL0
> >> >> before invoking the real signal handler, as discussed here [3].
> >> > This feels a bit bogus (though it's anyway orthogonal to this series).
> >>
> >> I'm not very fond of this either. However I believe this is the correct
> >> first step: bring arm64 in line with x86. Removing all restrictions
> >> before uaccess and then setting POR_EL0 to POR_EL0_INIT enables
> >> userspace to use any pkey for the alternate signal stack without an ABI
> >> change, albeit not in a very comfortable way (if the pkey is not 0).
> >
> > I see: we try not to prevent userspace from using whatever pkey it
> > likes for the alternate signal stack, but we are only permissive for
> > the bare minimum operations that userspace can't possibly control for
> > itself (i.e., writing the signal frame).
> >
> > This whole thing feels a bit of a botch, though.
> >
> > Do we know of anyone actually using a sigaltstack with a pkey other
> > than 0?  Why the urgency?  Code relying on an asm shim on x86 is
> > already nonportable, unless I've misunderstood something, so simply
> > turning on arm64 pkeys support in the kernel and libc shouldn't break
> > anything today?  (At least, nothing that wasn't asking to be broken.)
>
> As far as I know, Chrome plans on using a sigaltstack with a non-zero
> pkey as part of the V8 CFI and W^X work [0][1][2]. IIUC that was is part
> of the motivation for the x86 change. I don't know if it's urgent
> though.
>
> I added Stephen on CC who might be able to comment on the current state
> of things in Chrome. I don't think the code that uses a pkey on a
> sigaltstack is upstream yet.

We don't have any code for this in Chrome, since I believe it's not
supported by the kernel yet.

> [0]: https://v8.dev/blog/control-flow-integrity#signal-frame-corruption
> [1]: https://lore.kernel.org/lkml/CAEAAPHa3g0QwU=DZ2zVCqTCSh-+n2TtVKrQ07LvpwDjQ-F09gA@mail.gmail.com/
> [2]: https://docs.google.com/document/d/1O2jwK4dxI3nRcOJuPYkonhTkNQfbmwdvxQMyXgeaRHo
>
> >
> >>
> >> > Really, we want some way for userspace to tell the kernel what
> >> > permissions to use for the alternate signal stack and signal handler
> >> > using it, and then honour that request consistently (just as we try to
> >> > do for the main stack today).
> >> >
> >> > ss_flags is mostly unused... I wonder whether we could add something in
> >> > there?  Or add a sigaltstack2()?
> >>
> >> Yes, this would be sensible as a second step (backwards-compatible
> >> extension). Exactly how that API would look like is not trivial though:
> >> is the pkey implicitly derived from the pointer provided to
> >> sigaltstack()? Is there a need to specify another pkey for code, or do
> >> we just assume that the signal handler is only using code with pkey 0?
> >> (Not a concern on x86 as MPK doesn't restrict execution.) Would be very
> >> interested to hear opinions on this.
>
> I hadn't considered setting a non-zero pkey for code, but it sounds
> appealing.
>
> The general goal, IIUC, is for signal handlers to run in an isolated
> "context" using pkeys, in order to mitigate against an attacker trying
> to corrupt the CPU state on the stack from another thread. Then use this
> as a way to bypass any CFI mitigation, by setting an arbitrary PC and
> registers.

Right. We're mainly looking for a solution to protect the signal frame
against memory corruption.
I'm aware of two proposals on how to achieve this:
1) is using a pkey-protected sigaltstack, which requires a patchset
like [0] to allow xsave to write to the stack
2) is to store part of the sigframe on the shadow stack as Rick
Edgecombe proposed in [1]

[0] https://lore.kernel.org/lkml/20240802061318.2140081-1-aruna.ramakrishna@oracle.com/#t
[1] https://lore.kernel.org/lkml/2fb80876e286b4db8f9ef36bcce04bbf02af0de2.camel@intel.com/

> So sigaltstack+pkey helps with isolating the stack. Then it's up to the
> programmer to carefully write the signal handler code so it only uses
> pkey-tagged data that other threads cannot corrupt in order to trick the
> signal handler into writing to its own stack.
>
> In this context, using a non-default pkey for code might be useful, in
> order to differentiate between "valid" signal handlers and other
> functions. It could help fend against an attacker being able to use
> sigaction as a whole-function gadget to install any function as a signal
> hander. Basically mitigating going from a limited CFI bypass to an
> arbitrary CFI bypass.
>
> That being said, regarding the kernel API, it might be possible to do
> the above with this patch. We'd be using the proposed "assembly
> prologues" that sets POR_EL0 as the first thing then continues to the
> real signal handler. But if we can avoid those and directly ask the
> kernel what POR_EL0 should be set to, it'd be simpler (and maybe safer).
>
> >>
> >> Kevin
> >
> > I would vote for specifying the pkey (or, if feasible, PKRU or
> > modifications to it) in some bits of ss_flags, or in an additional
> > flags argument to sigaltstack2().
> >
> > Memory with a non-zero pkey cannot be used 100% portably, period, and
> > having non-RW(X) permissions on pkey 0 at any time is also not
> > portable, period.  So I'm not sure that having libc magically guess
> > what userspace's pkeys policy is supposed to be based on racily digging
> > metadata out of /proc/self/maps or a cache of it etc. would be such a
> > good idea.
> >
> > There are other ways to approach (or not approach) this though --
> > I would be interested to hear what other people think too...
>
> Thinking about this, I'm not sure about tying this API to sigaltstack,
> as this is about configuring the POR_EL0 register, which may control
> more than the stack.
>
> I actually have a concrete example of this in V8. There's a
> SetDefaultPermissionsForSignalHandler [3] function that needs to be
> called first thing on signal handlers to configure access to an
> allocated non-zero key.  This is independent from having a pkey-tagged
> sigaltstack or not, but I suppose later on it will need to be replaced
> with assembly when the stack is no-longer accessible.
>
> [3]: https://source.chromium.org/chromium/chromium/src/+/main:v8/include/v8-platform.h;l=665;drc=0abf23ec2a1bb475b1555790fdc72ef630a43c2a;bpv=1;bpt=1
>
> Doing this via sigaction as Catalin suggested makes sense to me, but I'm
> unsure how we express how POR_EL0 needs to be set solely using SA_*
> flags. Are we able to add a new architecture-specific payload to
> sigaction, or would that resort in a new syscall like sigaction2?
>
> As an alternative, I was wondering if this would warrant a new syscall
> like sigaltstack, but for CPU state.
>
> Thanks,
> Pierre
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5296 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20241022/b674d85d/attachment.p7s>