[PATCH v2 0/3] efi: Enable BTI for EFI runtimes services

[PATCH v2 0/3] efi: Enable BTI for EFI runtimes services

[Ard Biesheuvel]

The EFI spec v2.10 introduces a global flag in the memory attributes
table that indicates whether the EFI runtime code regions were emitted
with BTI landing pads, and can therefore tolerate being mapped with BTI
enforcement enabled.

Add the generic plumbing for this, and wire it up for arm64.

Changes since v1:
- enable BTI in UEFI code regions even if CONFIG_ARM64_BIT_KERNEL=n
- deal with BTI exceptions occuring in EFI code gracefully
- add equivalent handling to x86
- add Kees's R-b

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>

Ard Biesheuvel (3):
  efi: Discover BTI support in runtime services regions
  efi: arm64: Wire up BTI annotation in memory attributes table
  efi: x86: Wire up IBT annotation in memory attributes table

 arch/arm/include/asm/efi.h     |  2 +-
 arch/arm/kernel/efi.c          |  5 +++--
 arch/arm64/include/asm/efi.h   |  3 ++-
 arch/arm64/kernel/efi.c        | 17 ++++++++++++++---
 arch/arm64/kernel/traps.c      |  6 ++++++
 arch/riscv/include/asm/efi.h   |  2 +-
 arch/riscv/kernel/efi.c        |  3 ++-
 arch/x86/include/asm/efi.h     |  4 +++-
 arch/x86/include/asm/ibt.h     |  4 ++--
 arch/x86/kernel/apm_32.c       |  4 ++--
 arch/x86/kernel/cpu/common.c   |  5 +++--
 arch/x86/platform/efi/efi_64.c |  8 +++++++-
 drivers/firmware/efi/memattr.c |  7 ++++++-
 include/linux/efi.h            |  8 ++++++--
 14 files changed, 58 insertions(+), 20 deletions(-)

-- 
2.39.1

[PATCH v2 0/3] efi: Enable BTI for EFI runtimes services

[Ard Biesheuvel]

The EFI spec v2.10 introduces a global flag in the memory attributes
table that indicates whether the EFI runtime code regions were emitted
with BTI landing pads, and can therefore tolerate being mapped with BTI
enforcement enabled.

Add the generic plumbing for this, and wire it up for arm64.

Changes since v1:
- enable BTI in UEFI code regions even if CONFIG_ARM64_BIT_KERNEL=n
- deal with BTI exceptions occuring in EFI code gracefully
- add equivalent handling to x86
- add Kees's R-b

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>

Ard Biesheuvel (3):
  efi: Discover BTI support in runtime services regions
  efi: arm64: Wire up BTI annotation in memory attributes table
  efi: x86: Wire up IBT annotation in memory attributes table

 arch/arm/include/asm/efi.h     |  2 +-
 arch/arm/kernel/efi.c          |  5 +++--
 arch/arm64/include/asm/efi.h   |  3 ++-
 arch/arm64/kernel/efi.c        | 17 ++++++++++++++---
 arch/arm64/kernel/traps.c      |  6 ++++++
 arch/riscv/include/asm/efi.h   |  2 +-
 arch/riscv/kernel/efi.c        |  3 ++-
 arch/x86/include/asm/efi.h     |  4 +++-
 arch/x86/include/asm/ibt.h     |  4 ++--
 arch/x86/kernel/apm_32.c       |  4 ++--
 arch/x86/kernel/cpu/common.c   |  5 +++--
 arch/x86/platform/efi/efi_64.c |  8 +++++++-
 drivers/firmware/efi/memattr.c |  7 ++++++-
 include/linux/efi.h            |  8 ++++++--
 14 files changed, 58 insertions(+), 20 deletions(-)

-- 
2.39.1


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

[PATCH v2 1/3] efi: Discover BTI support in runtime services regions

[Ard Biesheuvel]

Add the generic plumbing to detect whether or not the runtime code
regions were constructed with BTI/IBT landing pads by the firmware,
permitting the OS to enable enforcement when mapping these regions into
the OS's address space.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
---
 arch/arm/include/asm/efi.h     | 2 +-
 arch/arm/kernel/efi.c          | 5 +++--
 arch/arm64/include/asm/efi.h   | 3 ++-
 arch/arm64/kernel/efi.c        | 3 ++-
 arch/riscv/include/asm/efi.h   | 2 +-
 arch/riscv/kernel/efi.c        | 3 ++-
 arch/x86/platform/efi/efi_64.c | 3 ++-
 drivers/firmware/efi/memattr.c | 7 ++++++-
 include/linux/efi.h            | 8 ++++++--
 9 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index b95241b1ca656f3c..78282ced50387dd3 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -20,7 +20,7 @@ void efi_init(void);
 void arm_efi_init(void);
 
 int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
-int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
+int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md, bool);
 
 #define arch_efi_call_virt_setup()	efi_virtmap_load()
 #define arch_efi_call_virt_teardown()	efi_virtmap_unload()
diff --git a/arch/arm/kernel/efi.c b/arch/arm/kernel/efi.c
index 882104f43b3b0928..e2b9d2618c6727c6 100644
--- a/arch/arm/kernel/efi.c
+++ b/arch/arm/kernel/efi.c
@@ -23,7 +23,8 @@ static int __init set_permissions(pte_t *ptep, unsigned long addr, void *data)
 }
 
 int __init efi_set_mapping_permissions(struct mm_struct *mm,
-				       efi_memory_desc_t *md)
+				       efi_memory_desc_t *md,
+				       bool ignored)
 {
 	unsigned long base, size;
 
@@ -71,7 +72,7 @@ int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
 	 * If stricter permissions were specified, apply them now.
 	 */
 	if (md->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))
-		return efi_set_mapping_permissions(mm, md);
+		return efi_set_mapping_permissions(mm, md, false);
 	return 0;
 }
 
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 31d13a6001df49c4..5d47d429672b1bfb 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -27,7 +27,8 @@ bool efi_runtime_fixup_exception(struct pt_regs *regs, const char *msg)
 #endif
 
 int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
-int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
+int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md,
+				bool has_bti);
 
 #define arch_efi_call_virt_setup()					\
 ({									\
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index fab05de2e12dd5d8..78ffd5aaddcbbaee 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -110,7 +110,8 @@ static int __init set_permissions(pte_t *ptep, unsigned long addr, void *data)
 }
 
 int __init efi_set_mapping_permissions(struct mm_struct *mm,
-				       efi_memory_desc_t *md)
+				       efi_memory_desc_t *md,
+				       bool has_bti)
 {
 	BUG_ON(md->type != EFI_RUNTIME_SERVICES_CODE &&
 	       md->type != EFI_RUNTIME_SERVICES_DATA);
diff --git a/arch/riscv/include/asm/efi.h b/arch/riscv/include/asm/efi.h
index 47d3ab0fcc36a186..29e9a0d84b16682f 100644
--- a/arch/riscv/include/asm/efi.h
+++ b/arch/riscv/include/asm/efi.h
@@ -19,7 +19,7 @@ extern void efi_init(void);
 #endif
 
 int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
-int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
+int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md, bool);
 
 #define arch_efi_call_virt_setup()      ({		\
 		sync_kernel_mappings(efi_mm.pgd);	\
diff --git a/arch/riscv/kernel/efi.c b/arch/riscv/kernel/efi.c
index 1aa540350abd31b0..aa6209a74c83ffc2 100644
--- a/arch/riscv/kernel/efi.c
+++ b/arch/riscv/kernel/efi.c
@@ -78,7 +78,8 @@ static int __init set_permissions(pte_t *ptep, unsigned long addr, void *data)
 }
 
 int __init efi_set_mapping_permissions(struct mm_struct *mm,
-				       efi_memory_desc_t *md)
+				       efi_memory_desc_t *md,
+				       bool ignored)
 {
 	BUG_ON(md->type != EFI_RUNTIME_SERVICES_CODE &&
 	       md->type != EFI_RUNTIME_SERVICES_DATA);
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index b36596bf0fc38f4f..2e6fe430cb07bbbc 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -389,7 +389,8 @@ static int __init efi_update_mappings(efi_memory_desc_t *md, unsigned long pf)
 	return err1 || err2;
 }
 
