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) <[email protected]>
---
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.
*
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) <[email protected]>
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 <[email protected]>
--
Kees Cook
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) <[email protected]>
>
> 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.
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) <[email protected]>
> >
> > 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 <[email protected]>
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) <[email protected]>
Reviewed-by: Sami Tolvanen <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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.
*
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) <[email protected]>
> > >
> > > 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
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.
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,
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 ;-)
On Thu, Jun 15, 2023 at 3:56 PM Peter Zijlstra <[email protected]> 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) <[email protected]>
> ---
> 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