[PATCH 0/2] x86/cfi: Fix FineIBT

[PATCH 0/2] x86/cfi: Fix FineIBT

[Peter Zijlstra]

Hi!

Alyssa reported a FineIBT issue; these patches sort that.

Basically the morale of the story is that CALL_NOSPEC and JMP_NOSPEC are
suspect and likely not what you want. The remaining users are:

arch/x86/crypto/crc32c-pcl-intel-asm_64.S:	JMP_NOSPEC bufp

This is broken on anything IBT afaict.

arch/x86/include/asm/mshyperv.h:			     CALL_NOSPEC
arch/x86/include/asm/mshyperv.h:	__asm__ __volatile__(CALL_NOSPEC
arch/x86/include/asm/mshyperv.h:		__asm__ __volatile__(CALL_NOSPEC
arch/x86/include/asm/mshyperv.h:		__asm__ __volatile__ (CALL_NOSPEC
arch/x86/include/asm/mshyperv.h:				     CALL_NOSPEC
arch/x86/include/asm/mshyperv.h:		__asm__ __volatile__ (CALL_NOSPEC
arch/x86/include/asm/xen/hypercall.h:	asm volatile(CALL_NOSPEC

These are hypercalls and supposedly the targets are having ENDBR on.

arch/x86/kernel/ftrace_64.S:	CALL_NOSPEC r8

The thing is !DYNAMIC_FTRACE only, which we can fix with a Kconfig
dependency I suppose.

arch/x86/kvm/emulate.c:	asm("push %[flags]; popf; " CALL_NOSPEC
arch/x86/kvm/emulate.c:	asm("push %[flags]; popf; " CALL_NOSPEC " ; pushf; pop %[flags]\n"

calls into the magic fastop stuff and should be ok, those have explicit
ENDBR + IBT_NOSEAL() annotations.

arch/x86/kvm/vmx/vmenter.S:	VMX_DO_EVENT_IRQOFF CALL_NOSPEC _ASM_ARG1

calls the IDT vectors manually, so should be ok (gross, but not broken
from this pov).

arch/x86/platform/efi/efi_stub_64.S:	CALL_NOSPEC rdi

effectively a nocfi call, so should be ok.

[PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls

[Peter Zijlstra]

The ret_from_fork stub does an indirect call to the kthread function,
but only knows about Retpolines. Instead of making the asm more
complicated, punt to C and let the compiler figure it out.

Specifically, this makes it a proper kCFI indirect call when needed (in
fact, it is nearly impossible to code a kCFI indirect call in asm).

This was the only callsite that was still calling func()+0 on regular
indirect functions.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/entry/entry_64.S        |    6 ++++--
 arch/x86/include/asm/switch_to.h |    2 ++
 arch/x86/kernel/process_64.c     |    5 +++++
 3 files changed, 11 insertions(+), 2 deletions(-)

--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -304,8 +304,10 @@ SYM_CODE_START_NOALIGN(ret_from_fork)
 1:
 	/* kernel thread */
 	UNWIND_HINT_END_OF_STACK
-	movq	%r12, %rdi
-	CALL_NOSPEC rbx
+	movq	%rbx, %rdi
+	movq	%r12, %rsi
+	call	kthread_from_fork
+
 	/*
 	 * A kernel thread is allowed to return here after successfully
 	 * calling kernel_execve().  Exit to userspace to complete the execve()
--- a/arch/x86/include/asm/switch_to.h
+++ b/arch/x86/include/asm/switch_to.h
@@ -74,6 +74,8 @@ static inline void update_task_stack(str
 #endif
 }
 
+extern void kthread_from_fork(int (*fn)(void *), void *);
+
 static inline void kthread_frame_init(struct inactive_task_frame *frame,
 				      int (*fun)(void *), void *arg)
 {
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -544,6 +544,11 @@ void compat_start_thread(struct pt_regs
 }
 #endif
 
+__visible noinstr void kthread_from_fork(int (*fn)(void *), void *arg)
+{
+	fn(arg);
+}
+
 /*
  *	switch_to(x,y) should switch tasks from x to y.
  *

Re: [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls

[Kees Cook]

On Thu, Jun 15, 2023 at 09:35:47PM +0200, Peter Zijlstra wrote:
> The ret_from_fork stub does an indirect call to the kthread function,
> but only knows about Retpolines. Instead of making the asm more
> complicated, punt to C and let the compiler figure it out.
> 
> Specifically, this makes it a proper kCFI indirect call when needed (in
> fact, it is nearly impossible to code a kCFI indirect call in asm).
> 
> This was the only callsite that was still calling func()+0 on regular
> indirect functions.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

I worry this creates a calling gadget, but I don't think it really
counts since it's just converting between two prototypes. Regardless:

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls

[Peter Zijlstra]

On Tue, Jun 20, 2023 at 02:56:22PM -0700, Kees Cook wrote:
> On Thu, Jun 15, 2023 at 09:35:47PM +0200, Peter Zijlstra wrote:
> > The ret_from_fork stub does an indirect call to the kthread function,
> > but only knows about Retpolines. Instead of making the asm more
> > complicated, punt to C and let the compiler figure it out.
> > 
> > Specifically, this makes it a proper kCFI indirect call when needed (in
> > fact, it is nearly impossible to code a kCFI indirect call in asm).
> > 
> > This was the only callsite that was still calling func()+0 on regular
> > indirect functions.
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> I worry this creates a calling gadget, but I don't think it really
> counts since it's just converting between two prototypes. Regardless:

Ah, since this will never be indirectly called, I should be able to
annotate this so it never can be. Let me see what I can get the compiler
to do.

Re: [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls

[Peter Zijlstra]

On Wed, Jun 21, 2023 at 10:52:17AM +0200, Peter Zijlstra wrote:
> On Tue, Jun 20, 2023 at 02:56:22PM -0700, Kees Cook wrote:
> > On Thu, Jun 15, 2023 at 09:35:47PM +0200, Peter Zijlstra wrote:
> > > The ret_from_fork stub does an indirect call to the kthread function,
> > > but only knows about Retpolines. Instead of making the asm more
> > > complicated, punt to C and let the compiler figure it out.
> > > 
> > > Specifically, this makes it a proper kCFI indirect call when needed (in
> > > fact, it is nearly impossible to code a kCFI indirect call in asm).
> > > 
> > > This was the only callsite that was still calling func()+0 on regular
> > > indirect functions.
> > > 
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > 
> > I worry this creates a calling gadget, but I don't think it really
> > counts since it's just converting between two prototypes. Regardless:
> 
> Ah, since this will never be indirectly called, I should be able to
> annotate this so it never can be. Let me see what I can get the compiler
> to do.

I can't seem to manage to have it clobber it's __cfi hash value. Ideally
we'd have an attribute to force the thing to 0 or something.

Best I can do is add __noendbr, which will inhibit the ENDBR.

Alternatively, I *can* write the thing in asm by hard-coding the hash
value, but that's not nice:

	mov	%rbx,%r11
	mov	%r12,%rdi
#ifdef CONFIG_CFI_CLANG
	mov	$0x76049ec3,%r10d
	add	-0xf(%r11),%r10d
	je	1f
	ud2
1:
#endif
	CALL_NOSPEC r11

should work.. but if ever that hash function changes we're in trouble.

---
Subject: x86/cfi: Fix ret_from_fork() indirect calls
From: Peter Zijlstra <peterz@infradead.org>
Date: Thu, 15 Jun 2023 21:35:47 +0200

The ret_from_fork() stub does an indirect call to the kthread function,
but only knows about Retpolines. Instead of making the asm more
complicated, punt to C and let the compiler figure it out.

Specifically, this makes it a proper kCFI indirect call when needed (in
fact, it is nearly impossible to code a kCFI indirect call in asm).

This was the only callsite that was still calling func()+0 on regular
indirect functions.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
Link: https://lore.kernel.org/r/20230615193722.127844423@infradead.org
---
 arch/x86/entry/entry_64.S        |    6 ++++--
 arch/x86/include/asm/switch_to.h |    2 ++
 arch/x86/kernel/process_64.c     |    5 +++++
 3 files changed, 11 insertions(+), 2 deletions(-)

--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -304,8 +304,10 @@ SYM_CODE_START_NOALIGN(ret_from_fork)
 1:
 	/* kernel thread */
 	UNWIND_HINT_END_OF_STACK
-	movq	%r12, %rdi
-	CALL_NOSPEC rbx
+	movq	%rbx, %rdi
+	movq	%r12, %rsi
+	call	kthread_from_fork
+
 	/*
 	 * A kernel thread is allowed to return here after successfully
 	 * calling kernel_execve().  Exit to userspace to complete the execve()
--- a/arch/x86/include/asm/switch_to.h
+++ b/arch/x86/include/asm/switch_to.h
@@ -74,6 +74,8 @@ static inline void update_task_stack(str
 #endif
 }
 
+extern __noendbr void kthread_from_fork(int (*fn)(void *), void *arg);
+
 static inline void kthread_frame_init(struct inactive_task_frame *frame,
 				      int (*fun)(void *), void *arg)
 {
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -544,6 +544,11 @@ void compat_start_thread(struct pt_regs
 }
 #endif
 
+__visible __noendbr void kthread_from_fork(int (*fn)(void *), void *arg)
+{
+	fn(arg);
+}
+
 /*
  *	switch_to(x,y) should switch tasks from x to y.
  *

Re: [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls

[Kees Cook]

On Wed, Jun 21, 2023 at 11:27:59AM +0200, Peter Zijlstra wrote:
> On Wed, Jun 21, 2023 at 10:52:17AM +0200, Peter Zijlstra wrote:
> > On Tue, Jun 20, 2023 at 02:56:22PM -0700, Kees Cook wrote:
> > > On Thu, Jun 15, 2023 at 09:35:47PM +0200, Peter Zijlstra wrote:
> > > > The ret_from_fork stub does an indirect call to the kthread function,
> > > > but only knows about Retpolines. Instead of making the asm more
> > > > complicated, punt to C and let the compiler figure it out.
> > > > 
> > > > Specifically, this makes it a proper kCFI indirect call when needed (in
> > > > fact, it is nearly impossible to code a kCFI indirect call in asm).
> > > > 
> > > > This was the only callsite that was still calling func()+0 on regular
> > > > indirect functions.
> > > > 
> > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > 
> > > I worry this creates a calling gadget, but I don't think it really
> > > counts since it's just converting between two prototypes. Regardless:
> > 
> > Ah, since this will never be indirectly called, I should be able to
> > annotate this so it never can be. Let me see what I can get the compiler
> > to do.

Ah yeah, it should be direct-called only. I keep forgetting about the
endbr removal pass.

> I can't seem to manage to have it clobber it's __cfi hash value. Ideally
> we'd have an attribute to force the thing to 0 or something.

Doesn't objtool have logic to figure out this is only ever
direct-called?

-- 
Kees Cook

Re: [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls

[Peter Zijlstra]

On Wed, Jun 21, 2023 at 11:08:46AM -0700, Kees Cook wrote:

> Ah yeah, it should be direct-called only. I keep forgetting about the
> endbr removal pass.
> 
> > I can't seem to manage to have it clobber it's __cfi hash value. Ideally
> > we'd have an attribute to force the thing to 0 or something.
> 
> Doesn't objtool have logic to figure out this is only ever
> direct-called?

It does; let me also use that same thing to clobber the kCFI hashes for
these functions.

Re: [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls

[Peter Zijlstra]

On Wed, Jun 21, 2023 at 08:16:59PM +0200, Peter Zijlstra wrote:
> On Wed, Jun 21, 2023 at 11:08:46AM -0700, Kees Cook wrote:
> 
> > Ah yeah, it should be direct-called only. I keep forgetting about the
> > endbr removal pass.
> > 
> > > I can't seem to manage to have it clobber it's __cfi hash value. Ideally
> > > we'd have an attribute to force the thing to 0 or something.
> > 
> > Doesn't objtool have logic to figure out this is only ever
> > direct-called?
> 
> It does; let me also use that same thing to clobber the kCFI hashes for
> these functions.

Completely untested... gotta go put the kids to bed. I'll try later.

--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -778,6 +778,8 @@ void __init_or_module noinline apply_ret
 
 #ifdef CONFIG_X86_KERNEL_IBT
 
+static void poison_hash(void *addr);
+
 static void __init_or_module poison_endbr(void *addr, bool warn)
 {
 	u32 endbr, poison = gen_endbr_poison();
@@ -802,6 +804,9 @@ static void __init_or_module poison_endb
 
 /*
  * Generated by: objtool --ibt
+ *
+ * Seal the functions for indirect calls by clobbering the ENDBR instructions
+ * and the kCFI hash value.
  */
 void __init_or_module noinline apply_ibt_endbr(s32 *start, s32 *end)
 {
@@ -812,7 +817,7 @@ void __init_or_module noinline apply_ibt
 
 		poison_endbr(addr, true);
 		if (IS_ENABLED(CONFIG_FINEIBT))
-			poison_endbr(addr - 16, false);
+			poison_hash(addr - 16);
 	}
 }
 
@@ -1193,6 +1198,38 @@ static void __apply_fineibt(s32 *start_r
 	pr_err("Something went horribly wrong trying to rewrite the CFI implementation.\n");
 }
 
+static inline void __poison_hash(void *addr)
+{
+	*(u32 *)hash = 0;
+}
+
+static void poison_hash(void *addr)
+{
+	switch (cfi_mode) {
+	case CFI_FINEIBT:
+		/*
+		 * __cfi_\func:
+		 *	osp nopl (%rax)
+		 *	subl	$0, %r10d
+		 *	jz	1f
+		 *	ud2
+		 * 1:	nop
+		 */
+		poison_endbr(addr, false);
+		__poison_hash(addr + 7);
+		break;
+
+	case CFI_KCFI:
+		/*
+		 * __cfi_\func:
+		 *	movl	$0, %eax
+		 *	.skip	11, 0x90
+		 */
+		__poison_hash(addr + 1);
+		break;
+	}
+}
+
 #else
 
 static void __apply_fineibt(s32 *start_retpoline, s32 *end_retpoline,
@@ -1200,6 +1237,8 @@ static void __apply_fineibt(s32 *start_r
 {
 }
 
+static void poison_hash(void *addr) { }
+
 #endif
 
 void apply_fineibt(s32 *start_retpoline, s32 *end_retpoline,

Re: [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls

[Peter Zijlstra]

On Wed, Jun 21, 2023 at 08:33:56PM +0200, Peter Zijlstra wrote:
> On Wed, Jun 21, 2023 at 08:16:59PM +0200, Peter Zijlstra wrote:
> > On Wed, Jun 21, 2023 at 11:08:46AM -0700, Kees Cook wrote:
> > 
> > > Ah yeah, it should be direct-called only. I keep forgetting about the
> > > endbr removal pass.
> > > 
> > > > I can't seem to manage to have it clobber it's __cfi hash value. Ideally
> > > > we'd have an attribute to force the thing to 0 or something.
> > > 
> > > Doesn't objtool have logic to figure out this is only ever
> > > direct-called?
> > 
> > It does; let me also use that same thing to clobber the kCFI hashes for
> > these functions.
> 
> Completely untested... gotta go put the kids to bed. I'll try later.

With a few minor edits it seems to actually boot too. I'll go write up a
Changelog tomorrow. Now I've got to discover how Drizzt's adventure
continues ;-)

Re: [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls

[Brian Gerst]

On Thu, Jun 15, 2023 at 3:56 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> The ret_from_fork stub does an indirect call to the kthread function,
> but only knows about Retpolines. Instead of making the asm more
> complicated, punt to C and let the compiler figure it out.
>
> Specifically, this makes it a proper kCFI indirect call when needed (in
> fact, it is nearly impossible to code a kCFI indirect call in asm).
>
> This was the only callsite that was still calling func()+0 on regular
> indirect functions.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/entry/entry_64.S        |    6 ++++--
>  arch/x86/include/asm/switch_to.h |    2 ++
>  arch/x86/kernel/process_64.c     |    5 +++++
>  3 files changed, 11 insertions(+), 2 deletions(-)
>
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -304,8 +304,10 @@ SYM_CODE_START_NOALIGN(ret_from_fork)
>  1:
>         /* kernel thread */
>         UNWIND_HINT_END_OF_STACK
> -       movq    %r12, %rdi
> -       CALL_NOSPEC rbx
> +       movq    %rbx, %rdi
> +       movq    %r12, %rsi
> +       call    kthread_from_fork
> +
>         /*
>          * A kernel thread is allowed to return here after successfully
>          * calling kernel_execve().  Exit to userspace to complete the execve()
> --- a/arch/x86/include/asm/switch_to.h
> +++ b/arch/x86/include/asm/switch_to.h
> @@ -74,6 +74,8 @@ static inline void update_task_stack(str
>  #endif
>  }
>
> +extern void kthread_from_fork(int (*fn)(void *), void *);
> +
>  static inline void kthread_frame_init(struct inactive_task_frame *frame,
>                                       int (*fun)(void *), void *arg)
>  {
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -544,6 +544,11 @@ void compat_start_thread(struct pt_regs
>  }
>  #endif
>
> +__visible noinstr void kthread_from_fork(int (*fn)(void *), void *arg)
> +{
> +       fn(arg);
> +}
> +
>  /*
>   *     switch_to(x,y) should switch tasks from x to y.
>   *

I think this makes a case for converting all of ret_from_fork() to C
(other than some minimal asm glue).  Patches coming soon.

Brian Gerst

[PATCH 2/2] x86/fineibt: Poison ENDBR at +0

[Peter Zijlstra]

Alyssa noticed that when building the kernel with CFI_CLANG+IBT and
booting on IBT enabled hardware obtain FineIBT, the indirect functions
look like:

  __cfi_foo:
	endbr64
	subl	$hash, %r10d
	jz	1f
	ud2
	nop
  1:
  foo:
	endbr64

This is because clang currently does not supress ENDBR emission for
functions it provides a __cfi prologue symbol for.

Having this second ENDBR however makes it possible to elide the CFI
check. Therefore, we should poison this second ENDBR (if present) when
switching to FineIBT mode.

Fixes: 931ab63664f0 ("x86/ibt: Implement FineIBT")
Reported-by: "Milburn, Alyssa" <alyssa.milburn@intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/alternative.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -940,6 +940,17 @@ static int cfi_rewrite_preamble(s32 *sta
 	return 0;
 }
 
+static void cfi_rewrite_endbr(s32 *start, s32 *end)
+{
+	s32 *s;
+
+	for (s = start; s < end; s++) {
+		void *addr = (void *)s + *s;
+
+		poison_endbr(addr+16, false);
+	}
+}
+
 /* .retpoline_sites */
 static int cfi_rand_callers(s32 *start, s32 *end)
 {
@@ -1034,14 +1045,19 @@ static void __apply_fineibt(s32 *start_r
 		return;
 
 	case CFI_FINEIBT:
+		/* place the FineIBT preamble at func()-16 */
 		ret = cfi_rewrite_preamble(start_cfi, end_cfi);
 		if (ret)
 			goto err;
 
+		/* rewrite the callers to target func()-16 */
 		ret = cfi_rewrite_callers(start_retpoline, end_retpoline);
 		if (ret)
 			goto err;
 
+		/* now that nobody targets func()+0, remove ENDBR there */
+		cfi_rewrite_endbr(start_cfi, end_cfi);
+
 		if (builtin)
 			pr_info("Using FineIBT CFI\n");
 		return;

Re: [PATCH 2/2] x86/fineibt: Poison ENDBR at +0

[Kees Cook]

On Thu, Jun 15, 2023 at 09:35:48PM +0200, Peter Zijlstra wrote:
> Alyssa noticed that when building the kernel with CFI_CLANG+IBT and
> booting on IBT enabled hardware obtain FineIBT, the indirect functions
> look like:
> 
>   __cfi_foo:
> 	endbr64
> 	subl	$hash, %r10d
> 	jz	1f
> 	ud2
> 	nop
>   1:
>   foo:
> 	endbr64
> 
> This is because clang currently does not supress ENDBR emission for
> functions it provides a __cfi prologue symbol for.

Should this be considered a bug in Clang?

> 
> Having this second ENDBR however makes it possible to elide the CFI
> check. Therefore, we should poison this second ENDBR (if present) when
> switching to FineIBT mode.
> 
> Fixes: 931ab63664f0 ("x86/ibt: Implement FineIBT")
> Reported-by: "Milburn, Alyssa" <alyssa.milburn@intel.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Looks like a good work-around.

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 2/2] x86/fineibt: Poison ENDBR at +0

[Sami Tolvanen]

On Tue, Jun 20, 2023 at 2:55 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Jun 15, 2023 at 09:35:48PM +0200, Peter Zijlstra wrote:
> > Alyssa noticed that when building the kernel with CFI_CLANG+IBT and
> > booting on IBT enabled hardware obtain FineIBT, the indirect functions
> > look like:
> >
> >   __cfi_foo:
> >       endbr64
> >       subl    $hash, %r10d
> >       jz      1f
> >       ud2
> >       nop
> >   1:
> >   foo:
> >       endbr64
> >
> > This is because clang currently does not supress ENDBR emission for
> > functions it provides a __cfi prologue symbol for.
>
> Should this be considered a bug in Clang?

The endbr is needed with KCFI if we have FineIBT disabled for some
reason. There's some discussion here:

https://github.com/ClangBuiltLinux/linux/issues/1735

However, since the kernel handles FineIBT patching, it might be
cleaner to let it also poison the extra endbr.

Sami

Re: [PATCH 2/2] x86/fineibt: Poison ENDBR at +0

[Peter Zijlstra]

On Tue, Jun 20, 2023 at 05:04:50PM -0700, Sami Tolvanen wrote:
> On Tue, Jun 20, 2023 at 2:55 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Thu, Jun 15, 2023 at 09:35:48PM +0200, Peter Zijlstra wrote:
> > > Alyssa noticed that when building the kernel with CFI_CLANG+IBT and
> > > booting on IBT enabled hardware obtain FineIBT, the indirect functions
> > > look like:
> > >
> > >   __cfi_foo:
> > >       endbr64
> > >       subl    $hash, %r10d
> > >       jz      1f
> > >       ud2
> > >       nop
> > >   1:
> > >   foo:
> > >       endbr64
> > >
> > > This is because clang currently does not supress ENDBR emission for
> > > functions it provides a __cfi prologue symbol for.
> >
> > Should this be considered a bug in Clang?
> 
> The endbr is needed with KCFI if we have FineIBT disabled for some
> reason. There's some discussion here:
> 
> https://github.com/ClangBuiltLinux/linux/issues/1735
> 
> However, since the kernel handles FineIBT patching, it might be
> cleaner to let it also poison the extra endbr.

That's what I get for replying before reading all replies. Anyway, we're
in agreement.

Re: [PATCH 2/2] x86/fineibt: Poison ENDBR at +0

[Peter Zijlstra]

On Tue, Jun 20, 2023 at 02:55:10PM -0700, Kees Cook wrote:
> On Thu, Jun 15, 2023 at 09:35:48PM +0200, Peter Zijlstra wrote:
> > Alyssa noticed that when building the kernel with CFI_CLANG+IBT and
> > booting on IBT enabled hardware obtain FineIBT, the indirect functions
> > look like:
> > 
> >   __cfi_foo:
> > 	endbr64
> > 	subl	$hash, %r10d
> > 	jz	1f
> > 	ud2
> > 	nop
> >   1:
> >   foo:
> > 	endbr64
> > 
> > This is because clang currently does not supress ENDBR emission for
> > functions it provides a __cfi prologue symbol for.
> 
> Should this be considered a bug in Clang?

No, I don't think so. I was going to say this is perhaps insufficiently
explored space, but upon more consideration I think this is actually
correct behaviour (and I need to write a better Changelog).

The issue is that the compiler generates code for kCFI+IBT, it doesn't
know about FineIBT *at*all*. Additionally, one can inhibit patching of
FineIBT by booting with 'cfi=kcfi' on IBT enabled hardware.

And in that case (kCFI+IBT), we'll do the caller hash check and still
jump to +0, so there really must be an ENDBR there.

Only if we were to dis-allow this combination could we say the ENDBR at
+0 becomes superfluous and should find means for the compiler not emit
it.

> > Having this second ENDBR however makes it possible to elide the CFI
> > check. Therefore, we should poison this second ENDBR (if present) when
> > switching to FineIBT mode.
> > 
> > Fixes: 931ab63664f0 ("x86/ibt: Implement FineIBT")
> > Reported-by: "Milburn, Alyssa" <alyssa.milburn@intel.com>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> Looks like a good work-around.
> 
> Acked-by: Kees Cook <keescook@chromium.org>

Thanks!

Re: [PATCH 2/2] x86/fineibt: Poison ENDBR at +0

[Peter Zijlstra]

On Wed, Jun 21, 2023 at 10:18:57AM +0200, Peter Zijlstra wrote:
> (and I need to write a better Changelog).

Updated changelog...

---
Subject: x86/fineibt: Poison ENDBR at +0
From: Peter Zijlstra <peterz@infradead.org>
Date: Thu, 15 Jun 2023 21:35:48 +0200

Alyssa noticed that when building the kernel with CFI_CLANG+IBT and
booting on IBT enabled hardware obtain FineIBT, the indirect functions
look like:

  __cfi_foo:
	endbr64
	subl	$hash, %r10d
	jz	1f
	ud2
	nop
  1:
  foo:
	endbr64

This is because the compiler generates code for kCFI+IBT. In that case
the caller does the hash check and will jump to +0, so there must be
an ENDBR there. The compiler doesn't know about FineIBT at all; also
it is possible to actually use kCFI+IBT when booting with 'cfi=kcfi'
on IBT enabled hardware.

Having this second ENDBR however makes it possible to elide the CFI
check. Therefore, we should poison this second ENDBR when switching to
FineIBT mode.

Fixes: 931ab63664f0 ("x86/ibt: Implement FineIBT")
Reported-by: "Milburn, Alyssa" <alyssa.milburn@intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
Acked-by: Kees Cook <keescook@chromium.org>
Link: https://lore.kernel.org/r/20230615193722.194131053@infradead.org
---
 arch/x86/kernel/alternative.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -1063,6 +1063,17 @@ static int cfi_rewrite_preamble(s32 *sta
 	return 0;
 }
 
+static void cfi_rewrite_endbr(s32 *start, s32 *end)
+{
+	s32 *s;
+
+	for (s = start; s < end; s++) {
+		void *addr = (void *)s + *s;
+
+		poison_endbr(addr+16, false);
+	}
+}
+
 /* .retpoline_sites */
 static int cfi_rand_callers(s32 *start, s32 *end)
 {
@@ -1157,14 +1168,19 @@ static void __apply_fineibt(s32 *start_r
 		return;
 
 	case CFI_FINEIBT:
+		/* place the FineIBT preamble at func()-16 */
 		ret = cfi_rewrite_preamble(start_cfi, end_cfi);
 		if (ret)
 			goto err;
 
+		/* rewrite the callers to target func()-16 */
 		ret = cfi_rewrite_callers(start_retpoline, end_retpoline);
 		if (ret)
 			goto err;
 
+		/* now that nobody targets func()+0, remove ENDBR there */
+		cfi_rewrite_endbr(start_cfi, end_cfi);
+
 		if (builtin)
 			pr_info("Using FineIBT CFI\n");
 		return;

Re: [PATCH 2/2] x86/fineibt: Poison ENDBR at +0

[Kees Cook]

On Wed, Jun 21, 2023 at 10:48:02AM +0200, Peter Zijlstra wrote:
> On Wed, Jun 21, 2023 at 10:18:57AM +0200, Peter Zijlstra wrote:
> > (and I need to write a better Changelog).
> 
> Updated changelog...

LGTM! Thanks :)

-- 
Kees Cook

[tip: x86/urgent] x86/fineibt: Poison ENDBR at +0

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     04505bbbbb15da950ea0239e328a76a3ad2376e0
Gitweb:        https://git.kernel.org/tip/04505bbbbb15da950ea0239e328a76a3ad2376e0
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Thu, 15 Jun 2023 21:35:48 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Mon, 10 Jul 2023 09:52:25 +02:00

x86/fineibt: Poison ENDBR at +0

Alyssa noticed that when building the kernel with CFI_CLANG+IBT and
booting on IBT enabled hardware to obtain FineIBT, the indirect
functions look like:

  __cfi_foo:
	endbr64
	subl	$hash, %r10d
	jz	1f
	ud2
	nop
  1:
  foo:
	endbr64

This is because the compiler generates code for kCFI+IBT. In that case
the caller does the hash check and will jump to +0, so there must be
an ENDBR there. The compiler doesn't know about FineIBT at all; also
it is possible to actually use kCFI+IBT when booting with 'cfi=kcfi'
on IBT enabled hardware.

Having this second ENDBR however makes it possible to elide the CFI
check. Therefore, we should poison this second ENDBR when switching to
FineIBT mode.

Fixes: 931ab63664f0 ("x86/ibt: Implement FineIBT")
Reported-by: "Milburn, Alyssa" <alyssa.milburn@intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
Link: https://lore.kernel.org/r/20230615193722.194131053@infradead.org
---
 arch/x86/kernel/alternative.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 04b25a2..d77aaab 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -1068,6 +1068,17 @@ static int cfi_rewrite_preamble(s32 *start, s32 *end)
 	return 0;
 }
 
+static void cfi_rewrite_endbr(s32 *start, s32 *end)
+{
+	s32 *s;
+
+	for (s = start; s < end; s++) {
+		void *addr = (void *)s + *s;
+
+		poison_endbr(addr+16, false);
+	}
+}
+
 /* .retpoline_sites */
 static int cfi_rand_callers(s32 *start, s32 *end)
 {
@@ -1162,14 +1173,19 @@ static void __apply_fineibt(s32 *start_retpoline, s32 *end_retpoline,
 		return;
 
 	case CFI_FINEIBT:
+		/* place the FineIBT preamble at func()-16 */
 		ret = cfi_rewrite_preamble(start_cfi, end_cfi);
 		if (ret)
 			goto err;
 
+		/* rewrite the callers to target func()-16 */
 		ret = cfi_rewrite_callers(start_retpoline, end_retpoline);
 		if (ret)
 			goto err;
 
+		/* now that nobody targets func()+0, remove ENDBR there */
+		cfi_rewrite_endbr(start_cfi, end_cfi);
+
 		if (builtin)
 			pr_info("Using FineIBT CFI\n");
 		return;

Re: [PATCH 0/2] x86/cfi: Fix FineIBT

[Sami Tolvanen]

On Thu, Jun 15, 2023 at 12:40 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> Hi!
>
> Alyssa reported a FineIBT issue; these patches sort that.
>
> Basically the morale of the story is that CALL_NOSPEC and JMP_NOSPEC are
> suspect and likely not what you want. The remaining users are:
>
> arch/x86/crypto/crc32c-pcl-intel-asm_64.S:      JMP_NOSPEC bufp
>
> This is broken on anything IBT afaict.
>
> arch/x86/include/asm/mshyperv.h:                             CALL_NOSPEC
> arch/x86/include/asm/mshyperv.h:        __asm__ __volatile__(CALL_NOSPEC
> arch/x86/include/asm/mshyperv.h:                __asm__ __volatile__(CALL_NOSPEC
> arch/x86/include/asm/mshyperv.h:                __asm__ __volatile__ (CALL_NOSPEC
> arch/x86/include/asm/mshyperv.h:                                     CALL_NOSPEC
> arch/x86/include/asm/mshyperv.h:                __asm__ __volatile__ (CALL_NOSPEC
> arch/x86/include/asm/xen/hypercall.h:   asm volatile(CALL_NOSPEC
>
> These are hypercalls and supposedly the targets are having ENDBR on.
>
> arch/x86/kernel/ftrace_64.S:    CALL_NOSPEC r8
>
> The thing is !DYNAMIC_FTRACE only, which we can fix with a Kconfig
> dependency I suppose.
>
> arch/x86/kvm/emulate.c: asm("push %[flags]; popf; " CALL_NOSPEC
> arch/x86/kvm/emulate.c: asm("push %[flags]; popf; " CALL_NOSPEC " ; pushf; pop %[flags]\n"
>
> calls into the magic fastop stuff and should be ok, those have explicit
> ENDBR + IBT_NOSEAL() annotations.
>
> arch/x86/kvm/vmx/vmenter.S:     VMX_DO_EVENT_IRQOFF CALL_NOSPEC _ASM_ARG1
>
> calls the IDT vectors manually, so should be ok (gross, but not broken
> from this pov).
>
> arch/x86/platform/efi/efi_stub_64.S:    CALL_NOSPEC rdi
>
> effectively a nocfi call, so should be ok.

Thanks, this looks good to me. For the series:

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

Sami