-static int __init efi_update_mem_attr(struct mm_struct *mm, efi_memory_desc_t *md)
+static int __init efi_update_mem_attr(struct mm_struct *mm, efi_memory_desc_t *md,
+				      bool has_ibt)
 {
 	unsigned long pf = 0;
 
diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
index 0a9aba5f9ceff0bf..3cbf00f04c5b60ca 100644
--- a/drivers/firmware/efi/memattr.c
+++ b/drivers/firmware/efi/memattr.c
@@ -129,6 +129,7 @@ int __init efi_memattr_apply_permissions(struct mm_struct *mm,
 					 efi_memattr_perm_setter fn)
 {
 	efi_memory_attributes_table_t *tbl;
+	bool has_bti = false;
 	int i, ret;
 
 	if (tbl_size <= sizeof(*tbl))
@@ -150,6 +151,10 @@ int __init efi_memattr_apply_permissions(struct mm_struct *mm,
 		return -ENOMEM;
 	}
 
+	if (tbl->version > 1 &&
+	    (tbl->flags & EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD))
+		has_bti = true;
+
 	if (efi_enabled(EFI_DBG))
 		pr_info("Processing EFI Memory Attributes table:\n");
 
@@ -169,7 +174,7 @@ int __init efi_memattr_apply_permissions(struct mm_struct *mm,
 				efi_md_typeattr_format(buf, sizeof(buf), &md));
 
 		if (valid) {
-			ret = fn(mm, &md);
+			ret = fn(mm, &md, has_bti);
 			if (ret)
 				pr_err("Error updating mappings, skipping subsequent md's\n");
 		}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 9d455d502ac92b65..df88786b59471714 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -584,11 +584,15 @@ typedef struct {
 
 #define EFI_INVALID_TABLE_ADDR		(~0UL)
 
+// BIT0 implies that Runtime code includes the forward control flow guard
+// instruction, such as X86 CET-IBT or ARM BTI.
+#define EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD	0x1
+
 typedef struct {
 	u32 version;
 	u32 num_entries;
 	u32 desc_size;
-	u32 reserved;
+	u32 flags;
 	efi_memory_desc_t entry[0];
 } efi_memory_attributes_table_t;
 
@@ -751,7 +755,7 @@ extern unsigned long efi_mem_attr_table;
  *                           argument in the page tables referred to by the
  *                           first argument.
  */
-typedef int (*efi_memattr_perm_setter)(struct mm_struct *, efi_memory_desc_t *);
+typedef int (*efi_memattr_perm_setter)(struct mm_struct *, efi_memory_desc_t *, bool);
 
 extern int efi_memattr_init(void);
 extern int efi_memattr_apply_permissions(struct mm_struct *mm,
-- 
2.39.1

[PATCH v2 1/3] efi: Discover BTI support in runtime services regions

[Ard Biesheuvel]

Add the generic plumbing to detect whether or not the runtime code
regions were constructed with BTI/IBT landing pads by the firmware,
permitting the OS to enable enforcement when mapping these regions into
the OS's address space.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
---
 arch/arm/include/asm/efi.h     | 2 +-
 arch/arm/kernel/efi.c          | 5 +++--
 arch/arm64/include/asm/efi.h   | 3 ++-
 arch/arm64/kernel/efi.c        | 3 ++-
 arch/riscv/include/asm/efi.h   | 2 +-
 arch/riscv/kernel/efi.c        | 3 ++-
 arch/x86/platform/efi/efi_64.c | 3 ++-
 drivers/firmware/efi/memattr.c | 7 ++++++-
 include/linux/efi.h            | 8 ++++++--
 9 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index b95241b1ca656f3c..78282ced50387dd3 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -20,7 +20,7 @@ void efi_init(void);
 void arm_efi_init(void);
 
 int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
-int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
+int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md, bool);
 
 #define arch_efi_call_virt_setup()	efi_virtmap_load()
 #define arch_efi_call_virt_teardown()	efi_virtmap_unload()
diff --git a/arch/arm/kernel/efi.c b/arch/arm/kernel/efi.c
index 882104f43b3b0928..e2b9d2618c6727c6 100644
--- a/arch/arm/kernel/efi.c
+++ b/arch/arm/kernel/efi.c
@@ -23,7 +23,8 @@ static int __init set_permissions(pte_t *ptep, unsigned long addr, void *data)
 }
 
 int __init efi_set_mapping_permissions(struct mm_struct *mm,
-				       efi_memory_desc_t *md)
+				       efi_memory_desc_t *md,
+				       bool ignored)
 {
 	unsigned long base, size;
 
@@ -71,7 +72,7 @@ int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
 	 * If stricter permissions were specified, apply them now.
 	 */
 	if (md->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))
-		return efi_set_mapping_permissions(mm, md);
+		return efi_set_mapping_permissions(mm, md, false);
 	return 0;
 }
 
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 31d13a6001df49c4..5d47d429672b1bfb 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -27,7 +27,8 @@ bool efi_runtime_fixup_exception(struct pt_regs *regs, const char *msg)
 #endif
 
 int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
-int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
+int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md,
+				bool has_bti);
 
 #define arch_efi_call_virt_setup()					\
 ({									\
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index fab05de2e12dd5d8..78ffd5aaddcbbaee 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -110,7 +110,8 @@ static int __init set_permissions(pte_t *ptep, unsigned long addr, void *data)
 }
 
 int __init efi_set_mapping_permissions(struct mm_struct *mm,
-				       efi_memory_desc_t *md)
+				       efi_memory_desc_t *md,
+				       bool has_bti)
 {
 	BUG_ON(md->type != EFI_RUNTIME_SERVICES_CODE &&
 	       md->type != EFI_RUNTIME_SERVICES_DATA);
diff --git a/arch/riscv/include/asm/efi.h b/arch/riscv/include/asm/efi.h
index 47d3ab0fcc36a186..29e9a0d84b16682f 100644
--- a/arch/riscv/include/asm/efi.h
+++ b/arch/riscv/include/asm/efi.h
@@ -19,7 +19,7 @@ extern void efi_init(void);
 #endif
 
 int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
-int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
+int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md, bool);
 
 #define arch_efi_call_virt_setup()      ({		\
 		sync_kernel_mappings(efi_mm.pgd);	\
diff --git a/arch/riscv/kernel/efi.c b/arch/riscv/kernel/efi.c
index 1aa540350abd31b0..aa6209a74c83ffc2 100644
--- a/arch/riscv/kernel/efi.c
+++ b/arch/riscv/kernel/efi.c
@@ -78,7 +78,8 @@ static int __init set_permissions(pte_t *ptep, unsigned long addr, void *data)
 }
 
 int __init efi_set_mapping_permissions(struct mm_struct *mm,
-				       efi_memory_desc_t *md)
+				       efi_memory_desc_t *md,
+				       bool ignored)
 {
 	BUG_ON(md->type != EFI_RUNTIME_SERVICES_CODE &&
 	       md->type != EFI_RUNTIME_SERVICES_DATA);
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index b36596bf0fc38f4f..2e6fe430cb07bbbc 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -389,7 +389,8 @@ static int __init efi_update_mappings(efi_memory_desc_t *md, unsigned long pf)
 	return err1 || err2;
 }
 
