efi: Enable BTI for EFI runtimes services

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 <[email protected]>

Reviewed-by Kees Cook <[email protected]>

---
 arch/arm64/kernel/efi.c   | 14 ++++++++++++--
 arch/arm64/kernel/traps.c |  6 ++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

arch/arm64/kernel/efi.c +5 -0

index 78ffd5aaddcbbaee..99971cd349f36310 100644
@@ -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)
 {

Quoted 29 lines · 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 <[email protected]>

Reviewed-by Kees Cook <[email protected]>

---
 arch/arm64/kernel/efi.c   | 14 ++++++++++++--
 arch/arm64/kernel/traps.c |  6 ++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

arch/arm64/kernel/efi.c +5 -0

index 78ffd5aaddcbbaee..99971cd349f36310 100644
@@ -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)
 {

arch/arm64/kernel/efi.c int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md) · line 114

Quoted 10 lines

arch/arm64/kernel/efi.c +3 -1

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

Quoted 47 lines · Will Deacon

Quoted 29 lines · 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 <[email protected]>

Reviewed-by Kees Cook <[email protected]>

---
 arch/arm64/kernel/efi.c   | 14 ++++++++++++--
 arch/arm64/kernel/traps.c |  6 ++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

arch/arm64/kernel/efi.c +5 -0

index 78ffd5aaddcbbaee..99971cd349f36310 100644
@@ -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)
 {

arch/arm64/kernel/efi.c int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md) · line 114

Quoted 10 lines

arch/arm64/kernel/efi.c +3 -1

-     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.

Quoted 1 line

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?

Quoted 48 lines · Ard Biesheuvel

Quoted 47 lines · Will Deacon

Quoted 29 lines · 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 <[email protected]>

Reviewed-by Kees Cook <[email protected]>

---
 arch/arm64/kernel/efi.c   | 14 ++++++++++++--
 arch/arm64/kernel/traps.c |  6 ++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

arch/arm64/kernel/efi.c +5 -0

index 78ffd5aaddcbbaee..99971cd349f36310 100644
@@ -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)
 {

arch/arm64/kernel/efi.c int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md) · line 114

Quoted 10 lines

arch/arm64/kernel/efi.c +3 -1

-     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.

Quoted 8 lines

Quoted 1 line

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?

Quoted 70 lines · Mark Rutland

Quoted 48 lines · Ard Biesheuvel

Quoted 47 lines · Will Deacon

Quoted 29 lines · 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 <[email protected]>

Reviewed-by Kees Cook <[email protected]>

---
 arch/arm64/kernel/efi.c   | 14 ++++++++++++--
 arch/arm64/kernel/traps.c |  6 ++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

arch/arm64/kernel/efi.c +5 -0

index 78ffd5aaddcbbaee..99971cd349f36310 100644
@@ -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)
 {

arch/arm64/kernel/efi.c int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md) · line 114

Quoted 10 lines

arch/arm64/kernel/efi.c +3 -1

-     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.

Quoted 8 lines

Quoted 1 line

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.

Quoted 6 lines

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?

Quoted 23 lines · Ard Biesheuvel

Quoted 11 lines · Mark Rutland

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.

Quoted 6 lines

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.

Quoted 2 lines

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?

Quoted 83 lines · Ard Biesheuvel

Quoted 70 lines · Mark Rutland

Quoted 48 lines · Ard Biesheuvel

Quoted 47 lines · Will Deacon

Quoted 29 lines · 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 <[email protected]>

Reviewed-by Kees Cook <[email protected]>

---
 arch/arm64/kernel/efi.c   | 14 ++++++++++++--
 arch/arm64/kernel/traps.c |  6 ++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

arch/arm64/kernel/efi.c +5 -0

index 78ffd5aaddcbbaee..99971cd349f36310 100644
@@ -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)
 {

arch/arm64/kernel/efi.c int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md) · line 114

Quoted 10 lines

arch/arm64/kernel/efi.c +3 -1

-     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.

Quoted 8 lines

Quoted 1 line

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.

Quoted 6 lines

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.

Quoted 2 lines

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?

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

Quoted 2 lines · Ard Biesheuvel

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.

Quoted 4 lines · Ard Biesheuvel

arch/x86/kernel/apm_32.c +0 -0

@@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)
 

arch/x86/kernel/apm_32.c static long __apm_bios_call(void *_call) · line 612

Quoted 4 lines

arch/x86/kernel/apm_32.c +1 -1

 	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.

Quoted 5 lines · Dave Hansen

Quoted 4 lines · Ard Biesheuvel

arch/x86/kernel/apm_32.c +0 -0

@@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)
 

Quoted 8 lines

arch/x86/kernel/apm_32.c static long __apm_bios_call(void *_call) · line 612

Quoted 4 lines

arch/x86/kernel/apm_32.c +1 -1

 	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 :-/

Quoted 20 lines · Peter Zijlstra

Quoted 5 lines · Dave Hansen

Quoted 4 lines · Ard Biesheuvel

arch/x86/kernel/apm_32.c +0 -0

@@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)
 

Quoted 8 lines

arch/x86/kernel/apm_32.c static long __apm_bios_call(void *_call) · line 612

Quoted 4 lines

arch/x86/kernel/apm_32.c +1 -1

 	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);

Quoted 7 lines

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

Quoted 62 lines · Mark Rutland

Quoted 20 lines · Peter Zijlstra

Quoted 5 lines · Dave Hansen

Quoted 4 lines · Ard Biesheuvel

arch/x86/kernel/apm_32.c +0 -0

@@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)
 

Quoted 8 lines

arch/x86/kernel/apm_32.c static long __apm_bios_call(void *_call) · line 612

Quoted 4 lines

arch/x86/kernel/apm_32.c +1 -1

        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);

Quoted 7 lines

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:

Quoted 8 lines

|   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);

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?

Quoted 73 lines · Kees Cook

Quoted 62 lines · Mark Rutland

Quoted 20 lines · Peter Zijlstra

Quoted 5 lines · Dave Hansen

Quoted 4 lines · Ard Biesheuvel

arch/x86/kernel/apm_32.c +0 -0

@@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)
 

Quoted 8 lines

arch/x86/kernel/apm_32.c static long __apm_bios_call(void *_call) · line 612

Quoted 4 lines

arch/x86/kernel/apm_32.c +1 -1

        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);

Quoted 7 lines

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:

Quoted 8 lines

|   enable: YES
|   other: 0x0
| foo:
|   enable: YES
|   other: 0x0
| foo:
|   enable: NO
|   other: 0xdead

Quoted 1 line

I am horrified and delighted.

+1

Quoted 10 lines

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,
};

Quoted 7 lines

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?

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 <[email protected]> # ibt_save() changes

Signed-off-by Ard Biesheuvel <[email protected]>

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 <[email protected]>

Cc Will Deacon <[email protected]>

Cc Kees Cook <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dave Hansen <[email protected]>

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