diff mbox series

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

Message ID 20230206124938.272988-3-ardb@kernel.org
State Accepted
Commit 1d959312e2f23c8ee6ed9432a6fa4416b267477b
Headers show
Series efi: Enable BTI for EFI runtimes services | expand

Commit Message

Ard Biesheuvel Feb. 6, 2023, 12:49 p.m. UTC
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@kernel.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
---
 arch/arm64/kernel/efi.c   | 14 ++++++++++++--
 arch/arm64/kernel/traps.c |  6 ++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

Comments

Mark Rutland Feb. 9, 2023, 3:13 p.m. UTC | #1
On Thu, Feb 09, 2023 at 03:21:55PM +0100, Ard Biesheuvel wrote:
> On Wed, 8 Feb 2023 at 15:36, Ard Biesheuvel <ardb@kernel.org> wrote:
> > On Wed, 8 Feb 2023 at 15:25, Mark Rutland <mark.rutland@arm.com> wrote:
> > > 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?
> 
> I'll merge this with the CONFIG_ARM64_BTI_KERNEL check re-added, if
> nobody minds?

That make sense to me; with the CONFIG_ARM64_BTI_KERNEL check:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.
Will Deacon Feb. 9, 2023, 3:48 p.m. UTC | #2
On Thu, Feb 09, 2023 at 03:21:55PM +0100, Ard Biesheuvel wrote:
> On Wed, 8 Feb 2023 at 15:36, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 8 Feb 2023 at 15:25, Mark Rutland <mark.rutland@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@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@kernel.org>
> > > > > > Reviewed-by: Kees Cook <keescook@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?
> 
> I'll merge this with the CONFIG_ARM64_BTI_KERNEL check re-added, if
> nobody minds?

Reviewed-by: Will Deacon <will@kernel.org>

Will
Mark Brown Feb. 20, 2023, 3:53 p.m. UTC | #3
On Wed, Feb 08, 2023 at 03:36:40PM +0100, Ard Biesheuvel wrote:

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

The concern with doing that for Linux was what would happen if someone
implemented a system with mixed BTI/no BTI support and then a task got
preempted and moved between the two, you might end up with PSTATE.BTYPE
incorrectly set and trigger a spurious fault.  That shouldn't be an
issue for EFI runtime services.
Ard Biesheuvel Feb. 20, 2023, 4:46 p.m. UTC | #4
On Mon, 20 Feb 2023 at 16:53, Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Feb 08, 2023 at 03:36:40PM +0100, Ard Biesheuvel wrote:
>
> > OTOH, what is the penalty for setting the GP attribute and using the
> > translation table on a core that does not implement BTI?
>
> The concern with doing that for Linux was what would happen if someone
> implemented a system with mixed BTI/no BTI support and then a task got
> preempted and moved between the two, you might end up with PSTATE.BTYPE
> incorrectly set and trigger a spurious fault.  That shouldn't be an
> issue for EFI runtime services.

Right, I hadn't figured that. But as you say, this shouldn't affect
EFI runtime services, as they are non-preemptible and therefore
non-migratable.
diff mbox series

Patch

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)
+		pte = set_pte_bit(pte, __pgprot(PTE_GP));
 	set_pte(ptep, pte);
 	return 0;
 }
@@ -113,6 +121,8 @@  int __init efi_set_mapping_permissions(struct mm_struct *mm,
 				       efi_memory_desc_t *md,
 				       bool has_bti)
 {
+	struct set_perm_data data = { md, has_bti };
+
 	BUG_ON(md->type != EFI_RUNTIME_SERVICES_CODE &&
 	       md->type != EFI_RUNTIME_SERVICES_DATA);
 
@@ -128,7 +138,7 @@  int __init efi_set_mapping_permissions(struct mm_struct *mm,
 	 */
 	return apply_to_page_range(mm, md->virt_addr,
 				   md->num_pages << EFI_PAGE_SHIFT,
-				   set_permissions, md);
+				   set_permissions, &data);
 }
 
 /*
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 4c0caa589e12de2a..1f366b94ea8e233a 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -18,6 +18,7 @@ 
 #include <linux/module.h>
 #include <linux/kexec.h>
 #include <linux/delay.h>
+#include <linux/efi.h>
 #include <linux/init.h>
 #include <linux/sched/signal.h>
 #include <linux/sched/debug.h>
@@ -33,6 +34,7 @@ 
 #include <asm/cpufeature.h>
 #include <asm/daifflags.h>
 #include <asm/debug-monitors.h>
+#include <asm/efi.h>
 #include <asm/esr.h>
 #include <asm/exception.h>
 #include <asm/extable.h>
@@ -492,6 +494,10 @@  void do_el0_bti(struct pt_regs *regs)
 
 void do_el1_bti(struct pt_regs *regs, unsigned long esr)
 {
+	if (efi_runtime_fixup_exception(regs, "BTI violation")) {
+		regs->pstate &= ~PSR_BTYPE_MASK;
+		return;
+	}
 	die("Oops - BTI", regs, esr);
 }