-static int __init efi_update_mem_attr(struct mm_struct *mm, efi_memory_desc_t *md)
+static int __init efi_update_mem_attr(struct mm_struct *mm, efi_memory_desc_t *md,
+				      bool has_ibt)
 {
 	unsigned long pf = 0;
 
diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
index 0a9aba5f9ceff0bf..3cbf00f04c5b60ca 100644
--- a/drivers/firmware/efi/memattr.c
+++ b/drivers/firmware/efi/memattr.c
@@ -129,6 +129,7 @@ int __init efi_memattr_apply_permissions(struct mm_struct *mm,
 					 efi_memattr_perm_setter fn)
 {
 	efi_memory_attributes_table_t *tbl;
+	bool has_bti = false;
 	int i, ret;
 
 	if (tbl_size <= sizeof(*tbl))
@@ -150,6 +151,10 @@ int __init efi_memattr_apply_permissions(struct mm_struct *mm,
 		return -ENOMEM;
 	}
 
+	if (tbl->version > 1 &&
+	    (tbl->flags & EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD))
+		has_bti = true;
+
 	if (efi_enabled(EFI_DBG))
 		pr_info("Processing EFI Memory Attributes table:\n");
 
@@ -169,7 +174,7 @@ int __init efi_memattr_apply_permissions(struct mm_struct *mm,
 				efi_md_typeattr_format(buf, sizeof(buf), &md));
 
 		if (valid) {
-			ret = fn(mm, &md);
+			ret = fn(mm, &md, has_bti);
 			if (ret)
 				pr_err("Error updating mappings, skipping subsequent md's\n");
 		}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 9d455d502ac92b65..df88786b59471714 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -584,11 +584,15 @@ typedef struct {
 
 #define EFI_INVALID_TABLE_ADDR		(~0UL)
 
+// BIT0 implies that Runtime code includes the forward control flow guard
+// instruction, such as X86 CET-IBT or ARM BTI.
+#define EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD	0x1
+
 typedef struct {
 	u32 version;
 	u32 num_entries;
 	u32 desc_size;
-	u32 reserved;
+	u32 flags;
 	efi_memory_desc_t entry[0];
 } efi_memory_attributes_table_t;
 
@@ -751,7 +755,7 @@ extern unsigned long efi_mem_attr_table;
  *                           argument in the page tables referred to by the
  *                           first argument.
  */
-typedef int (*efi_memattr_perm_setter)(struct mm_struct *, efi_memory_desc_t *);
+typedef int (*efi_memattr_perm_setter)(struct mm_struct *, efi_memory_desc_t *, bool);
 
 extern int efi_memattr_init(void);
 extern int efi_memattr_apply_permissions(struct mm_struct *mm,
-- 
2.39.1


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

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

[Ard Biesheuvel]

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)
+		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);
 }
 
-- 
2.39.1

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

[Ard Biesheuvel]

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)
+		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);
 }
 
-- 
2.39.1


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

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

[Will Deacon]

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?

Will

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

[Will Deacon]

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?

Will

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

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

[Ard Biesheuvel]

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?

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

[Ard Biesheuvel]

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?

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

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

[Mark Rutland]

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?

Thanks,
Mark.

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

[Mark Rutland]

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?

Thanks,
Mark.

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

Re: [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@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?

Re: [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@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?

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

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

[Ard Biesheuvel]

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?

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

[Ard Biesheuvel]

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?

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

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

[Mark Rutland]

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.

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

[Mark Rutland]

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.

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

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

[Will Deacon]

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

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

[Will Deacon]

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

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 510 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 510 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

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

[Ard Biesheuvel]

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.

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

[Ard Biesheuvel]

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.

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

[PATCH v2 3/3] efi: x86: Wire up IBT annotation in memory attributes table

[Ard Biesheuvel]

UEFI v2.10 extends the EFI memory attributes table with a flag that
indicates whether or not all RuntimeServicesCode regions were
constructed with ENDBR landing pads, permitting the OS to map these
regions with IBT restrictions enabled.

So let's take this into account on x86 as well.

Suggested-by: Peter Zijlstra <peterz@infradead.org> # ibt_save() changes
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/include/asm/efi.h     | 4 +++-
 arch/x86/include/asm/ibt.h     | 4 ++--
 arch/x86/kernel/apm_32.c       | 4 ++--
 arch/x86/kernel/cpu/common.c   | 5 +++--
 arch/x86/platform/efi/efi_64.c | 5 +++++
 5 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index cd19b9eca3f63cbd..9f8ded3de0381973 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -106,6 +106,8 @@ static inline void efi_fpu_end(void)
 
 extern asmlinkage u64 __efi_call(void *fp, ...);
 
+extern bool efi_disable_ibt_for_runtime;
+
 #define efi_call(...) ({						\
 	__efi_nargs_check(efi_call, 7, __VA_ARGS__);			\
 	__efi_call(__VA_ARGS__);					\
@@ -121,7 +123,7 @@ extern asmlinkage u64 __efi_call(void *fp, ...);
 
 #undef arch_efi_call_virt
 #define arch_efi_call_virt(p, f, args...) ({				\
-	u64 ret, ibt = ibt_save();					\
+	u64 ret, ibt = ibt_save(efi_disable_ibt_for_runtime);		\
 	ret = efi_call((void *)p->f, args);				\
 	ibt_restore(ibt);						\
 	ret;								\
diff --git a/arch/x86/include/asm/ibt.h b/arch/x86/include/asm/ibt.h
index 9b08082a5d9f564b..ab427fdab4115357 100644
--- a/arch/x86/include/asm/ibt.h
+++ b/arch/x86/include/asm/ibt.h
@@ -74,7 +74,7 @@ static inline bool is_endbr(u32 val)
 	return val == gen_endbr();
 }
 
-extern __noendbr u64 ibt_save(void);
+extern __noendbr u64 ibt_save(bool);
 extern __noendbr void ibt_restore(u64 save);
 
 #else /* __ASSEMBLY__ */
@@ -100,7 +100,7 @@ extern __noendbr void ibt_restore(u64 save);
 
 static inline bool is_endbr(u32 val) { return false; }
 
-static inline u64 ibt_save(void) { return 0; }
+static inline u64 ibt_save(bool) { return 0; }
 static inline void ibt_restore(u64 save) { }
 
 #else /* __ASSEMBLY__ */
diff --git a/arch/x86/kernel/apm_32.c b/arch/x86/kernel/apm_32.c
index 60e330cdbd175648..c6c15ce1952fb62e 100644
--- a/arch/x86/kernel/apm_32.c
+++ b/arch/x86/kernel/apm_32.c
@@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)
 
 	apm_irq_save(flags);
 	firmware_restrict_branch_speculation_start();
-	ibt = ibt_save();
+	ibt = ibt_save(true);
 	APM_DO_SAVE_SEGS;
 	apm_bios_call_asm(call->func, call->ebx, call->ecx,
 			  &call->eax, &call->ebx, &call->ecx, &call->edx,
@@ -690,7 +690,7 @@ static long __apm_bios_call_simple(void *_call)
 
 	apm_irq_save(flags);
 	firmware_restrict_branch_speculation_start();
-	ibt = ibt_save();
+	ibt = ibt_save(true);
 	APM_DO_SAVE_SEGS;
 	error = apm_bios_call_simple_asm(call->func, call->ebx, call->ecx,
 					 &call->eax);
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 9cfca3d7d0e207c5..54b246414eebb7b9 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -571,13 +571,14 @@ __setup("nopku", setup_disable_pku);
 
 #ifdef CONFIG_X86_KERNEL_IBT
 
-__noendbr u64 ibt_save(void)
+__noendbr u64 ibt_save(bool disable)
 {
 	u64 msr = 0;
 
 	if (cpu_feature_enabled(X86_FEATURE_IBT)) {
 		rdmsrl(MSR_IA32_S_CET, msr);
-		wrmsrl(MSR_IA32_S_CET, msr & ~CET_ENDBR_EN);
+		if (disable)
+			wrmsrl(MSR_IA32_S_CET, msr & ~CET_ENDBR_EN);
 	}
 
 	return msr;
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 2e6fe430cb07bbbc..232acf418cfbe625 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -389,11 +389,15 @@ static int __init efi_update_mappings(efi_memory_desc_t *md, unsigned long pf)
 	return err1 || err2;
 }
 
+bool efi_disable_ibt_for_runtime __ro_after_init = true;
+
 static int __init efi_update_mem_attr(struct mm_struct *mm, efi_memory_desc_t *md,
 				      bool has_ibt)
 {
 	unsigned long pf = 0;
 
+	efi_disable_ibt_for_runtime |= !has_ibt;
+
 	if (md->attribute & EFI_MEMORY_XP)
 		pf |= _PAGE_NX;
 
@@ -415,6 +419,7 @@ void __init efi_runtime_update_mappings(void)
 	 * exists, since it is intended to supersede EFI_PROPERTIES_TABLE.
 	 */
 	if (efi_enabled(EFI_MEM_ATTR)) {
+		efi_disable_ibt_for_runtime = false;
 		efi_memattr_apply_permissions(NULL, efi_update_mem_attr);
 		return;
 	}
-- 
2.39.1

[PATCH v2 3/3] efi: x86: Wire up IBT annotation in memory attributes table

[Ard Biesheuvel]

UEFI v2.10 extends the EFI memory attributes table with a flag that
indicates whether or not all RuntimeServicesCode regions were
constructed with ENDBR landing pads, permitting the OS to map these
regions with IBT restrictions enabled.

So let's take this into account on x86 as well.

Suggested-by: Peter Zijlstra <peterz@infradead.org> # ibt_save() changes
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/include/asm/efi.h     | 4 +++-
 arch/x86/include/asm/ibt.h     | 4 ++--
 arch/x86/kernel/apm_32.c       | 4 ++--
 arch/x86/kernel/cpu/common.c   | 5 +++--
 arch/x86/platform/efi/efi_64.c | 5 +++++
 5 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index cd19b9eca3f63cbd..9f8ded3de0381973 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -106,6 +106,8 @@ static inline void efi_fpu_end(void)
 
 extern asmlinkage u64 __efi_call(void *fp, ...);
 
+extern bool efi_disable_ibt_for_runtime;
+
 #define efi_call(...) ({						\
 	__efi_nargs_check(efi_call, 7, __VA_ARGS__);			\
 	__efi_call(__VA_ARGS__);					\
@@ -121,7 +123,7 @@ extern asmlinkage u64 __efi_call(void *fp, ...);
 
 #undef arch_efi_call_virt
 #define arch_efi_call_virt(p, f, args...) ({				\
-	u64 ret, ibt = ibt_save();					\
+	u64 ret, ibt = ibt_save(efi_disable_ibt_for_runtime);		\
 	ret = efi_call((void *)p->f, args);				\
 	ibt_restore(ibt);						\
 	ret;								\
diff --git a/arch/x86/include/asm/ibt.h b/arch/x86/include/asm/ibt.h
index 9b08082a5d9f564b..ab427fdab4115357 100644
--- a/arch/x86/include/asm/ibt.h
+++ b/arch/x86/include/asm/ibt.h
@@ -74,7 +74,7 @@ static inline bool is_endbr(u32 val)
 	return val == gen_endbr();
 }
 
-extern __noendbr u64 ibt_save(void);
+extern __noendbr u64 ibt_save(bool);
 extern __noendbr void ibt_restore(u64 save);
 
 #else /* __ASSEMBLY__ */
@@ -100,7 +100,7 @@ extern __noendbr void ibt_restore(u64 save);
 
 static inline bool is_endbr(u32 val) { return false; }
 
-static inline u64 ibt_save(void) { return 0; }
+static inline u64 ibt_save(bool) { return 0; }
 static inline void ibt_restore(u64 save) { }
 
 #else /* __ASSEMBLY__ */
diff --git a/arch/x86/kernel/apm_32.c b/arch/x86/kernel/apm_32.c
index 60e330cdbd175648..c6c15ce1952fb62e 100644
--- a/arch/x86/kernel/apm_32.c
+++ b/arch/x86/kernel/apm_32.c
@@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)
 
 	apm_irq_save(flags);
 	firmware_restrict_branch_speculation_start();
