[PATCH v2 2/3] efi: arm64: Wire up BTI annotation in memory attributes table

[Ard Biesheuvel]

On Wed, 8 Feb 2023 at 15:25, Mark Rutland <mark.rutland at arm.com> wrote:
>
> On Wed, Feb 08, 2023 at 02:03:45PM +0100, Ard Biesheuvel wrote:
> > On Wed, 8 Feb 2023 at 14:00, Will Deacon <will at kernel.org> wrote:
> > >
> > > On Mon, Feb 06, 2023 at 01:49:37PM +0100, Ard Biesheuvel wrote:
> > > > UEFI v2.10 extends the EFI memory attributes table with a flag that
> > > > indicates whether or not all RuntimeServicesCode regions were
> > > > constructed with BTI landing pads, permitting the OS to map these
> > > > regions with BTI restrictions enabled.
> > > >
> > > > So let's take this into account on arm64.
> > > >
> > > > Signed-off-by: Ard Biesheuvel <ardb at kernel.org>
> > > > Reviewed-by: Kees Cook <keescook at chromium.org>
> > > > ---
> > > >  arch/arm64/kernel/efi.c   | 14 ++++++++++++--
> > > >  arch/arm64/kernel/traps.c |  6 ++++++
> > > >  2 files changed, 18 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> > > > index 78ffd5aaddcbbaee..99971cd349f36310 100644
> > > > --- a/arch/arm64/kernel/efi.c
> > > > +++ b/arch/arm64/kernel/efi.c
> > > > @@ -96,15 +96,23 @@ int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
> > > >       return 0;
> > > >  }
> > > >
> > > > +struct set_perm_data {
> > > > +     const efi_memory_desc_t *md;
> > > > +     bool                    has_bti;
> > > > +};
> > > > +
> > > >  static int __init set_permissions(pte_t *ptep, unsigned long addr, void *data)
> > > >  {
> > > > -     efi_memory_desc_t *md = data;
> > > > +     struct set_perm_data *spd = data;
> > > > +     const efi_memory_desc_t *md = spd->md;
> > > >       pte_t pte = READ_ONCE(*ptep);
> > > >
> > > >       if (md->attribute & EFI_MEMORY_RO)
> > > >               pte = set_pte_bit(pte, __pgprot(PTE_RDONLY));
> > > >       if (md->attribute & EFI_MEMORY_XP)
> > > >               pte = set_pte_bit(pte, __pgprot(PTE_PXN));
> > > > +     else if (system_supports_bti() && spd->has_bti)
> > >
> > > system_supports_bti() seems to check CONFIG_ARM64_BTI rather than
> > > CONFIG_ARM64_BTI_KERNEL. In theory, I think this means we could have
> > > mismatched BTI support, so it might be slightly more robust to use the
> > > latter option here even thought the runtime services aren't kernel code.
> > >
> > > What do you think?
> >
> > v1 checked for CONFIG_ARM64_BTI_KERNEL as well, but I dropped it
> > because we can do the enforcement even without it.
> >
> > I'm not sure how mismatched BTI support factors into that, though,
> > given that CONFIG_ARM64_BTI_KERNEL is set at compile time. You mean
> > mismatched between cores, right?
>
> I believe that there's no issue with mismatched CPUs, but there *might* might
> be a different issue with the ordering of feature detection and usage of the
> cap:
>
> * If CONFIG_ARM64_BTI_KERNEL=y, then the ARM64_BTI cap is detected as a strict
>   boot cpu feature, and secondaries without it will be rejected.
>
> * If CONFIG_ARM64_BTI_KERNEL=n then the ARM64_BTI cap is detected as a system
>   feature, and so we only set the cap bit after bringing all secondary CPUs
>   online, and only when *all* CPUs support it.
>
>   The happens under setup_cpu_features(), called from smp_cpus_done().
>
> So there's no issue with mismatch, but if system_supports_bti is called before
> smp_cpus_done() on a CONFIG_ARM64_BTI_KERNEL kernel it will return false. When
> do we set up the EFI mappings relative to that?
>

Currently it is an early initcall so before SMP, but that is not
really necessary - the EFI table that carries this annotation is an
overlay that could easily be applied later.

OTOH, what is the penalty for setting the GP attribute and using the
translation table on a core that does not implement BTI?