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

On Wed, 8 Feb 2023 at 15:25, Mark Rutland [off-list ref] wrote:

On Wed, Feb 08, 2023 at 02:03:45PM +0100, Ard Biesheuvel wrote:

quoted

On Wed, 8 Feb 2023 at 14:00, Will Deacon [off-list ref] wrote:

quoted

On Mon, Feb 06, 2023 at 01:49:37PM +0100, Ard Biesheuvel wrote:

quoted

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 <redacted>
---
 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?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel