diff mbox series

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

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

Commit Message

Ard Biesheuvel Feb. 6, 2023, 12:49 p.m. UTC
UEFI v2.10 extends the EFI memory attributes table with a flag that
indicates whether or not all RuntimeServicesCode regions were
constructed with 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(-)

Comments

Dave Hansen Feb. 8, 2023, 3:17 p.m. UTC | #1
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>
Peter Zijlstra Feb. 8, 2023, 5:30 p.m. UTC | #2
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>
Peter Zijlstra Feb. 8, 2023, 8:14 p.m. UTC | #3
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
:-/
Mark Rutland Feb. 8, 2023, 8:55 p.m. UTC | #4
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.
Kees Cook Feb. 9, 2023, 4:13 p.m. UTC | #5
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);
Ard Biesheuvel Feb. 9, 2023, 4:23 p.m. UTC | #6
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?
Dave Hansen Feb. 9, 2023, 4:27 p.m. UTC | #7
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.
Kees Cook Feb. 9, 2023, 4:37 p.m. UTC | #8
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.
diff mbox series

Patch

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