-	ibt = ibt_save();
+	ibt = ibt_save(true);
 	APM_DO_SAVE_SEGS;
 	apm_bios_call_asm(call->func, call->ebx, call->ecx,
 			  &call->eax, &call->ebx, &call->ecx, &call->edx,
@@ -690,7 +690,7 @@ static long __apm_bios_call_simple(void *_call)
 
 	apm_irq_save(flags);
 	firmware_restrict_branch_speculation_start();
-	ibt = ibt_save();
+	ibt = ibt_save(true);
 	APM_DO_SAVE_SEGS;
 	error = apm_bios_call_simple_asm(call->func, call->ebx, call->ecx,
 					 &call->eax);
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 9cfca3d7d0e207c5..54b246414eebb7b9 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -571,13 +571,14 @@ __setup("nopku", setup_disable_pku);
 
 #ifdef CONFIG_X86_KERNEL_IBT
 
-__noendbr u64 ibt_save(void)
+__noendbr u64 ibt_save(bool disable)
 {
 	u64 msr = 0;
 
 	if (cpu_feature_enabled(X86_FEATURE_IBT)) {
 		rdmsrl(MSR_IA32_S_CET, msr);
-		wrmsrl(MSR_IA32_S_CET, msr & ~CET_ENDBR_EN);
+		if (disable)
+			wrmsrl(MSR_IA32_S_CET, msr & ~CET_ENDBR_EN);
 	}
 
 	return msr;
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 2e6fe430cb07bbbc..232acf418cfbe625 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -389,11 +389,15 @@ static int __init efi_update_mappings(efi_memory_desc_t *md, unsigned long pf)
 	return err1 || err2;
 }
 
+bool efi_disable_ibt_for_runtime __ro_after_init = true;
+
 static int __init efi_update_mem_attr(struct mm_struct *mm, efi_memory_desc_t *md,
 				      bool has_ibt)
 {
 	unsigned long pf = 0;
 
+	efi_disable_ibt_for_runtime |= !has_ibt;
+
 	if (md->attribute & EFI_MEMORY_XP)
 		pf |= _PAGE_NX;
 
@@ -415,6 +419,7 @@ void __init efi_runtime_update_mappings(void)
 	 * exists, since it is intended to supersede EFI_PROPERTIES_TABLE.
 	 */
 	if (efi_enabled(EFI_MEM_ATTR)) {
+		efi_disable_ibt_for_runtime = false;
 		efi_memattr_apply_permissions(NULL, efi_update_mem_attr);
 		return;
 	}
-- 
2.39.1


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

Re: [PATCH v2 3/3] efi: x86: Wire up IBT annotation in memory attributes table

[Dave Hansen]

On 2/6/23 04:49, Ard Biesheuvel wrote:
> --- a/arch/x86/kernel/apm_32.c
> +++ b/arch/x86/kernel/apm_32.c
> @@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)
>  
>  	apm_irq_save(flags);
>  	firmware_restrict_branch_speculation_start();
> -	ibt = ibt_save();
> +	ibt = ibt_save(true);

My only nit with these is the bare use of 'true'/'false'.  It's
impossible to tell at the call-site what the 'true' means.  So, if you
happen to respin these and see a nice way to remedy this I'd appreciate it.

But they're still OK as-is.  For the x86 bits:

Acked-by: Dave Hansen <dave.hansen@linux.intel.com>

Re: [PATCH v2 3/3] efi: x86: Wire up IBT annotation in memory attributes table

[Dave Hansen]

On 2/6/23 04:49, Ard Biesheuvel wrote:
> --- a/arch/x86/kernel/apm_32.c
> +++ b/arch/x86/kernel/apm_32.c
> @@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)
>  
>  	apm_irq_save(flags);
>  	firmware_restrict_branch_speculation_start();
> -	ibt = ibt_save();
> +	ibt = ibt_save(true);

My only nit with these is the bare use of 'true'/'false'.  It's
impossible to tell at the call-site what the 'true' means.  So, if you
happen to respin these and see a nice way to remedy this I'd appreciate it.

But they're still OK as-is.  For the x86 bits:

Acked-by: Dave Hansen <dave.hansen@linux.intel.com>

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

Re: [PATCH v2 3/3] efi: x86: Wire up IBT annotation in memory attributes table

[Peter Zijlstra]

On Wed, Feb 08, 2023 at 07:17:15AM -0800, Dave Hansen wrote:
> On 2/6/23 04:49, Ard Biesheuvel wrote:
> > --- a/arch/x86/kernel/apm_32.c
> > +++ b/arch/x86/kernel/apm_32.c
> > @@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)
> >  
> >  	apm_irq_save(flags);
> >  	firmware_restrict_branch_speculation_start();
> > -	ibt = ibt_save();
> > +	ibt = ibt_save(true);
> 
> My only nit with these is the bare use of 'true'/'false'.  It's
> impossible to tell at the call-site what the 'true' means.  So, if you
> happen to respin these and see a nice way to remedy this I'd appreciate it.

I've often wished for a named argument extention to C, much like named
initializers, such that one can write:

	ibt_save(.disable = true);

Because the thing you mention is very common with boolean arguments, the
what gets lost in the argument name and true/false just isn't very
telling.

But yeah, even if by some miracle all compiler guys were like, YES! and
implemented it tomorrow, we couldn't use it for a good few years anyway
:-/

Re: [PATCH v2 3/3] efi: x86: Wire up IBT annotation in memory attributes table

[Peter Zijlstra]

