Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support
On Fri, Oct 18, 2019 at 12:16:03PM +0100, Mark Rutland wrote:
> [adding mm folk]
>
> On Fri, Oct 11, 2019 at 06:20:15PM +0100, Dave Martin wrote:
> > On Fri, Oct 11, 2019 at 04:10:29PM +0100, Mark Rutland wrote:
> > > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > > > +#define arch_validate_prot(prot, addr) arm64_validate_prot(prot, addr)
> > > > +static inline int arm64_validate_prot(unsigned long prot, unsigned
> > > > long addr)
> > > > +{
> > > > + unsigned long supported = PROT_READ | PROT_WRITE | PROT_EXEC |
> > > > PROT_SEM;
> > > > +
> > > > + if (system_supports_bti())
> > > > + supported |= PROT_BTI;
> > > > +
> > > > + return (prot & ~supported) == 0;
> > > > +}
> > >
> > > If we have this check, can we ever get into arm64_calc_vm_prot_bits()
> > > with PROT_BIT but !system_supports_bti()?
> > >
> > > ... or can that become:
> > >
> > > return (prot & PROT_BTI) ? VM_ARM64_BTI : 0;
> >
> > We can reach this via mmap() and friends IIUC.
> >
> > Since this function only gets called once-ish per vma I have a weak
> > preference for keeping the check here to avoid code fragility.
> >
> >
> > It does feel like arch_validate_prot() is supposed to be a generic gate
> > for prot flags coming into the kernel via any route though, but only the
> > mprotect() path actually uses it.
> >
> > This function originally landed in v2.6.27 as part of the powerpc strong
> > access ordering support (PROT_SAO):
> >
> > b845f313d78e ("mm: Allow architectures to define additional protection
> > bits")
> > ef3d3246a0d0 ("powerpc/mm: Add Strong Access Ordering support")
> >
> > where the mmap() path uses arch_calc_vm_prot_bits() without
> > arch_validate_prot(), just as in the current code. powerpc's original
> > arch_calc_vm_prot_bits() does no obvious policing.
> >
> > This might be a bug. I can draft a patch to add it for the mmap() path
> > for people to comment on ... I can't figure out yet whether or not the
> > difference is intentional or there's some subtlety that I'm missed.
>
> From reading those two commit messages, it looks like this was an
> oversight. I'd expect that we should apply this check for any
> user-provided prot (i.e. it should apply to both mprotect and mmap).
>
> Ben, Andrew, does that make sense to you?
>
> ... or was there some reason to only do this for mprotect?
>
> Thanks,
> Mark.
For now, I'll drop a comment under the tearoff noting this outstanding
question.
The resulting behaviour is slightly odd, but doesn't seem unsafe, and
we can of course tidy it up later. I think the risk of userspace
becoming dependent on randomly passing PROT_BTI to mprotect() even
when unsupported is low.
[...]
Cheers
---Dave
Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support
On Fri, Oct 18, 2019 at 12:10:03PM +0100, Mark Rutland wrote:
> On Fri, Oct 11, 2019 at 06:20:15PM +0100, Dave Martin wrote:
> > On Fri, Oct 11, 2019 at 04:10:29PM +0100, Mark Rutland wrote:
> > > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > > > +#define arch_calc_vm_prot_bits(prot, pkey)
> > > > arm64_calc_vm_prot_bits(prot)
> > > > +static inline unsigned long arm64_calc_vm_prot_bits(unsigned long prot)
> > > > +{
> > > > + if (system_supports_bti() && (prot & PROT_BTI))
> > > > + return VM_ARM64_BTI;
> > > > +
> > > > + return 0;
> > > > +}
> > >
> > > Can we call this arch_calc_vm_prot_bits() directly, with all the
> > > arguments:
> > >
> > > static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot,
> > > unsigned long pkey)
> > > {
> > > ...
> > > }
> > > #define arch_calc_vm_prot_bits arch_calc_vm_prot_bits
> > >
> > > ... as that makes it a bit easier to match definition with use, and just
> > > definign the name makes it a bit clearer that that's probably for the
> > > benefit of some ifdeffery.
> > >
> > > Likewise for the other functions here.
> > >
> > > > +#define arch_vm_get_page_prot(vm_flags)
> > > > arm64_vm_get_page_prot(vm_flags)
> > > > +static inline pgprot_t arm64_vm_get_page_prot(unsigned long vm_flags)
> > > > +{
> > > > + return (vm_flags & VM_ARM64_BTI) ? __pgprot(PTE_GP) :
> > > > __pgprot(0);
> > > > +}
> > > > +
> > > > +#define arch_validate_prot(prot, addr) arm64_validate_prot(prot, addr)
> > > > +static inline int arm64_validate_prot(unsigned long prot, unsigned
> > > > long addr)
> >
> > Can do, though it looks like a used sparc as a template, and that has a
> > sparc_ prefix.
> >
> > powerpc uses the generic name, as does x86 ... in its UAPI headers.
> > Odd.
> >
> > I can change the names here, though I'm not sure it adds a lot of value.
> >
> > If you feel strongly I can do it.
>
> I'd really prefer it because it minimizes surprises, and makes it much
> easier to hop around the codebase and find the thing you're looking for.
OK, I've no objection in that case. I'll make the change.
[...]
Cheers
---Dave
Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support
On Fri, Oct 18, 2019 at 12:05:52PM +0100, Mark Rutland wrote: > On Fri, Oct 11, 2019 at 05:42:00PM +0100, Dave Martin wrote: > > On Fri, Oct 11, 2019 at 05:01:13PM +0100, Dave Martin wrote: > > > On Fri, Oct 11, 2019 at 04:44:45PM +0100, Dave Martin wrote: > > > > On Fri, Oct 11, 2019 at 04:40:43PM +0100, Mark Rutland wrote: > > > > > On Fri, Oct 11, 2019 at 04:32:26PM +0100, Dave Martin wrote: [...] > > > > > > Either way, I feel we should do this: any function in a PROT_BTI > > > > > > page > > > > > > should have a suitable landing pad. There's no reason I can see why > > > > > > a protection given to any other callback function should be omitted > > > > > > for a signal handler. > > > > > > > > > > > > Note, if the signal handler isn't in a PROT_BTI page then overriding > > > > > > BTYPE here will not trigger a Branch Target exception. > > > > > > > > > > > > I'm happy to drop a brief comment into the code also, once we're > > > > > > agreed on what the code should be doing. > > > > > > > > > > So long as there's a comment as to why, I have no strong feelings > > > > > here. > > > > > :) > > > > > > > > OK, I think it's worth a brief comment in the code either way, so I'll > > > > add something. > > > > > > Hmm, come to think of it we do need special logic for a particular case > > > here: > > > > > > If we are delivering a SIGILL here and the SIGILL handler was registered > > > with SA_NODEFER then we will get into a spin, repeatedly delivering > > > the BTI-triggered SIGILL to the same (bad) entry point. > > > > > > Without SA_NODEFER, the SIGILL becomes fatal, which is the desired > > > behaviour, but we'll need to catch this recursion explicitly. > > > > > > > > > It's similar to the special force_sigsegv() case in > > > linux/kernel/signal.c... > > > > > > Thoughts? > > > > On second thought, maybe we don't need to do anything special. > > > > A SIGSEGV handler registered with (SA_NODEFER & ~SA_RESETHAND) and that > > dereferences a duff address would spin similarly. > > > > This SIGILL case doesn't really seem different. Either way it's a > > livelock of the user task that doesn't compromise the kernel. There > > are plenty of ways for such a livelock to happen. > > That sounds reasonable to me. OK, I guess we can park this discussion for now. Cheers ---Dave
Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support
[adding mm folk]
On Fri, Oct 11, 2019 at 06:20:15PM +0100, Dave Martin wrote:
> On Fri, Oct 11, 2019 at 04:10:29PM +0100, Mark Rutland wrote:
> > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > > +#define arch_validate_prot(prot, addr) arm64_validate_prot(prot, addr)
> > > +static inline int arm64_validate_prot(unsigned long prot, unsigned long
> > > addr)
> > > +{
> > > + unsigned long supported = PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM;
> > > +
> > > + if (system_supports_bti())
> > > + supported |= PROT_BTI;
> > > +
> > > + return (prot & ~supported) == 0;
> > > +}
> >
> > If we have this check, can we ever get into arm64_calc_vm_prot_bits()
> > with PROT_BIT but !system_supports_bti()?
> >
> > ... or can that become:
> >
> > return (prot & PROT_BTI) ? VM_ARM64_BTI : 0;
>
> We can reach this via mmap() and friends IIUC.
>
> Since this function only gets called once-ish per vma I have a weak
> preference for keeping the check here to avoid code fragility.
>
>
> It does feel like arch_validate_prot() is supposed to be a generic gate
> for prot flags coming into the kernel via any route though, but only the
> mprotect() path actually uses it.
>
> This function originally landed in v2.6.27 as part of the powerpc strong
> access ordering support (PROT_SAO):
>
> b845f313d78e ("mm: Allow architectures to define additional protection bits")
> ef3d3246a0d0 ("powerpc/mm: Add Strong Access Ordering support")
>
> where the mmap() path uses arch_calc_vm_prot_bits() without
> arch_validate_prot(), just as in the current code. powerpc's original
> arch_calc_vm_prot_bits() does no obvious policing.
>
> This might be a bug. I can draft a patch to add it for the mmap() path
> for people to comment on ... I can't figure out yet whether or not the
> difference is intentional or there's some subtlety that I'm missed.
>From reading those two commit messages, it looks like this was an
oversight. I'd expect that we should apply this check for any
user-provided prot (i.e. it should apply to both mprotect and mmap).
Ben, Andrew, does that make sense to you?
... or was there some reason to only do this for mprotect?
Thanks,
Mark.
> mmap( ... prot = -1 ... ) succeeds with effective rwx permissions and no
> apparent ill effects on my random x86 box, but mprotect(..., -1) fails
> with -EINVAL.
>
> This is at least strange.
>
> Theoretically, tightening this would be an ABI break, though I'd say
> this behaviour is not intentional.
>
> Thoughts?
>
> [...]
>
> Cheers
> ---Dave
Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support
On Fri, Oct 11, 2019 at 06:20:15PM +0100, Dave Martin wrote:
> On Fri, Oct 11, 2019 at 04:10:29PM +0100, Mark Rutland wrote:
> > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > > +#define arch_calc_vm_prot_bits(prot, pkey) arm64_calc_vm_prot_bits(prot)
> > > +static inline unsigned long arm64_calc_vm_prot_bits(unsigned long prot)
> > > +{
> > > + if (system_supports_bti() && (prot & PROT_BTI))
> > > + return VM_ARM64_BTI;
> > > +
> > > + return 0;
> > > +}
> >
> > Can we call this arch_calc_vm_prot_bits() directly, with all the
> > arguments:
> >
> > static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot,
> >unsigned long pkey)
> > {
> > ...
> > }
> > #define arch_calc_vm_prot_bits arch_calc_vm_prot_bits
> >
> > ... as that makes it a bit easier to match definition with use, and just
> > definign the name makes it a bit clearer that that's probably for the
> > benefit of some ifdeffery.
> >
> > Likewise for the other functions here.
> >
> > > +#define arch_vm_get_page_prot(vm_flags) arm64_vm_get_page_prot(vm_flags)
> > > +static inline pgprot_t arm64_vm_get_page_prot(unsigned long vm_flags)
> > > +{
> > > + return (vm_flags & VM_ARM64_BTI) ? __pgprot(PTE_GP) : __pgprot(0);
> > > +}
> > > +
> > > +#define arch_validate_prot(prot, addr) arm64_validate_prot(prot, addr)
> > > +static inline int arm64_validate_prot(unsigned long prot, unsigned long
> > > addr)
>
> Can do, though it looks like a used sparc as a template, and that has a
> sparc_ prefix.
>
> powerpc uses the generic name, as does x86 ... in its UAPI headers.
> Odd.
>
> I can change the names here, though I'm not sure it adds a lot of value.
>
> If you feel strongly I can do it.
I'd really prefer it because it minimizes surprises, and makes it much
easier to hop around the codebase and find the thing you're looking for.
I'll reply on the other issue in a separate reply.
Thanks,
Mark.
Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support
On Fri, Oct 11, 2019 at 05:42:00PM +0100, Dave Martin wrote:
> On Fri, Oct 11, 2019 at 05:01:13PM +0100, Dave Martin wrote:
> > On Fri, Oct 11, 2019 at 04:44:45PM +0100, Dave Martin wrote:
> > > On Fri, Oct 11, 2019 at 04:40:43PM +0100, Mark Rutland wrote:
> > > > On Fri, Oct 11, 2019 at 04:32:26PM +0100, Dave Martin wrote:
> > > > > On Fri, Oct 11, 2019 at 11:25:33AM -0400, Richard Henderson wrote:
> > > > > > On 10/11/19 11:10 AM, Mark Rutland wrote:
> > > > > > > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > > > > > >> @@ -730,6 +730,11 @@ static void setup_return
> > > > > > >> regs->regs[29] = (unsigned long)&user->next_frame->fp;
> > > > > > >> regs->pc = (unsigned long)ka->sa.sa_handler;
> > > > > > >>
> > > > > > >> +if (system_supports_bti()) {
> > > > > > >> +regs->pstate &= ~PSR_BTYPE_MASK;
> > > > > > >> +regs->pstate |= PSR_BTYPE_CALL;
> > > > > > >> +}
> > > > > > >> +
> > > > > > >
> > > > > > > I think we might need a comment as to what we're trying to ensure
> > > > > > > here.
> > > > > > >
> > > > > > > I was under the (perhaps mistaken) impression that we'd generate a
> > > > > > > pristine pstate for a signal handler, and it's not clear to me
> > > > > > > that we
> > > > > > > must ensure the first instruction is a target instruction.
> > > > > >
> > > > > > I think it makes sense to treat entry into a signal handler as a
> > > > > > call. Code
> > > > > > that has been compiled for BTI, and whose page has been marked with
> > > > > > PROT_BTI,
> > > > > > will already have the pauth/bti markup at the beginning of the
> > > > > > signal handler
> > > > > > function; we might as well verify that.
> > > > > >
> > > > > > Otherwise sigaction becomes a hole by which an attacker can force
> > > > > > execution to
> > > > > > start at any arbitrary address.
> > > > >
> > > > > Ack, that's the intended rationale -- I also outlined this in the
> > > > > commit
> > > > > message.
> > > >
> > > > Ah, sorry. I evidently did not read that thoroughly enough.
> > > >
> > > > > Does this sound reasonable?
> > > > >
> > > > >
> > > > > Either way, I feel we should do this: any function in a PROT_BTI page
> > > > > should have a suitable landing pad. There's no reason I can see why
> > > > > a protection given to any other callback function should be omitted
> > > > > for a signal handler.
> > > > >
> > > > > Note, if the signal handler isn't in a PROT_BTI page then overriding
> > > > > BTYPE here will not trigger a Branch Target exception.
> > > > >
> > > > > I'm happy to drop a brief comment into the code also, once we're
> > > > > agreed on what the code should be doing.
> > > >
> > > > So long as there's a comment as to why, I have no strong feelings here.
> > > > :)
> > >
> > > OK, I think it's worth a brief comment in the code either way, so I'll
> > > add something.
> >
> > Hmm, come to think of it we do need special logic for a particular case
> > here:
> >
> > If we are delivering a SIGILL here and the SIGILL handler was registered
> > with SA_NODEFER then we will get into a spin, repeatedly delivering
> > the BTI-triggered SIGILL to the same (bad) entry point.
> >
> > Without SA_NODEFER, the SIGILL becomes fatal, which is the desired
> > behaviour, but we'll need to catch this recursion explicitly.
> >
> >
> > It's similar to the special force_sigsegv() case in
> > linux/kernel/signal.c...
> >
> > Thoughts?
>
> On second thought, maybe we don't need to do anything special.
>
> A SIGSEGV handler registered with (SA_NODEFER & ~SA_RESETHAND) and that
> dereferences a duff address would spin similarly.
>
> This SIGILL case doesn't really seem different. Either way it's a
> livelock of the user task that doesn't compromise the kernel. There
> are plenty of ways for such a livelock to happen.
That sounds reasonable to me.
Thanks,
Mark.
Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support
On Fri, Oct 11, 2019 at 04:10:29PM +0100, Mark Rutland wrote:
> On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > This patch adds the bare minimum required to expose the ARMv8.5
> > Branch Target Identification feature to userspace.
> >
> > By itself, this does _not_ automatically enable BTI for any initial
> > executable pages mapped by execve(). This will come later, but for
> > now it should be possible to enable BTI manually on those pages by
> > using mprotect() from within the target process.
> >
> > Other arches already using the generic mman.h are already using
> > 0x10 for arch-specific prot flags, so we use that for PROT_BTI
> > here.
> >
> > For consistency, signal handler entry points in BTI guarded pages
> > are required to be annotated as such, just like any other function.
> > This blocks a relatively minor attack vector, but comforming
> > userspace will have the annotations anyway, so we may as well
> > enforce them.
> >
> > Signed-off-by: Dave Martin
> >
> > ---
> >
> > Changes since v1:
> >
> > * Configure SCTLR_EL1.BTx to disallow BR onto a PACIxSP instruction
> >(except via X16/X17):
> >
> >The AArch64 procedure call standard requires binaries marked with
> >GNU_PROPERTY_AARCH64_FEATURE_1_BTI to use X16/X17 in trampolines
> >and tail calls, so it makes no sense to be permissive.
> >
> > * Rename PROT_BTI_GUARDED to PROT_BTI.
> >
> > * Rename VM_ARM64_GP to VM_ARM64_BTI:
> >
> >Although the architectural name for the BTI page table bit is "GP",
> >BTI is nonetheless the feature it controls. So avoid introducing
> >the "GP" naming just for this -- it's just an unecessary extra
> >source of confusion.
> >
> > * Tidy up masking with ~PSR_BTYPE_MASK.
> >
> > * Drop masking out of BTYPE on SVC, with a comment outlining why.
> >
> > * Split PSR_BTYPE_SHIFT definition into this patch. It's not
> >useful yet, but it makes sense to define PSR_BTYPE_* using this
> >from the outset.
> >
> > * Migrate to ct_user_exit_irqoff in entry.S:el0_bti.
>
> [...]
>
> > diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h
> > new file mode 100644
> > index 000..cbfe3238
> > --- /dev/null
> > +++ b/arch/arm64/include/asm/mman.h
> > @@ -0,0 +1,33 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __ASM_MMAN_H__
> > +#define __ASM_MMAN_H__
> > +
> > +#include
> > +
> > +#define arch_calc_vm_prot_bits(prot, pkey) arm64_calc_vm_prot_bits(prot)
> > +static inline unsigned long arm64_calc_vm_prot_bits(unsigned long prot)
> > +{
> > + if (system_supports_bti() && (prot & PROT_BTI))
> > + return VM_ARM64_BTI;
> > +
> > + return 0;
> > +}
>
> Can we call this arch_calc_vm_prot_bits() directly, with all the
> arguments:
>
> static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot,
> unsigned long pkey)
> {
> ...
> }
> #define arch_calc_vm_prot_bits arch_calc_vm_prot_bits
>
> ... as that makes it a bit easier to match definition with use, and just
> definign the name makes it a bit clearer that that's probably for the
> benefit of some ifdeffery.
>
> Likewise for the other functions here.
>
> > +#define arch_vm_get_page_prot(vm_flags) arm64_vm_get_page_prot(vm_flags)
> > +static inline pgprot_t arm64_vm_get_page_prot(unsigned long vm_flags)
> > +{
> > + return (vm_flags & VM_ARM64_BTI) ? __pgprot(PTE_GP) : __pgprot(0);
> > +}
> > +
> > +#define arch_validate_prot(prot, addr) arm64_validate_prot(prot, addr)
> > +static inline int arm64_validate_prot(unsigned long prot, unsigned long
> > addr)
Can do, though it looks like a used sparc as a template, and that has a
sparc_ prefix.
powerpc uses the generic name, as does x86 ... in its UAPI headers.
Odd.
I can change the names here, though I'm not sure it adds a lot of value.
If you feel strongly I can do it.
> > +{
> > + unsigned long supported = PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM;
> > +
> > + if (system_supports_bti())
> > + supported |= PROT_BTI;
> > +
> > + return (prot & ~supported) == 0;
> > +}
>
> If we have this check, can we ever get into arm64_calc_vm_prot_bits()
> with PROT_BIT but !system_supports_bti()?
>
> ... or can that become:
>
> return (prot & PROT_BTI) ? VM_ARM64_BTI : 0;
We can reach this via mmap() and friends IIUC.
Since this function only gets called once-ish per vma I have a weak
preference for keeping the check here to avoid code fragility.
It does feel like arch_validate_prot() is supposed to be a generic gate
for prot flags coming into the kernel via any route though, but only the
mprotect() path actually uses it.
This function originally landed in v2.6.27 as part of the powerpc strong
access ordering support (PROT_SAO):
b845f313d78e ("mm: Allow architectures to define additional protection bits")
ef3d3246a0d0 ("powerpc/mm: Add Strong Access Ordering support")
where the mmap() path uses arch_calc_
Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support
On Fri, Oct 11, 2019 at 05:01:13PM +0100, Dave Martin wrote:
> On Fri, Oct 11, 2019 at 04:44:45PM +0100, Dave Martin wrote:
> > On Fri, Oct 11, 2019 at 04:40:43PM +0100, Mark Rutland wrote:
> > > On Fri, Oct 11, 2019 at 04:32:26PM +0100, Dave Martin wrote:
> > > > On Fri, Oct 11, 2019 at 11:25:33AM -0400, Richard Henderson wrote:
> > > > > On 10/11/19 11:10 AM, Mark Rutland wrote:
> > > > > > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > > > > >> @@ -730,6 +730,11 @@ static void setup_return
> > > > > >>regs->regs[29] = (unsigned long)&user->next_frame->fp;
> > > > > >>regs->pc = (unsigned long)ka->sa.sa_handler;
> > > > > >>
> > > > > >> + if (system_supports_bti()) {
> > > > > >> + regs->pstate &= ~PSR_BTYPE_MASK;
> > > > > >> + regs->pstate |= PSR_BTYPE_CALL;
> > > > > >> + }
> > > > > >> +
> > > > > >
> > > > > > I think we might need a comment as to what we're trying to ensure
> > > > > > here.
> > > > > >
> > > > > > I was under the (perhaps mistaken) impression that we'd generate a
> > > > > > pristine pstate for a signal handler, and it's not clear to me that
> > > > > > we
> > > > > > must ensure the first instruction is a target instruction.
> > > > >
> > > > > I think it makes sense to treat entry into a signal handler as a
> > > > > call. Code
> > > > > that has been compiled for BTI, and whose page has been marked with
> > > > > PROT_BTI,
> > > > > will already have the pauth/bti markup at the beginning of the signal
> > > > > handler
> > > > > function; we might as well verify that.
> > > > >
> > > > > Otherwise sigaction becomes a hole by which an attacker can force
> > > > > execution to
> > > > > start at any arbitrary address.
> > > >
> > > > Ack, that's the intended rationale -- I also outlined this in the commit
> > > > message.
> > >
> > > Ah, sorry. I evidently did not read that thoroughly enough.
> > >
> > > > Does this sound reasonable?
> > > >
> > > >
> > > > Either way, I feel we should do this: any function in a PROT_BTI page
> > > > should have a suitable landing pad. There's no reason I can see why
> > > > a protection given to any other callback function should be omitted
> > > > for a signal handler.
> > > >
> > > > Note, if the signal handler isn't in a PROT_BTI page then overriding
> > > > BTYPE here will not trigger a Branch Target exception.
> > > >
> > > > I'm happy to drop a brief comment into the code also, once we're
> > > > agreed on what the code should be doing.
> > >
> > > So long as there's a comment as to why, I have no strong feelings here.
> > > :)
> >
> > OK, I think it's worth a brief comment in the code either way, so I'll
> > add something.
>
> Hmm, come to think of it we do need special logic for a particular case
> here:
>
> If we are delivering a SIGILL here and the SIGILL handler was registered
> with SA_NODEFER then we will get into a spin, repeatedly delivering
> the BTI-triggered SIGILL to the same (bad) entry point.
>
> Without SA_NODEFER, the SIGILL becomes fatal, which is the desired
> behaviour, but we'll need to catch this recursion explicitly.
>
>
> It's similar to the special force_sigsegv() case in
> linux/kernel/signal.c...
>
> Thoughts?
On second thought, maybe we don't need to do anything special.
A SIGSEGV handler registered with (SA_NODEFER & ~SA_RESETHAND) and that
dereferences a duff address would spin similarly.
This SIGILL case doesn't really seem different. Either way it's a
livelock of the user task that doesn't compromise the kernel. There
are plenty of ways for such a livelock to happen.
Cheers
---Dave
Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support
On Fri, Oct 11, 2019 at 04:44:45PM +0100, Dave Martin wrote:
> On Fri, Oct 11, 2019 at 04:40:43PM +0100, Mark Rutland wrote:
> > On Fri, Oct 11, 2019 at 04:32:26PM +0100, Dave Martin wrote:
> > > On Fri, Oct 11, 2019 at 11:25:33AM -0400, Richard Henderson wrote:
> > > > On 10/11/19 11:10 AM, Mark Rutland wrote:
> > > > > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > > > >> @@ -730,6 +730,11 @@ static void setup_return
> > > > >> regs->regs[29] = (unsigned long)&user->next_frame->fp;
> > > > >> regs->pc = (unsigned long)ka->sa.sa_handler;
> > > > >>
> > > > >> +if (system_supports_bti()) {
> > > > >> +regs->pstate &= ~PSR_BTYPE_MASK;
> > > > >> +regs->pstate |= PSR_BTYPE_CALL;
> > > > >> +}
> > > > >> +
> > > > >
> > > > > I think we might need a comment as to what we're trying to ensure
> > > > > here.
> > > > >
> > > > > I was under the (perhaps mistaken) impression that we'd generate a
> > > > > pristine pstate for a signal handler, and it's not clear to me that we
> > > > > must ensure the first instruction is a target instruction.
> > > >
> > > > I think it makes sense to treat entry into a signal handler as a call.
> > > > Code
> > > > that has been compiled for BTI, and whose page has been marked with
> > > > PROT_BTI,
> > > > will already have the pauth/bti markup at the beginning of the signal
> > > > handler
> > > > function; we might as well verify that.
> > > >
> > > > Otherwise sigaction becomes a hole by which an attacker can force
> > > > execution to
> > > > start at any arbitrary address.
> > >
> > > Ack, that's the intended rationale -- I also outlined this in the commit
> > > message.
> >
> > Ah, sorry. I evidently did not read that thoroughly enough.
> >
> > > Does this sound reasonable?
> > >
> > >
> > > Either way, I feel we should do this: any function in a PROT_BTI page
> > > should have a suitable landing pad. There's no reason I can see why
> > > a protection given to any other callback function should be omitted
> > > for a signal handler.
> > >
> > > Note, if the signal handler isn't in a PROT_BTI page then overriding
> > > BTYPE here will not trigger a Branch Target exception.
> > >
> > > I'm happy to drop a brief comment into the code also, once we're
> > > agreed on what the code should be doing.
> >
> > So long as there's a comment as to why, I have no strong feelings here.
> > :)
>
> OK, I think it's worth a brief comment in the code either way, so I'll
> add something.
Hmm, come to think of it we do need special logic for a particular case
here:
If we are delivering a SIGILL here and the SIGILL handler was registered
with SA_NODEFER then we will get into a spin, repeatedly delivering
the BTI-triggered SIGILL to the same (bad) entry point.
Without SA_NODEFER, the SIGILL becomes fatal, which is the desired
behaviour, but we'll need to catch this recursion explicitly.
It's similar to the special force_sigsegv() case in
linux/kernel/signal.c...
Thoughts?
Cheers
---Dave
Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support
On Fri, Oct 11, 2019 at 04:40:43PM +0100, Mark Rutland wrote:
> On Fri, Oct 11, 2019 at 04:32:26PM +0100, Dave Martin wrote:
> > On Fri, Oct 11, 2019 at 11:25:33AM -0400, Richard Henderson wrote:
> > > On 10/11/19 11:10 AM, Mark Rutland wrote:
> > > > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > > >> @@ -730,6 +730,11 @@ static void setup_return
> > > >>regs->regs[29] = (unsigned long)&user->next_frame->fp;
> > > >>regs->pc = (unsigned long)ka->sa.sa_handler;
> > > >>
> > > >> + if (system_supports_bti()) {
> > > >> + regs->pstate &= ~PSR_BTYPE_MASK;
> > > >> + regs->pstate |= PSR_BTYPE_CALL;
> > > >> + }
> > > >> +
> > > >
> > > > I think we might need a comment as to what we're trying to ensure here.
> > > >
> > > > I was under the (perhaps mistaken) impression that we'd generate a
> > > > pristine pstate for a signal handler, and it's not clear to me that we
> > > > must ensure the first instruction is a target instruction.
> > >
> > > I think it makes sense to treat entry into a signal handler as a call.
> > > Code
> > > that has been compiled for BTI, and whose page has been marked with
> > > PROT_BTI,
> > > will already have the pauth/bti markup at the beginning of the signal
> > > handler
> > > function; we might as well verify that.
> > >
> > > Otherwise sigaction becomes a hole by which an attacker can force
> > > execution to
> > > start at any arbitrary address.
> >
> > Ack, that's the intended rationale -- I also outlined this in the commit
> > message.
>
> Ah, sorry. I evidently did not read that thoroughly enough.
>
> > Does this sound reasonable?
> >
> >
> > Either way, I feel we should do this: any function in a PROT_BTI page
> > should have a suitable landing pad. There's no reason I can see why
> > a protection given to any other callback function should be omitted
> > for a signal handler.
> >
> > Note, if the signal handler isn't in a PROT_BTI page then overriding
> > BTYPE here will not trigger a Branch Target exception.
> >
> > I'm happy to drop a brief comment into the code also, once we're
> > agreed on what the code should be doing.
>
> So long as there's a comment as to why, I have no strong feelings here.
> :)
OK, I think it's worth a brief comment in the code either way, so I'll
add something.
Cheers
---Dave
Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support
On Fri, Oct 11, 2019 at 04:32:26PM +0100, Dave Martin wrote:
> On Fri, Oct 11, 2019 at 11:25:33AM -0400, Richard Henderson wrote:
> > On 10/11/19 11:10 AM, Mark Rutland wrote:
> > > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > >> @@ -730,6 +730,11 @@ static void setup_return
> > >> regs->regs[29] = (unsigned long)&user->next_frame->fp;
> > >> regs->pc = (unsigned long)ka->sa.sa_handler;
> > >>
> > >> +if (system_supports_bti()) {
> > >> +regs->pstate &= ~PSR_BTYPE_MASK;
> > >> +regs->pstate |= PSR_BTYPE_CALL;
> > >> +}
> > >> +
> > >
> > > I think we might need a comment as to what we're trying to ensure here.
> > >
> > > I was under the (perhaps mistaken) impression that we'd generate a
> > > pristine pstate for a signal handler, and it's not clear to me that we
> > > must ensure the first instruction is a target instruction.
> >
> > I think it makes sense to treat entry into a signal handler as a call. Code
> > that has been compiled for BTI, and whose page has been marked with
> > PROT_BTI,
> > will already have the pauth/bti markup at the beginning of the signal
> > handler
> > function; we might as well verify that.
> >
> > Otherwise sigaction becomes a hole by which an attacker can force execution
> > to
> > start at any arbitrary address.
>
> Ack, that's the intended rationale -- I also outlined this in the commit
> message.
Ah, sorry. I evidently did not read that thoroughly enough.
> Does this sound reasonable?
>
>
> Either way, I feel we should do this: any function in a PROT_BTI page
> should have a suitable landing pad. There's no reason I can see why
> a protection given to any other callback function should be omitted
> for a signal handler.
>
> Note, if the signal handler isn't in a PROT_BTI page then overriding
> BTYPE here will not trigger a Branch Target exception.
>
> I'm happy to drop a brief comment into the code also, once we're
> agreed on what the code should be doing.
So long as there's a comment as to why, I have no strong feelings here.
:)
Thanks,
Mark.
Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support
On Fri, Oct 11, 2019 at 11:25:33AM -0400, Richard Henderson wrote:
> On 10/11/19 11:10 AM, Mark Rutland wrote:
> > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> >> @@ -730,6 +730,11 @@ static void setup_return
> >>regs->regs[29] = (unsigned long)&user->next_frame->fp;
> >>regs->pc = (unsigned long)ka->sa.sa_handler;
> >>
> >> + if (system_supports_bti()) {
> >> + regs->pstate &= ~PSR_BTYPE_MASK;
> >> + regs->pstate |= PSR_BTYPE_CALL;
> >> + }
> >> +
> >
> > I think we might need a comment as to what we're trying to ensure here.
> >
> > I was under the (perhaps mistaken) impression that we'd generate a
> > pristine pstate for a signal handler, and it's not clear to me that we
> > must ensure the first instruction is a target instruction.
>
> I think it makes sense to treat entry into a signal handler as a call. Code
> that has been compiled for BTI, and whose page has been marked with PROT_BTI,
> will already have the pauth/bti markup at the beginning of the signal handler
> function; we might as well verify that.
>
> Otherwise sigaction becomes a hole by which an attacker can force execution to
> start at any arbitrary address.
Ack, that's the intended rationale -- I also outlined this in the commit
message.
Does this sound reasonable?
Either way, I feel we should do this: any function in a PROT_BTI page
should have a suitable landing pad. There's no reason I can see why
a protection given to any other callback function should be omitted
for a signal handler.
Note, if the signal handler isn't in a PROT_BTI page then overriding
BTYPE here will not trigger a Branch Target exception.
I'm happy to drop a brief comment into the code also, once we're
agreed on what the code should be doing.
Cheers
---Dave
Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support
On 10/11/19 11:10 AM, Mark Rutland wrote:
> On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
>> @@ -730,6 +730,11 @@ static void setup_return
>> regs->regs[29] = (unsigned long)&user->next_frame->fp;
>> regs->pc = (unsigned long)ka->sa.sa_handler;
>>
>> +if (system_supports_bti()) {
>> +regs->pstate &= ~PSR_BTYPE_MASK;
>> +regs->pstate |= PSR_BTYPE_CALL;
>> +}
>> +
>
> I think we might need a comment as to what we're trying to ensure here.
>
> I was under the (perhaps mistaken) impression that we'd generate a
> pristine pstate for a signal handler, and it's not clear to me that we
> must ensure the first instruction is a target instruction.
I think it makes sense to treat entry into a signal handler as a call. Code
that has been compiled for BTI, and whose page has been marked with PROT_BTI,
will already have the pauth/bti markup at the beginning of the signal handler
function; we might as well verify that.
Otherwise sigaction becomes a hole by which an attacker can force execution to
start at any arbitrary address.
r~
Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support
On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> This patch adds the bare minimum required to expose the ARMv8.5
> Branch Target Identification feature to userspace.
>
> By itself, this does _not_ automatically enable BTI for any initial
> executable pages mapped by execve(). This will come later, but for
> now it should be possible to enable BTI manually on those pages by
> using mprotect() from within the target process.
>
> Other arches already using the generic mman.h are already using
> 0x10 for arch-specific prot flags, so we use that for PROT_BTI
> here.
>
> For consistency, signal handler entry points in BTI guarded pages
> are required to be annotated as such, just like any other function.
> This blocks a relatively minor attack vector, but comforming
> userspace will have the annotations anyway, so we may as well
> enforce them.
>
> Signed-off-by: Dave Martin
>
> ---
>
> Changes since v1:
>
> * Configure SCTLR_EL1.BTx to disallow BR onto a PACIxSP instruction
>(except via X16/X17):
>
>The AArch64 procedure call standard requires binaries marked with
>GNU_PROPERTY_AARCH64_FEATURE_1_BTI to use X16/X17 in trampolines
>and tail calls, so it makes no sense to be permissive.
>
> * Rename PROT_BTI_GUARDED to PROT_BTI.
>
> * Rename VM_ARM64_GP to VM_ARM64_BTI:
>
>Although the architectural name for the BTI page table bit is "GP",
>BTI is nonetheless the feature it controls. So avoid introducing
>the "GP" naming just for this -- it's just an unecessary extra
>source of confusion.
>
> * Tidy up masking with ~PSR_BTYPE_MASK.
>
> * Drop masking out of BTYPE on SVC, with a comment outlining why.
>
> * Split PSR_BTYPE_SHIFT definition into this patch. It's not
>useful yet, but it makes sense to define PSR_BTYPE_* using this
>from the outset.
>
> * Migrate to ct_user_exit_irqoff in entry.S:el0_bti.
[...]
> diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h
> new file mode 100644
> index 000..cbfe3238
> --- /dev/null
> +++ b/arch/arm64/include/asm/mman.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ASM_MMAN_H__
> +#define __ASM_MMAN_H__
> +
> +#include
> +
> +#define arch_calc_vm_prot_bits(prot, pkey) arm64_calc_vm_prot_bits(prot)
> +static inline unsigned long arm64_calc_vm_prot_bits(unsigned long prot)
> +{
> + if (system_supports_bti() && (prot & PROT_BTI))
> + return VM_ARM64_BTI;
> +
> + return 0;
> +}
Can we call this arch_calc_vm_prot_bits() directly, with all the
arguments:
static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot,
unsigned long pkey)
{
...
}
#define arch_calc_vm_prot_bits arch_calc_vm_prot_bits
... as that makes it a bit easier to match definition with use, and just
definign the name makes it a bit clearer that that's probably for the
benefit of some ifdeffery.
Likewise for the other functions here.
> +#define arch_vm_get_page_prot(vm_flags) arm64_vm_get_page_prot(vm_flags)
> +static inline pgprot_t arm64_vm_get_page_prot(unsigned long vm_flags)
> +{
> + return (vm_flags & VM_ARM64_BTI) ? __pgprot(PTE_GP) : __pgprot(0);
> +}
> +
> +#define arch_validate_prot(prot, addr) arm64_validate_prot(prot, addr)
> +static inline int arm64_validate_prot(unsigned long prot, unsigned long addr)
> +{
> + unsigned long supported = PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM;
> +
> + if (system_supports_bti())
> + supported |= PROT_BTI;
> +
> + return (prot & ~supported) == 0;
> +}
If we have this check, can we ever get into arm64_calc_vm_prot_bits()
with PROT_BIT but !system_supports_bti()?
... or can that become:
return (prot & PROT_BTI) ? VM_ARM64_BTI : 0;
> +#endif /* ! __ASM_MMAN_H__ */
> diff --git a/arch/arm64/include/asm/pgtable-hwdef.h
> b/arch/arm64/include/asm/pgtable-hwdef.h
> index 3df60f9..f85d1fc 100644
> --- a/arch/arm64/include/asm/pgtable-hwdef.h
> +++ b/arch/arm64/include/asm/pgtable-hwdef.h
> @@ -150,6 +150,7 @@
> #define PTE_SHARED (_AT(pteval_t, 3) << 8) /* SH[1:0],
> inner shareable */
> #define PTE_AF (_AT(pteval_t, 1) << 10)/*
> Access Flag */
> #define PTE_NG (_AT(pteval_t, 1) << 11)/* nG */
> +#define PTE_GP (_AT(pteval_t, 1) << 50)/* BTI
> guarded */
As a heads-up for anyone looking at the latest ARM ARM (ARM DDI
0487E.a), GP is missing from some of the descriptions of the table
formats in section D5.3.1 in the latest ARM ARM (ARM DDI 0487E.a), which
imply it's RES0.
It looks like that'll be fixed for the next release.
[...]
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 84a8227..6c5adea 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -737,6 +737,8 @@ el0_sync:
> b.eqel0_pc
> cmp x24, #ESR_ELx_EC_UNKNOWN
[PATCH v2 05/12] arm64: Basic Branch Target Identification support
This patch adds the bare minimum required to expose the ARMv8.5 Branch Target Identification feature to userspace. By itself, this does _not_ automatically enable BTI for any initial executable pages mapped by execve(). This will come later, but for now it should be possible to enable BTI manually on those pages by using mprotect() from within the target process. Other arches already using the generic mman.h are already using 0x10 for arch-specific prot flags, so we use that for PROT_BTI here. For consistency, signal handler entry points in BTI guarded pages are required to be annotated as such, just like any other function. This blocks a relatively minor attack vector, but comforming userspace will have the annotations anyway, so we may as well enforce them. Signed-off-by: Dave Martin --- Changes since v1: * Configure SCTLR_EL1.BTx to disallow BR onto a PACIxSP instruction (except via X16/X17): The AArch64 procedure call standard requires binaries marked with GNU_PROPERTY_AARCH64_FEATURE_1_BTI to use X16/X17 in trampolines and tail calls, so it makes no sense to be permissive. * Rename PROT_BTI_GUARDED to PROT_BTI. * Rename VM_ARM64_GP to VM_ARM64_BTI: Although the architectural name for the BTI page table bit is "GP", BTI is nonetheless the feature it controls. So avoid introducing the "GP" naming just for this -- it's just an unecessary extra source of confusion. * Tidy up masking with ~PSR_BTYPE_MASK. * Drop masking out of BTYPE on SVC, with a comment outlining why. * Split PSR_BTYPE_SHIFT definition into this patch. It's not useful yet, but it makes sense to define PSR_BTYPE_* using this from the outset. * Migrate to ct_user_exit_irqoff in entry.S:el0_bti. --- Documentation/arm64/cpu-feature-registers.rst | 2 ++ Documentation/arm64/elf_hwcaps.rst| 4 arch/arm64/Kconfig| 23 +++ arch/arm64/include/asm/cpucaps.h | 3 ++- arch/arm64/include/asm/cpufeature.h | 6 + arch/arm64/include/asm/esr.h | 2 +- arch/arm64/include/asm/hwcap.h| 1 + arch/arm64/include/asm/mman.h | 33 +++ arch/arm64/include/asm/pgtable-hwdef.h| 1 + arch/arm64/include/asm/pgtable.h | 2 +- arch/arm64/include/asm/ptrace.h | 3 +++ arch/arm64/include/asm/sysreg.h | 4 arch/arm64/include/uapi/asm/hwcap.h | 1 + arch/arm64/include/uapi/asm/mman.h| 9 arch/arm64/include/uapi/asm/ptrace.h | 1 + arch/arm64/kernel/cpufeature.c| 33 +++ arch/arm64/kernel/cpuinfo.c | 1 + arch/arm64/kernel/entry.S | 11 + arch/arm64/kernel/ptrace.c| 2 +- arch/arm64/kernel/signal.c| 5 arch/arm64/kernel/syscall.c | 18 +++ arch/arm64/kernel/traps.c | 7 ++ include/linux/mm.h| 3 +++ 23 files changed, 171 insertions(+), 4 deletions(-) create mode 100644 arch/arm64/include/asm/mman.h create mode 100644 arch/arm64/include/uapi/asm/mman.h diff --git a/Documentation/arm64/cpu-feature-registers.rst b/Documentation/arm64/cpu-feature-registers.rst index b86828f..c96c7df 100644 --- a/Documentation/arm64/cpu-feature-registers.rst +++ b/Documentation/arm64/cpu-feature-registers.rst @@ -174,6 +174,8 @@ infrastructure: +--+-+-+ | SSBS | [7-4] |y| +--+-+-+ + | BT | [3-0] |y| + +--+-+-+ 4) MIDR_EL1 - Main ID Register diff --git a/Documentation/arm64/elf_hwcaps.rst b/Documentation/arm64/elf_hwcaps.rst index 91f7952..296dcac 100644 --- a/Documentation/arm64/elf_hwcaps.rst +++ b/Documentation/arm64/elf_hwcaps.rst @@ -201,6 +201,10 @@ HWCAP2_FRINT Functionality implied by ID_AA64ISAR1_EL1.FRINTTS == 0b0001. +HWCAP2_BTI + +Functionality implied by ID_AA64PFR0_EL1.BT == 0b0001. + 4. Unused AT_HWCAP bits --- diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 41a9b42..159ee69 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -1410,6 +1410,29 @@ config ARM64_PTR_AUTH endmenu +menu "ARMv8.5 architectural features" + +config ARM64_BTI + bool "Branch Target Identification support" + default y + help + Branch Target Identification (part of the ARMv8.5 Extensions) + provides a mechanism to limit the set of locations to which computed + branch instructions such as BR or BLR can jump. + + This is intended to provide complementary protection to other control + flow integrity protection mechanisms, such as
