* [PATCH 0/2] x86/cfi: Fix FineIBT
@ 2023-06-15 19:35 Peter Zijlstra
2023-06-15 19:35 ` [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls Peter Zijlstra
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Peter Zijlstra @ 2023-06-15 19:35 UTC (permalink / raw
To: x86, alyssa.milburn
Cc: linux-kernel, peterz, samitolvanen, keescook, jpoimboe, joao,
tim.c.chen
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls
2023-06-15 19:35 [PATCH 0/2] x86/cfi: Fix FineIBT Peter Zijlstra
@ 2023-06-15 19:35 ` Peter Zijlstra
2023-06-20 21:56 ` Kees Cook
2023-06-21 21:07 ` Brian Gerst
2023-06-15 19:35 ` [PATCH 2/2] x86/fineibt: Poison ENDBR at +0 Peter Zijlstra
2023-06-16 23:24 ` [PATCH 0/2] x86/cfi: Fix FineIBT Sami Tolvanen
2 siblings, 2 replies; 19+ messages in thread
From: Peter Zijlstra @ 2023-06-15 19:35 UTC (permalink / raw
To: x86, alyssa.milburn
Cc: linux-kernel, peterz, samitolvanen, keescook, jpoimboe, joao,
tim.c.chen
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.
*
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/2] x86/fineibt: Poison ENDBR at +0
2023-06-15 19:35 [PATCH 0/2] x86/cfi: Fix FineIBT Peter Zijlstra
2023-06-15 19:35 ` [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls Peter Zijlstra
@ 2023-06-15 19:35 ` Peter Zijlstra
2023-06-20 21:55 ` Kees Cook
2023-07-10 8:13 ` [tip: x86/urgent] " tip-bot2 for Peter Zijlstra
2023-06-16 23:24 ` [PATCH 0/2] x86/cfi: Fix FineIBT Sami Tolvanen
2 siblings, 2 replies; 19+ messages in thread
From: Peter Zijlstra @ 2023-06-15 19:35 UTC (permalink / raw
To: x86, alyssa.milburn
Cc: linux-kernel, peterz, samitolvanen, keescook, jpoimboe, joao,
tim.c.chen
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;
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/2] x86/cfi: Fix FineIBT
2023-06-15 19:35 [PATCH 0/2] x86/cfi: Fix FineIBT Peter Zijlstra
2023-06-15 19:35 ` [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls Peter Zijlstra
2023-06-15 19:35 ` [PATCH 2/2] x86/fineibt: Poison ENDBR at +0 Peter Zijlstra
@ 2023-06-16 23:24 ` Sami Tolvanen
2 siblings, 0 replies; 19+ messages in thread
From: Sami Tolvanen @ 2023-06-16 23:24 UTC (permalink / raw
To: Peter Zijlstra
Cc: x86, alyssa.milburn, linux-kernel, keescook, jpoimboe, joao,
tim.c.chen
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] x86/fineibt: Poison ENDBR at +0
2023-06-15 19:35 ` [PATCH 2/2] x86/fineibt: Poison ENDBR at +0 Peter Zijlstra
@ 2023-06-20 21:55 ` Kees Cook
2023-06-21 0:04 ` Sami Tolvanen
2023-06-21 8:18 ` Peter Zijlstra
2023-07-10 8:13 ` [tip: x86/urgent] " tip-bot2 for Peter Zijlstra
1 sibling, 2 replies; 19+ messages in thread
From: Kees Cook @ 2023-06-20 21:55 UTC (permalink / raw
To: Peter Zijlstra
Cc: x86, alyssa.milburn, linux-kernel, samitolvanen, jpoimboe, joao,
tim.c.chen
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls
2023-06-15 19:35 ` [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls Peter Zijlstra
@ 2023-06-20 21:56 ` Kees Cook
2023-06-21 8:52 ` Peter Zijlstra
2023-06-21 21:07 ` Brian Gerst
1 sibling, 1 reply; 19+ messages in thread
From: Kees Cook @ 2023-06-20 21:56 UTC (permalink / raw
To: Peter Zijlstra
Cc: x86, alyssa.milburn, linux-kernel, samitolvanen, jpoimboe, joao,
tim.c.chen
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] x86/fineibt: Poison ENDBR at +0
2023-06-20 21:55 ` Kees Cook
@ 2023-06-21 0:04 ` Sami Tolvanen
2023-06-21 8:51 ` Peter Zijlstra
2023-06-21 8:18 ` Peter Zijlstra
1 sibling, 1 reply; 19+ messages in thread
From: Sami Tolvanen @ 2023-06-21 0:04 UTC (permalink / raw
To: Kees Cook
Cc: Peter Zijlstra, x86, alyssa.milburn, linux-kernel, jpoimboe, joao,
tim.c.chen
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] x86/fineibt: Poison ENDBR at +0
2023-06-20 21:55 ` Kees Cook
2023-06-21 0:04 ` Sami Tolvanen
@ 2023-06-21 8:18 ` Peter Zijlstra
2023-06-21 8:48 ` Peter Zijlstra
1 sibling, 1 reply; 19+ messages in thread
From: Peter Zijlstra @ 2023-06-21 8:18 UTC (permalink / raw
To: Kees Cook
Cc: x86, alyssa.milburn, linux-kernel, samitolvanen, jpoimboe, joao,
tim.c.chen
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!
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] x86/fineibt: Poison ENDBR at +0
2023-06-21 8:18 ` Peter Zijlstra
@ 2023-06-21 8:48 ` Peter Zijlstra
2023-06-21 18:07 ` Kees Cook
0 siblings, 1 reply; 19+ messages in thread
From: Peter Zijlstra @ 2023-06-21 8:48 UTC (permalink / raw
To: Kees Cook
Cc: x86, alyssa.milburn, linux-kernel, samitolvanen, jpoimboe, joao,
tim.c.chen
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;
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] x86/fineibt: Poison ENDBR at +0
2023-06-21 0:04 ` Sami Tolvanen
@ 2023-06-21 8:51 ` Peter Zijlstra
0 siblings, 0 replies; 19+ messages in thread
From: Peter Zijlstra @ 2023-06-21 8:51 UTC (permalink / raw
To: Sami Tolvanen
Cc: Kees Cook, x86, alyssa.milburn, linux-kernel, jpoimboe, joao,
tim.c.chen
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls
2023-06-20 21:56 ` Kees Cook
@ 2023-06-21 8:52 ` Peter Zijlstra
2023-06-21 9:27 ` Peter Zijlstra
0 siblings, 1 reply; 19+ messages in thread
From: Peter Zijlstra @ 2023-06-21 8:52 UTC (permalink / raw
To: Kees Cook
Cc: x86, alyssa.milburn, linux-kernel, samitolvanen, jpoimboe, joao,
tim.c.chen
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls
2023-06-21 8:52 ` Peter Zijlstra
@ 2023-06-21 9:27 ` Peter Zijlstra
2023-06-21 18:08 ` Kees Cook
0 siblings, 1 reply; 19+ messages in thread
From: Peter Zijlstra @ 2023-06-21 9:27 UTC (permalink / raw
To: Kees Cook
Cc: x86, alyssa.milburn, linux-kernel, samitolvanen, jpoimboe, joao,
tim.c.chen
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.
*
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] x86/fineibt: Poison ENDBR at +0
2023-06-21 8:48 ` Peter Zijlstra
@ 2023-06-21 18:07 ` Kees Cook
0 siblings, 0 replies; 19+ messages in thread
From: Kees Cook @ 2023-06-21 18:07 UTC (permalink / raw
To: Peter Zijlstra
Cc: x86, alyssa.milburn, linux-kernel, samitolvanen, jpoimboe, joao,
tim.c.chen
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls
2023-06-21 9:27 ` Peter Zijlstra
@ 2023-06-21 18:08 ` Kees Cook
2023-06-21 18:16 ` Peter Zijlstra
0 siblings, 1 reply; 19+ messages in thread
From: Kees Cook @ 2023-06-21 18:08 UTC (permalink / raw
To: Peter Zijlstra
Cc: x86, alyssa.milburn, linux-kernel, samitolvanen, jpoimboe, joao,
tim.c.chen
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls
2023-06-21 18:08 ` Kees Cook
@ 2023-06-21 18:16 ` Peter Zijlstra
2023-06-21 18:33 ` Peter Zijlstra
0 siblings, 1 reply; 19+ messages in thread
From: Peter Zijlstra @ 2023-06-21 18:16 UTC (permalink / raw
To: Kees Cook
Cc: x86, alyssa.milburn, linux-kernel, samitolvanen, jpoimboe, joao,
tim.c.chen
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls
2023-06-21 18:16 ` Peter Zijlstra
@ 2023-06-21 18:33 ` Peter Zijlstra
2023-06-21 20:13 ` Peter Zijlstra
0 siblings, 1 reply; 19+ messages in thread
From: Peter Zijlstra @ 2023-06-21 18:33 UTC (permalink / raw
To: Kees Cook
Cc: x86, alyssa.milburn, linux-kernel, samitolvanen, jpoimboe, joao,
tim.c.chen
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,
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls
2023-06-21 18:33 ` Peter Zijlstra
@ 2023-06-21 20:13 ` Peter Zijlstra
0 siblings, 0 replies; 19+ messages in thread
From: Peter Zijlstra @ 2023-06-21 20:13 UTC (permalink / raw
To: Kees Cook
Cc: x86, alyssa.milburn, linux-kernel, samitolvanen, jpoimboe, joao,
tim.c.chen
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 ;-)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls
2023-06-15 19:35 ` [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls Peter Zijlstra
2023-06-20 21:56 ` Kees Cook
@ 2023-06-21 21:07 ` Brian Gerst
1 sibling, 0 replies; 19+ messages in thread
From: Brian Gerst @ 2023-06-21 21:07 UTC (permalink / raw
To: Peter Zijlstra
Cc: x86, alyssa.milburn, linux-kernel, samitolvanen, keescook,
jpoimboe, joao, tim.c.chen
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [tip: x86/urgent] x86/fineibt: Poison ENDBR at +0
2023-06-15 19:35 ` [PATCH 2/2] x86/fineibt: Poison ENDBR at +0 Peter Zijlstra
2023-06-20 21:55 ` Kees Cook
@ 2023-07-10 8:13 ` tip-bot2 for Peter Zijlstra
1 sibling, 0 replies; 19+ messages in thread
From: tip-bot2 for Peter Zijlstra @ 2023-07-10 8:13 UTC (permalink / raw
To: linux-tip-commits
Cc: Milburn, Alyssa, Peter Zijlstra (Intel), Kees Cook, Sami Tolvanen,
x86, linux-kernel
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;
^ permalink raw reply related [flat|nested] 19+ messages in thread
end of thread, other threads:[~2023-07-10 8:15 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-15 19:35 [PATCH 0/2] x86/cfi: Fix FineIBT Peter Zijlstra
2023-06-15 19:35 ` [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls Peter Zijlstra
2023-06-20 21:56 ` Kees Cook
2023-06-21 8:52 ` Peter Zijlstra
2023-06-21 9:27 ` Peter Zijlstra
2023-06-21 18:08 ` Kees Cook
2023-06-21 18:16 ` Peter Zijlstra
2023-06-21 18:33 ` Peter Zijlstra
2023-06-21 20:13 ` Peter Zijlstra
2023-06-21 21:07 ` Brian Gerst
2023-06-15 19:35 ` [PATCH 2/2] x86/fineibt: Poison ENDBR at +0 Peter Zijlstra
2023-06-20 21:55 ` Kees Cook
2023-06-21 0:04 ` Sami Tolvanen
2023-06-21 8:51 ` Peter Zijlstra
2023-06-21 8:18 ` Peter Zijlstra
2023-06-21 8:48 ` Peter Zijlstra
2023-06-21 18:07 ` Kees Cook
2023-07-10 8:13 ` [tip: x86/urgent] " tip-bot2 for Peter Zijlstra
2023-06-16 23:24 ` [PATCH 0/2] x86/cfi: Fix FineIBT Sami Tolvanen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).