On Wed, Feb 08, 2023 at 07:17:15AM -0800, Dave Hansen wrote:
> On 2/6/23 04:49, Ard Biesheuvel wrote:
> > --- a/arch/x86/kernel/apm_32.c
> > +++ b/arch/x86/kernel/apm_32.c
> > @@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)
> >  
> >  	apm_irq_save(flags);
> >  	firmware_restrict_branch_speculation_start();
> > -	ibt = ibt_save();
> > +	ibt = ibt_save(true);
> 
> My only nit with these is the bare use of 'true'/'false'.  It's
> impossible to tell at the call-site what the 'true' means.  So, if you
> happen to respin these and see a nice way to remedy this I'd appreciate it.

I've often wished for a named argument extention to C, much like named
initializers, such that one can write:

	ibt_save(.disable = true);

Because the thing you mention is very common with boolean arguments, the
what gets lost in the argument name and true/false just isn't very
telling.

But yeah, even if by some miracle all compiler guys were like, YES! and
implemented it tomorrow, we couldn't use it for a good few years anyway
:-/

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

Re: [PATCH v2 3/3] efi: x86: Wire up IBT annotation in memory attributes table

[Mark Rutland]

On Wed, Feb 08, 2023 at 09:14:53PM +0100, Peter Zijlstra wrote:
> On Wed, Feb 08, 2023 at 07:17:15AM -0800, Dave Hansen wrote:
> > On 2/6/23 04:49, Ard Biesheuvel wrote:
> > > --- a/arch/x86/kernel/apm_32.c
> > > +++ b/arch/x86/kernel/apm_32.c
> > > @@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)
> > >  
> > >  	apm_irq_save(flags);
> > >  	firmware_restrict_branch_speculation_start();
> > > -	ibt = ibt_save();
> > > +	ibt = ibt_save(true);
> > 
> > My only nit with these is the bare use of 'true'/'false'.  It's
> > impossible to tell at the call-site what the 'true' means.  So, if you
> > happen to respin these and see a nice way to remedy this I'd appreciate it.
> 
> I've often wished for a named argument extention to C, much like named
> initializers, such that one can write:
> 
> 	ibt_save(.disable = true);
> 
> Because the thing you mention is very common with boolean arguments, the
> what gets lost in the argument name and true/false just isn't very
> telling.
> 
> But yeah, even if by some miracle all compiler guys were like, YES! and
> implemented it tomorrow, we couldn't use it for a good few years anyway
> :-/

Well... ;)

| [mark@lakrids:~]% cat args.c        
| #include <stdbool.h>
| #include <stdio.h>
| 
| struct foo_args {
| 	bool enable;
| 	unsigned long other;
| };
| 
| void __foo(struct foo_args args)
| {
| 	printf("foo:\n"
| 	       "  enable: %s\n"
| 	       "  other: 0x%lx\n",
| 	       args.enable ? "YES" : "NO",
| 	       args.other);
| }
| 
| #define foo(args...) \
| 	__foo((struct foo_args) { args })
| 
| 
| int main(int argc, char *argv[])
| {
| 	foo(true);
| 	foo(.enable = true);
| 	foo(false, .other=0xdead);
| }
| [mark@lakrids:~]% gcc args.c -o args
| [mark@lakrids:~]% ./args            
| foo:
|   enable: YES
|   other: 0x0
| foo:
|   enable: YES
|   other: 0x0
| foo:
|   enable: NO
|   other: 0xdead

Mark.

Re: [PATCH v2 3/3] efi: x86: Wire up IBT annotation in memory attributes table

[Mark Rutland]

On Wed, Feb 08, 2023 at 09:14:53PM +0100, Peter Zijlstra wrote:
> On Wed, Feb 08, 2023 at 07:17:15AM -0800, Dave Hansen wrote:
> > On 2/6/23 04:49, Ard Biesheuvel wrote:
> > > --- a/arch/x86/kernel/apm_32.c
> > > +++ b/arch/x86/kernel/apm_32.c
> > > @@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)
> > >  
> > >  	apm_irq_save(flags);
> > >  	firmware_restrict_branch_speculation_start();
> > > -	ibt = ibt_save();
> > > +	ibt = ibt_save(true);
> > 
> > My only nit with these is the bare use of 'true'/'false'.  It's
> > impossible to tell at the call-site what the 'true' means.  So, if you
> > happen to respin these and see a nice way to remedy this I'd appreciate it.
> 
> I've often wished for a named argument extention to C, much like named
> initializers, such that one can write:
> 
> 	ibt_save(.disable = true);
> 
> Because the thing you mention is very common with boolean arguments, the
> what gets lost in the argument name and true/false just isn't very
> telling.
> 
> But yeah, even if by some miracle all compiler guys were like, YES! and
> implemented it tomorrow, we couldn't use it for a good few years anyway
> :-/

Well... ;)

| [mark@lakrids:~]% cat args.c        
| #include <stdbool.h>
| #include <stdio.h>
| 
| struct foo_args {
| 	bool enable;
| 	unsigned long other;
| };
| 
| void __foo(struct foo_args args)
| {
| 	printf("foo:\n"
| 	       "  enable: %s\n"
| 	       "  other: 0x%lx\n",
| 	       args.enable ? "YES" : "NO",
| 	       args.other);
| }
| 
| #define foo(args...) \
| 	__foo((struct foo_args) { args })
| 
| 
| int main(int argc, char *argv[])
| {
| 	foo(true);
| 	foo(.enable = true);
| 	foo(false, .other=0xdead);
| }
| [mark@lakrids:~]% gcc args.c -o args
| [mark@lakrids:~]% ./args            
| foo:
|   enable: YES
|   other: 0x0
| foo:
|   enable: YES
|   other: 0x0
| foo:
|   enable: NO
|   other: 0xdead

Mark.

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

Re: [PATCH v2 3/3] efi: x86: Wire up IBT annotation in memory attributes table

[Kees Cook]

On Wed, Feb 08, 2023 at 08:55:19PM +0000, Mark Rutland wrote:
> On Wed, Feb 08, 2023 at 09:14:53PM +0100, Peter Zijlstra wrote:
> > On Wed, Feb 08, 2023 at 07:17:15AM -0800, Dave Hansen wrote:
> > > On 2/6/23 04:49, Ard Biesheuvel wrote:
> > > > --- a/arch/x86/kernel/apm_32.c
> > > > +++ b/arch/x86/kernel/apm_32.c
> > > > @@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)
> > > >  
> > > >  	apm_irq_save(flags);
> > > >  	firmware_restrict_branch_speculation_start();
> > > > -	ibt = ibt_save();
> > > > +	ibt = ibt_save(true);
> > > 
> > > My only nit with these is the bare use of 'true'/'false'.  It's
> > > impossible to tell at the call-site what the 'true' means.  So, if you
> > > happen to respin these and see a nice way to remedy this I'd appreciate it.
> > 
> > I've often wished for a named argument extention to C, much like named
> > initializers, such that one can write:
> > 
> > 	ibt_save(.disable = true);
> > 
> > Because the thing you mention is very common with boolean arguments, the
> > what gets lost in the argument name and true/false just isn't very
> > telling.
> > 
> > But yeah, even if by some miracle all compiler guys were like, YES! and
> > implemented it tomorrow, we couldn't use it for a good few years anyway
> > :-/
> 
> Well... ;)
> 
> | [mark@lakrids:~]% cat args.c        
> | #include <stdbool.h>
> | #include <stdio.h>
> | 
> | struct foo_args {
> | 	bool enable;
> | 	unsigned long other;
> | };
> | 
> | void __foo(struct foo_args args)
> | {
> | 	printf("foo:\n"
> | 	       "  enable: %s\n"
> | 	       "  other: 0x%lx\n",
> | 	       args.enable ? "YES" : "NO",
> | 	       args.other);
> | }
> | 
> | #define foo(args...) \
> | 	__foo((struct foo_args) { args })
> | 
> | 
> | int main(int argc, char *argv[])
> | {
> | 	foo(true);
> | 	foo(.enable = true);
> | 	foo(false, .other=0xdead);
> | }
> | [mark@lakrids:~]% gcc args.c -o args
> | [mark@lakrids:~]% ./args            
> | foo:
> |   enable: YES
> |   other: 0x0
> | foo:
> |   enable: YES
> |   other: 0x0
> | foo:
> |   enable: NO
> |   other: 0xdead

I am horrified and delighted. And the resulting codegen is identical:
https://godbolt.org/z/eKTMPYc17

Without this fancy solution, what I'd seen is just using an enum:

enum do_the_thing {
	THING_DISABLE = 0,
	THING_ENABLE,
};

void foo(enum do_the_thing enable)
{
	if (enable) { ... }
}

foo(THING_ENABLE);


-- 
Kees Cook

Re: [PATCH v2 3/3] efi: x86: Wire up IBT annotation in memory attributes table

[Kees Cook]

On Wed, Feb 08, 2023 at 08:55:19PM +0000, Mark Rutland wrote:
> On Wed, Feb 08, 2023 at 09:14:53PM +0100, Peter Zijlstra wrote:
> > On Wed, Feb 08, 2023 at 07:17:15AM -0800, Dave Hansen wrote:
> > > On 2/6/23 04:49, Ard Biesheuvel wrote:
> > > > --- a/arch/x86/kernel/apm_32.c
> > > > +++ b/arch/x86/kernel/apm_32.c
> > > > @@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)
> > > >  
> > > >  	apm_irq_save(flags);
> > > >  	firmware_restrict_branch_speculation_start();
> > > > -	ibt = ibt_save();
> > > > +	ibt = ibt_save(true);
> > > 
> > > My only nit with these is the bare use of 'true'/'false'.  It's
> > > impossible to tell at the call-site what the 'true' means.  So, if you
> > > happen to respin these and see a nice way to remedy this I'd appreciate it.
> > 
> > I've often wished for a named argument extention to C, much like named
> > initializers, such that one can write:
> > 
> > 	ibt_save(.disable = true);
> > 
> > Because the thing you mention is very common with boolean arguments, the
> > what gets lost in the argument name and true/false just isn't very
> > telling.
> > 
> > But yeah, even if by some miracle all compiler guys were like, YES! and
> > implemented it tomorrow, we couldn't use it for a good few years anyway
> > :-/
> 
> Well... ;)
> 
> | [mark@lakrids:~]% cat args.c        
> | #include <stdbool.h>
> | #include <stdio.h>
> | 
> | struct foo_args {
> | 	bool enable;
> | 	unsigned long other;
> | };
> | 
> | void __foo(struct foo_args args)
> | {
> | 	printf("foo:\n"
> | 	       "  enable: %s\n"
> | 	       "  other: 0x%lx\n",
> | 	       args.enable ? "YES" : "NO",
> | 	       args.other);
> | }
> | 
> | #define foo(args...) \
> | 	__foo((struct foo_args) { args })
> | 
> | 
> | int main(int argc, char *argv[])
> | {
> | 	foo(true);
> | 	foo(.enable = true);
> | 	foo(false, .other=0xdead);
> | }
> | [mark@lakrids:~]% gcc args.c -o args
> | [mark@lakrids:~]% ./args            
> | foo:
> |   enable: YES
> |   other: 0x0
> | foo:
> |   enable: YES
> |   other: 0x0
> | foo:
> |   enable: NO
> |   other: 0xdead

I am horrified and delighted. And the resulting codegen is identical:
https://godbolt.org/z/eKTMPYc17

Without this fancy solution, what I'd seen is just using an enum:

enum do_the_thing {
	THING_DISABLE = 0,
	THING_ENABLE,
};

void foo(enum do_the_thing enable)
{
	if (enable) { ... }
}

foo(THING_ENABLE);


-- 
Kees Cook

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

Re: [PATCH v2 3/3] efi: x86: Wire up IBT annotation in memory attributes table

[Ard Biesheuvel]

On Thu, 9 Feb 2023 at 17:13, Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Feb 08, 2023 at 08:55:19PM +0000, Mark Rutland wrote:
> > On Wed, Feb 08, 2023 at 09:14:53PM +0100, Peter Zijlstra wrote:
> > > On Wed, Feb 08, 2023 at 07:17:15AM -0800, Dave Hansen wrote:
> > > > On 2/6/23 04:49, Ard Biesheuvel wrote:
> > > > > --- a/arch/x86/kernel/apm_32.c
> > > > > +++ b/arch/x86/kernel/apm_32.c
> > > > > @@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)
> > > > >
> > > > >         apm_irq_save(flags);
> > > > >         firmware_restrict_branch_speculation_start();
> > > > > -       ibt = ibt_save();
> > > > > +       ibt = ibt_save(true);
> > > >
> > > > My only nit with these is the bare use of 'true'/'false'.  It's
> > > > impossible to tell at the call-site what the 'true' means.  So, if you
> > > > happen to respin these and see a nice way to remedy this I'd appreciate it.
> > >
> > > I've often wished for a named argument extention to C, much like named
> > > initializers, such that one can write:
> > >
> > >     ibt_save(.disable = true);
> > >
> > > Because the thing you mention is very common with boolean arguments, the
> > > what gets lost in the argument name and true/false just isn't very
> > > telling.
> > >
> > > But yeah, even if by some miracle all compiler guys were like, YES! and
> > > implemented it tomorrow, we couldn't use it for a good few years anyway
> > > :-/
> >
> > Well... ;)
> >
> > | [mark@lakrids:~]% cat args.c
> > | #include <stdbool.h>
> > | #include <stdio.h>
> > |
> > | struct foo_args {
> > |     bool enable;
> > |     unsigned long other;
> > | };
> > |
> > | void __foo(struct foo_args args)
> > | {
> > |     printf("foo:\n"
> > |            "  enable: %s\n"
> > |            "  other: 0x%lx\n",
> > |            args.enable ? "YES" : "NO",
> > |            args.other);
> > | }
> > |
> > | #define foo(args...) \
> > |     __foo((struct foo_args) { args })
> > |
> > |
> > | int main(int argc, char *argv[])
> > | {
> > |     foo(true);
> > |     foo(.enable = true);
> > |     foo(false, .other=0xdead);
> > | }
> > | [mark@lakrids:~]% gcc args.c -o args
> > | [mark@lakrids:~]% ./args
> > | foo:
> > |   enable: YES
> > |   other: 0x0
> > | foo:
> > |   enable: YES
> > |   other: 0x0
> > | foo:
> > |   enable: NO
> > |   other: 0xdead
>
> I am horrified and delighted.

+1

> And the resulting codegen is identical:
> https://godbolt.org/z/eKTMPYc17
>
> Without this fancy solution, what I'd seen is just using an enum:
>
> enum do_the_thing {
>         THING_DISABLE = 0,
>         THING_ENABLE,
> };
>
> void foo(enum do_the_thing enable)
> {
>         if (enable) { ... }
> }
>
> foo(THING_ENABLE);
>

I have no strong preference one way or the other, but given that
apm_32.c is not the epicenter of new development, and the call from
EFI code is self-documenting already ('
ibt_save(efi_disable_ibt_for_runtime)', I'm inclined to just queue the
patch as-is, and leave it to whoever feels inclined to spend more free
time on this to come up with some nice polish to put on top.

Unless anyone minds?

Re: [PATCH v2 3/3] efi: x86: Wire up IBT annotation in memory attributes table

[Ard Biesheuvel]

On Thu, 9 Feb 2023 at 17:13, Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Feb 08, 2023 at 08:55:19PM +0000, Mark Rutland wrote:
> > On Wed, Feb 08, 2023 at 09:14:53PM +0100, Peter Zijlstra wrote:
> > > On Wed, Feb 08, 2023 at 07:17:15AM -0800, Dave Hansen wrote:
> > > > On 2/6/23 04:49, Ard Biesheuvel wrote:
> > > > > --- a/arch/x86/kernel/apm_32.c
> > > > > +++ b/arch/x86/kernel/apm_32.c
> > > > > @@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)
> > > > >
> > > > >         apm_irq_save(flags);
> > > > >         firmware_restrict_branch_speculation_start();
> > > > > -       ibt = ibt_save();
> > > > > +       ibt = ibt_save(true);
> > > >
> > > > My only nit with these is the bare use of 'true'/'false'.  It's
> > > > impossible to tell at the call-site what the 'true' means.  So, if you
> > > > happen to respin these and see a nice way to remedy this I'd appreciate it.
> > >
> > > I've often wished for a named argument extention to C, much like named
> > > initializers, such that one can write:
> > >
> > >     ibt_save(.disable = true);
> > >
> > > Because the thing you mention is very common with boolean arguments, the
> > > what gets lost in the argument name and true/false just isn't very
> > > telling.
> > >
> > > But yeah, even if by some miracle all compiler guys were like, YES! and
> > > implemented it tomorrow, we couldn't use it for a good few years anyway
> > > :-/
> >
> > Well... ;)
> >
> > | [mark@lakrids:~]% cat args.c
> > | #include <stdbool.h>
> > | #include <stdio.h>
> > |
> > | struct foo_args {
> > |     bool enable;
> > |     unsigned long other;
> > | };
> > |
> > | void __foo(struct foo_args args)
> > | {
> > |     printf("foo:\n"
> > |            "  enable: %s\n"
> > |            "  other: 0x%lx\n",
> > |            args.enable ? "YES" : "NO",
> > |            args.other);
> > | }
> > |
> > | #define foo(args...) \
> > |     __foo((struct foo_args) { args })
> > |
> > |
> > | int main(int argc, char *argv[])
> > | {
> > |     foo(true);
> > |     foo(.enable = true);
> > |     foo(false, .other=0xdead);
> > | }
> > | [mark@lakrids:~]% gcc args.c -o args
> > | [mark@lakrids:~]% ./args
> > | foo:
> > |   enable: YES
> > |   other: 0x0
> > | foo:
> > |   enable: YES
> > |   other: 0x0
> > | foo:
> > |   enable: NO
> > |   other: 0xdead
>
> I am horrified and delighted.

+1

> And the resulting codegen is identical:
> https://godbolt.org/z/eKTMPYc17
>
> Without this fancy solution, what I'd seen is just using an enum:
>
> enum do_the_thing {
>         THING_DISABLE = 0,
>         THING_ENABLE,
> };
>
> void foo(enum do_the_thing enable)
> {
>         if (enable) { ... }
> }
>
> foo(THING_ENABLE);
>

I have no strong preference one way or the other, but given that
apm_32.c is not the epicenter of new development, and the call from
EFI code is self-documenting already ('
ibt_save(efi_disable_ibt_for_runtime)', I'm inclined to just queue the
patch as-is, and leave it to whoever feels inclined to spend more free
time on this to come up with some nice polish to put on top.

Unless anyone minds?

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

Re: [PATCH v2 3/3] efi: x86: Wire up IBT annotation in memory attributes table

[Dave Hansen]

On 2/9/23 08:23, Ard Biesheuvel wrote:
> I have no strong preference one way or the other, but given that
> apm_32.c is not the epicenter of new development, and the call from
> EFI code is self-documenting already ('
> ibt_save(efi_disable_ibt_for_runtime)', I'm inclined to just queue the
> patch as-is, and leave it to whoever feels inclined to spend more free
> time on this to come up with some nice polish to put on top.
> 
> Unless anyone minds?

No objections from the x86 side.

Re: [PATCH v2 3/3] efi: x86: Wire up IBT annotation in memory attributes table

[Dave Hansen]

On 2/9/23 08:23, Ard Biesheuvel wrote:
> I have no strong preference one way or the other, but given that
> apm_32.c is not the epicenter of new development, and the call from
> EFI code is self-documenting already ('
> ibt_save(efi_disable_ibt_for_runtime)', I'm inclined to just queue the
> patch as-is, and leave it to whoever feels inclined to spend more free
> time on this to come up with some nice polish to put on top.
> 
> Unless anyone minds?

No objections from the x86 side.

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

Re: [PATCH v2 3/3] efi: x86: Wire up IBT annotation in memory attributes table

[Kees Cook]

On Thu, Feb 09, 2023 at 05:23:02PM +0100, Ard Biesheuvel wrote:
> On Thu, 9 Feb 2023 at 17:13, Kees Cook <keescook@chromium.org> wrote:
> >
> > On Wed, Feb 08, 2023 at 08:55:19PM +0000, Mark Rutland wrote:
> > > On Wed, Feb 08, 2023 at 09:14:53PM +0100, Peter Zijlstra wrote:
> > > > On Wed, Feb 08, 2023 at 07:17:15AM -0800, Dave Hansen wrote:
> > > > > On 2/6/23 04:49, Ard Biesheuvel wrote:
> > > > > > --- a/arch/x86/kernel/apm_32.c
> > > > > > +++ b/arch/x86/kernel/apm_32.c
> > > > > > @@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)
> > > > > >
> > > > > >         apm_irq_save(flags);
> > > > > >         firmware_restrict_branch_speculation_start();
> > > > > > -       ibt = ibt_save();
> > > > > > +       ibt = ibt_save(true);
> > > > >
> > > > > My only nit with these is the bare use of 'true'/'false'.  It's
> > > > > impossible to tell at the call-site what the 'true' means.  So, if you
> > > > > happen to respin these and see a nice way to remedy this I'd appreciate it.
> > > >
> > > > I've often wished for a named argument extention to C, much like named
> > > > initializers, such that one can write:
> > > >
> > > >     ibt_save(.disable = true);
> > > >
> > > > Because the thing you mention is very common with boolean arguments, the
> > > > what gets lost in the argument name and true/false just isn't very
> > > > telling.
> > > >
> > > > But yeah, even if by some miracle all compiler guys were like, YES! and
> > > > implemented it tomorrow, we couldn't use it for a good few years anyway
> > > > :-/
> > >
> > > Well... ;)
> > >
> > > | [mark@lakrids:~]% cat args.c
> > > | #include <stdbool.h>
> > > | #include <stdio.h>
> > > |
> > > | struct foo_args {
> > > |     bool enable;
> > > |     unsigned long other;
> > > | };
> > > |
> > > | void __foo(struct foo_args args)
> > > | {
> > > |     printf("foo:\n"
> > > |            "  enable: %s\n"
> > > |            "  other: 0x%lx\n",
> > > |            args.enable ? "YES" : "NO",
> > > |            args.other);
> > > | }
> > > |
> > > | #define foo(args...) \
> > > |     __foo((struct foo_args) { args })
> > > |
> > > |
> > > | int main(int argc, char *argv[])
> > > | {
> > > |     foo(true);
> > > |     foo(.enable = true);
> > > |     foo(false, .other=0xdead);
> > > | }
> > > | [mark@lakrids:~]% gcc args.c -o args
> > > | [mark@lakrids:~]% ./args
> > > | foo:
> > > |   enable: YES
> > > |   other: 0x0
> > > | foo:
> > > |   enable: YES
> > > |   other: 0x0
> > > | foo:
> > > |   enable: NO
> > > |   other: 0xdead
> >
> > I am horrified and delighted.
> 
> +1
> 
> > And the resulting codegen is identical:
> > https://godbolt.org/z/eKTMPYc17
> >
> > Without this fancy solution, what I'd seen is just using an enum:
> >
> > enum do_the_thing {
> >         THING_DISABLE = 0,
> >         THING_ENABLE,
> > };
> >
> > void foo(enum do_the_thing enable)
> > {
> >         if (enable) { ... }
> > }
> >
> > foo(THING_ENABLE);
> >
> 
> I have no strong preference one way or the other, but given that
> apm_32.c is not the epicenter of new development, and the call from
> EFI code is self-documenting already ('
> ibt_save(efi_disable_ibt_for_runtime)', I'm inclined to just queue the
> patch as-is, and leave it to whoever feels inclined to spend more free
> time on this to come up with some nice polish to put on top.
> 
> Unless anyone minds?

Yeah, this was just commentary. I think the patch is fine as-is.

-- 
Kees Cook

Re: [PATCH v2 3/3] efi: x86: Wire up IBT annotation in memory attributes table

[Kees Cook]

On Thu, Feb 09, 2023 at 05:23:02PM +0100, Ard Biesheuvel wrote:
> On Thu, 9 Feb 2023 at 17:13, Kees Cook <keescook@chromium.org> wrote:
> >
> > On Wed, Feb 08, 2023 at 08:55:19PM +0000, Mark Rutland wrote:
> > > On Wed, Feb 08, 2023 at 09:14:53PM +0100, Peter Zijlstra wrote:
> > > > On Wed, Feb 08, 2023 at 07:17:15AM -0800, Dave Hansen wrote:
> > > > > On 2/6/23 04:49, Ard Biesheuvel wrote:
> > > > > > --- a/arch/x86/kernel/apm_32.c
> > > > > > +++ b/arch/x86/kernel/apm_32.c
> > > > > > @@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)
> > > > > >
> > > > > >         apm_irq_save(flags);
> > > > > >         firmware_restrict_branch_speculation_start();
> > > > > > -       ibt = ibt_save();
> > > > > > +       ibt = ibt_save(true);
> > > > >
> > > > > My only nit with these is the bare use of 'true'/'false'.  It's
> > > > > impossible to tell at the call-site what the 'true' means.  So, if you
> > > > > happen to respin these and see a nice way to remedy this I'd appreciate it.
> > > >
> > > > I've often wished for a named argument extention to C, much like named
> > > > initializers, such that one can write:
> > > >
> > > >     ibt_save(.disable = true);
> > > >
> > > > Because the thing you mention is very common with boolean arguments, the
> > > > what gets lost in the argument name and true/false just isn't very
> > > > telling.
> > > >
> > > > But yeah, even if by some miracle all compiler guys were like, YES! and
> > > > implemented it tomorrow, we couldn't use it for a good few years anyway
> > > > :-/
> > >
> > > Well... ;)
> > >
> > > | [mark@lakrids:~]% cat args.c
> > > | #include <stdbool.h>
> > > | #include <stdio.h>
> > > |
> > > | struct foo_args {
> > > |     bool enable;
> > > |     unsigned long other;
> > > | };
> > > |
> > > | void __foo(struct foo_args args)
> > > | {
> > > |     printf("foo:\n"
> > > |            "  enable: %s\n"
> > > |            "  other: 0x%lx\n",
> > > |            args.enable ? "YES" : "NO",
> > > |            args.other);
> > > | }
> > > |
> > > | #define foo(args...) \
> > > |     __foo((struct foo_args) { args })
> > > |
> > > |
> > > | int main(int argc, char *argv[])
> > > | {
> > > |     foo(true);
> > > |     foo(.enable = true);
> > > |     foo(false, .other=0xdead);
> > > | }
> > > | [mark@lakrids:~]% gcc args.c -o args
> > > | [mark@lakrids:~]% ./args
> > > | foo:
> > > |   enable: YES
> > > |   other: 0x0
> > > | foo:
> > > |   enable: YES
> > > |   other: 0x0
> > > | foo:
> > > |   enable: NO
> > > |   other: 0xdead
> >
> > I am horrified and delighted.
> 
> +1
> 
> > And the resulting codegen is identical:
> > https://godbolt.org/z/eKTMPYc17
> >
> > Without this fancy solution, what I'd seen is just using an enum:
> >
> > enum do_the_thing {
> >         THING_DISABLE = 0,
> >         THING_ENABLE,
> > };
> >
> > void foo(enum do_the_thing enable)
> > {
> >         if (enable) { ... }
> > }
> >
> > foo(THING_ENABLE);
> >
> 
> I have no strong preference one way or the other, but given that
> apm_32.c is not the epicenter of new development, and the call from
> EFI code is self-documenting already ('
> ibt_save(efi_disable_ibt_for_runtime)', I'm inclined to just queue the
> patch as-is, and leave it to whoever feels inclined to spend more free
> time on this to come up with some nice polish to put on top.
> 
> Unless anyone minds?

Yeah, this was just commentary. I think the patch is fine as-is.

-- 
Kees Cook

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

Re: [PATCH v2 3/3] efi: x86: Wire up IBT annotation in memory attributes table

[Peter Zijlstra]

On Mon, Feb 06, 2023 at 01:49:38PM +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 ENDBR landing pads, permitting the OS to map these
> regions with IBT restrictions enabled.
> 
> So let's take this into account on x86 as well.
> 
> Suggested-by: Peter Zijlstra <peterz@infradead.org> # ibt_save() changes
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Looks about right; would be lovely if someone with a fresh enough
firmware image could actually test this though..

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Re: [PATCH v2 3/3] efi: x86: Wire up IBT annotation in memory attributes table

[Peter Zijlstra]

On Mon, Feb 06, 2023 at 01:49:38PM +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 ENDBR landing pads, permitting the OS to map these
> regions with IBT restrictions enabled.
> 
> So let's take this into account on x86 as well.
> 
> Suggested-by: Peter Zijlstra <peterz@infradead.org> # ibt_save() changes
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Looks about right; would be lovely if someone with a fresh enough
firmware image could actually test this though..

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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

Re: [PATCH v2 0/3] efi: Enable BTI for EFI runtimes services

[Ard Biesheuvel]

On Mon, 6 Feb 2023 at 13:49, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> The EFI spec v2.10 introduces a global flag in the memory attributes
> table that indicates whether the EFI runtime code regions were emitted
> with BTI landing pads, and can therefore tolerate being mapped with BTI
> enforcement enabled.
>
> Add the generic plumbing for this, and wire it up for arm64.
>
> Changes since v1:
> - enable BTI in UEFI code regions even if CONFIG_ARM64_BIT_KERNEL=n
> - deal with BTI exceptions occuring in EFI code gracefully
> - add equivalent handling to x86
> - add Kees's R-b
>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
>

x86, arm64 maintainers: I'd like to queue these up in the EFI tree.
Any objections?



> Ard Biesheuvel (3):
>   efi: Discover BTI support in runtime services regions
>   efi: arm64: Wire up BTI annotation in memory attributes table
>   efi: x86: Wire up IBT annotation in memory attributes table
>
>  arch/arm/include/asm/efi.h     |  2 +-
>  arch/arm/kernel/efi.c          |  5 +++--
>  arch/arm64/include/asm/efi.h   |  3 ++-
>  arch/arm64/kernel/efi.c        | 17 ++++++++++++++---
>  arch/arm64/kernel/traps.c      |  6 ++++++
>  arch/riscv/include/asm/efi.h   |  2 +-
>  arch/riscv/kernel/efi.c        |  3 ++-
>  arch/x86/include/asm/efi.h     |  4 +++-
>  arch/x86/include/asm/ibt.h     |  4 ++--
>  arch/x86/kernel/apm_32.c       |  4 ++--
>  arch/x86/kernel/cpu/common.c   |  5 +++--
>  arch/x86/platform/efi/efi_64.c |  8 +++++++-
>  drivers/firmware/efi/memattr.c |  7 ++++++-
>  include/linux/efi.h            |  8 ++++++--
>  14 files changed, 58 insertions(+), 20 deletions(-)
>
> --
> 2.39.1
>

Re: [PATCH v2 0/3] efi: Enable BTI for EFI runtimes services

[Ard Biesheuvel]

On Mon, 6 Feb 2023 at 13:49, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> The EFI spec v2.10 introduces a global flag in the memory attributes
> table that indicates whether the EFI runtime code regions were emitted
> with BTI landing pads, and can therefore tolerate being mapped with BTI
> enforcement enabled.
>
> Add the generic plumbing for this, and wire it up for arm64.
>
> Changes since v1:
> - enable BTI in UEFI code regions even if CONFIG_ARM64_BIT_KERNEL=n
> - deal with BTI exceptions occuring in EFI code gracefully
> - add equivalent handling to x86
> - add Kees's R-b
>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
>

x86, arm64 maintainers: I'd like to queue these up in the EFI tree.
Any objections?



> Ard Biesheuvel (3):
>   efi: Discover BTI support in runtime services regions
>   efi: arm64: Wire up BTI annotation in memory attributes table
>   efi: x86: Wire up IBT annotation in memory attributes table
>
>  arch/arm/include/asm/efi.h     |  2 +-
>  arch/arm/kernel/efi.c          |  5 +++--
>  arch/arm64/include/asm/efi.h   |  3 ++-
>  arch/arm64/kernel/efi.c        | 17 ++++++++++++++---
>  arch/arm64/kernel/traps.c      |  6 ++++++
>  arch/riscv/include/asm/efi.h   |  2 +-
>  arch/riscv/kernel/efi.c        |  3 ++-
>  arch/x86/include/asm/efi.h     |  4 +++-
>  arch/x86/include/asm/ibt.h     |  4 ++--
>  arch/x86/kernel/apm_32.c       |  4 ++--
>  arch/x86/kernel/cpu/common.c   |  5 +++--
>  arch/x86/platform/efi/efi_64.c |  8 +++++++-
>  drivers/firmware/efi/memattr.c |  7 ++++++-
>  include/linux/efi.h            |  8 ++++++--
>  14 files changed, 58 insertions(+), 20 deletions(-)
>
> --
> 2.39.1
>

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