v5.15.57 regression - boot panic after retbleed backports with CONFIG_KPROBES_SANITY_TEST=y

v5.15.57 regression - boot panic after retbleed backports with CONFIG_KPROBES_SANITY_TEST=y

[Paul Gortmaker]

The panic comes from the sanity test code, but after trying to boil down the
.config differences between the kitchen sink our test team uses, and a
"defconfig", it seems there are at least a couple extra dependencies for
creating a reproducer:

  make defconfig
  echo CONFIG_FUNCTION_TRACER=y >> .config
  echo CONFIG_KPROBES_SANITY_TEST=y >> .config
  echo CONFIG_UNWINDER_FRAME_POINTER=y >> .config
  yes "" | make oldconfig

Note that ftrace is probably just opening the door to CONFIG_KPROBES_ON_FTRACE=y

The report I got was with gcc-11 on an Atom; I was able to reproduce it
with the default gcc-7 found on Ubuntu 18.04 and booting on a Xeon v2 -
so it seems to not be specific to gcc options or processor features.

I don't know if the v5.15 backports were specifically tested to be fully
bisectable, but if we assume they are, a bisect between 56 and 57 says:

   commit 1d61a2988612ac0632134454d5407c63ae0b9d42 (refs/bisect/bad)
   Author: Peter Zijlstra <peterz@infradead.org>
   Date:   Tue Jun 14 23:15:45 2022 +0200
   
       x86: Use return-thunk in asm code
       
       commit aa3d480315ba6c3025a60958e1981072ea37c3df upstream.
       
       Use the return thunk in asm code. If the thunk isn't needed, it will
       get patched into a RET instruction during boot by apply_returns().

Splat follows:

   rcu: Hierarchical SRCU implementation.
   Kprobe smoke test: started
   BUG: unable to handle page fault for address: ffffffffc110f3e7
   #PF: supervisor instruction fetch in kernel mode
   #PF: error_code(0x0010) - not-present page
   PGD b2c60f067 P4D b2c60f067 PUD b2c611067 PMD 0
   Oops: 0010 [#1] SMP NOPTI
   CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.15.57 #33
   Hardware name: Intel Corporation S2600CP/S2600CP, BIOS SE5C600.86B.02.06.E006.013120181511 01/31/2018
   RIP: 0010:0xffffffffc110f3e7
   Code: Unable to access opcode bytes at RIP 0xffffffffc110f3bd.
   RSP: 0000:ffffae4bc006be38 EFLAGS: 00010246
   RAX: ffffffffb973f310 RBX: 0000000000000000 RCX: 0000000000000000
   RDX: 0000000000000000 RSI: 0000000000000000 RDI: 000000005856e7bd
   RBP: ffffae4bc006be60 R08: 0000000000000000 R09: 0000000000000001
   R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000001
   R13: ffffffffbae38560 R14: 0000000000000000 R15: 0000000000000000
   FS:  0000000000000000(0000) GS:ffff8c92df800000(0000) knlGS:0000000000000000
   CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
   CR2: ffffffffc110f3bd CR3: 0000000b2c60c001 CR4: 00000000001706f0
   Call Trace:
    <TASK>
    ? kprobe_target+0x5/0x20
    ? init_test_probes+0x78/0x420
    init_kprobes+0x16c/0x18e
    ? init_optprobes+0x27/0x27
    do_one_initcall+0x43/0x1d0
    kernel_init_freeable+0xf1/0x240
    ? rest_init+0xd0/0xd0
    kernel_init+0x1a/0x120
    ret_from_fork+0x1f/0x30
    </TASK>
   Modules linked in:
   CR2: ffffffffc110f3e7
   ---[ end trace 759f040622219261 ]---

Re: v5.15.57 regression - boot panic after retbleed backports with CONFIG_KPROBES_SANITY_TEST=y

[Thadeu Lima de Souza Cascardo]

On Fri, Aug 05, 2022 at 04:04:38PM -0400, Paul Gortmaker wrote:
> The panic comes from the sanity test code, but after trying to boil down the
> .config differences between the kitchen sink our test team uses, and a
> "defconfig", it seems there are at least a couple extra dependencies for
> creating a reproducer:
> 
>   make defconfig
>   echo CONFIG_FUNCTION_TRACER=y >> .config
>   echo CONFIG_KPROBES_SANITY_TEST=y >> .config
>   echo CONFIG_UNWINDER_FRAME_POINTER=y >> .config
>   yes "" | make oldconfig
> 
> Note that ftrace is probably just opening the door to CONFIG_KPROBES_ON_FTRACE=y
> 
> The report I got was with gcc-11 on an Atom; I was able to reproduce it
> with the default gcc-7 found on Ubuntu 18.04 and booting on a Xeon v2 -
> so it seems to not be specific to gcc options or processor features.
> 
> I don't know if the v5.15 backports were specifically tested to be fully
> bisectable, but if we assume they are, a bisect between 56 and 57 says:
> 
>    commit 1d61a2988612ac0632134454d5407c63ae0b9d42 (refs/bisect/bad)
>    Author: Peter Zijlstra <peterz@infradead.org>
>    Date:   Tue Jun 14 23:15:45 2022 +0200
>    
>        x86: Use return-thunk in asm code
>        
>        commit aa3d480315ba6c3025a60958e1981072ea37c3df upstream.
>        
>        Use the return thunk in asm code. If the thunk isn't needed, it will
>        get patched into a RET instruction during boot by apply_returns().
> 
> Splat follows:
> 
>    rcu: Hierarchical SRCU implementation.
>    Kprobe smoke test: started
>    BUG: unable to handle page fault for address: ffffffffc110f3e7
>    #PF: supervisor instruction fetch in kernel mode
>    #PF: error_code(0x0010) - not-present page
>    PGD b2c60f067 P4D b2c60f067 PUD b2c611067 PMD 0
>    Oops: 0010 [#1] SMP NOPTI
>    CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.15.57 #33
>    Hardware name: Intel Corporation S2600CP/S2600CP, BIOS SE5C600.86B.02.06.E006.013120181511 01/31/2018
>    RIP: 0010:0xffffffffc110f3e7
>    Code: Unable to access opcode bytes at RIP 0xffffffffc110f3bd.
>    RSP: 0000:ffffae4bc006be38 EFLAGS: 00010246
>    RAX: ffffffffb973f310 RBX: 0000000000000000 RCX: 0000000000000000
>    RDX: 0000000000000000 RSI: 0000000000000000 RDI: 000000005856e7bd
>    RBP: ffffae4bc006be60 R08: 0000000000000000 R09: 0000000000000001
>    R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000001
>    R13: ffffffffbae38560 R14: 0000000000000000 R15: 0000000000000000
>    FS:  0000000000000000(0000) GS:ffff8c92df800000(0000) knlGS:0000000000000000
>    CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>    CR2: ffffffffc110f3bd CR3: 0000000b2c60c001 CR4: 00000000001706f0
>    Call Trace:
>     <TASK>
>     ? kprobe_target+0x5/0x20
>     ? init_test_probes+0x78/0x420
>     init_kprobes+0x16c/0x18e
>     ? init_optprobes+0x27/0x27
>     do_one_initcall+0x43/0x1d0
>     kernel_init_freeable+0xf1/0x240
>     ? rest_init+0xd0/0xd0
>     kernel_init+0x1a/0x120
>     ret_from_fork+0x1f/0x30
>     </TASK>
>    Modules linked in:
>    CR2: ffffffffc110f3e7
>    ---[ end trace 759f040622219261 ]---

Can you try the patch below?

Thanks.
Cascardo.

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 74c2f88a43d0..6bb479ce1ae4 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -321,12 +321,12 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 	unsigned long offset;
 	unsigned long npages;
 	unsigned long size;
-	unsigned long retq;
 	unsigned long *ptr;
 	void *trampoline;
 	void *ip;
 	/* 48 8b 15 <offset> is movq <offset>(%rip), %rdx */
 	unsigned const char op_ref[] = { 0x48, 0x8b, 0x15 };
+	unsigned const char retq[] = { RET_INSN_OPCODE, INT3_INSN_OPCODE };
 	union ftrace_op_code_union op_ptr;
 	int ret;
 
@@ -364,15 +364,10 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 		goto fail;
 
 	ip = trampoline + size;
-
-	/* The trampoline ends with ret(q) */
-	retq = (unsigned long)ftrace_stub;
 	if (cpu_feature_enabled(X86_FEATURE_RETHUNK))
 		memcpy(ip, text_gen_insn(JMP32_INSN_OPCODE, ip, &__x86_return_thunk), JMP32_INSN_SIZE);
 	else
-		ret = copy_from_kernel_nofault(ip, (void *)retq, RET_SIZE);
-	if (WARN_ON(ret < 0))
-		goto fail;
+		memcpy(ip, retq, sizeof(retq));
 
 	/* No need to test direct calls on created trampolines */
 	if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {

Re: v5.15.57 regression - boot panic after retbleed backports with CONFIG_KPROBES_SANITY_TEST=y

[Paul Gortmaker]

[Re: v5.15.57 regression - boot panic after retbleed backports with CONFIG_KPROBES_SANITY_TEST=y] On 05/08/2022 (Fri 18:13) Thadeu Lima de Souza Cascardo wrote:

> On Fri, Aug 05, 2022 at 04:04:38PM -0400, Paul Gortmaker wrote:
> > The panic comes from the sanity test code, but after trying to boil down the
> > .config differences between the kitchen sink our test team uses, and a
> > "defconfig", it seems there are at least a couple extra dependencies for
> > creating a reproducer:

[...]

> > 
> >    rcu: Hierarchical SRCU implementation.
> >    Kprobe smoke test: started
> >    BUG: unable to handle page fault for address: ffffffffc110f3e7
> >    #PF: supervisor instruction fetch in kernel mode
> >    #PF: error_code(0x0010) - not-present page
> >    PGD b2c60f067 P4D b2c60f067 PUD b2c611067 PMD 0
> >    Oops: 0010 [#1] SMP NOPTI
> >    CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.15.57 #33

[...]

> Can you try the patch below?

[    2.529263] rcu: Hierarchical SRCU implementation.
[    2.530393] Kprobe smoke test: started
[    2.555965] Kprobe smoke test: passed successfully
[    2.556454] smp: Bringing up secondary CPUs ...

As per above, the same spot in the kprobe test seems to manage to not
panic anymore and the remainder of the boot looks clean and normal.

I tested directly on vanilla v5.15.57.

Thanks for the quick response!
Paul.
--

> 
> Thanks.
> Cascardo.
> 
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 74c2f88a43d0..6bb479ce1ae4 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -321,12 +321,12 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
>  	unsigned long offset;
>  	unsigned long npages;
>  	unsigned long size;
> -	unsigned long retq;
>  	unsigned long *ptr;
>  	void *trampoline;
>  	void *ip;
>  	/* 48 8b 15 <offset> is movq <offset>(%rip), %rdx */
>  	unsigned const char op_ref[] = { 0x48, 0x8b, 0x15 };
> +	unsigned const char retq[] = { RET_INSN_OPCODE, INT3_INSN_OPCODE };
>  	union ftrace_op_code_union op_ptr;
>  	int ret;
>  
> @@ -364,15 +364,10 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
>  		goto fail;
>  
>  	ip = trampoline + size;
> -
> -	/* The trampoline ends with ret(q) */
> -	retq = (unsigned long)ftrace_stub;
>  	if (cpu_feature_enabled(X86_FEATURE_RETHUNK))
>  		memcpy(ip, text_gen_insn(JMP32_INSN_OPCODE, ip, &__x86_return_thunk), JMP32_INSN_SIZE);
>  	else
> -		ret = copy_from_kernel_nofault(ip, (void *)retq, RET_SIZE);
> -	if (WARN_ON(ret < 0))
> -		goto fail;
> +		memcpy(ip, retq, sizeof(retq));
>  
>  	/* No need to test direct calls on created trampolines */
>  	if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {

Re: v5.15.57 regression - boot panic after retbleed backports with CONFIG_KPROBES_SANITY_TEST=y

[Greg Kroah-Hartman]

On Fri, Aug 05, 2022 at 08:11:00PM -0400, Paul Gortmaker wrote:
> [Re: v5.15.57 regression - boot panic after retbleed backports with CONFIG_KPROBES_SANITY_TEST=y] On 05/08/2022 (Fri 18:13) Thadeu Lima de Souza Cascardo wrote:
> 
> > On Fri, Aug 05, 2022 at 04:04:38PM -0400, Paul Gortmaker wrote:
> > > The panic comes from the sanity test code, but after trying to boil down the
> > > .config differences between the kitchen sink our test team uses, and a
> > > "defconfig", it seems there are at least a couple extra dependencies for
> > > creating a reproducer:
> 
> [...]
> 
> > > 
> > >    rcu: Hierarchical SRCU implementation.
> > >    Kprobe smoke test: started
> > >    BUG: unable to handle page fault for address: ffffffffc110f3e7
> > >    #PF: supervisor instruction fetch in kernel mode
> > >    #PF: error_code(0x0010) - not-present page
> > >    PGD b2c60f067 P4D b2c60f067 PUD b2c611067 PMD 0
> > >    Oops: 0010 [#1] SMP NOPTI
> > >    CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.15.57 #33
> 
> [...]
> 
> > Can you try the patch below?
> 
> [    2.529263] rcu: Hierarchical SRCU implementation.
> [    2.530393] Kprobe smoke test: started
> [    2.555965] Kprobe smoke test: passed successfully
> [    2.556454] smp: Bringing up secondary CPUs ...
> 
> As per above, the same spot in the kprobe test seems to manage to not
> panic anymore and the remainder of the boot looks clean and normal.
> 
> I tested directly on vanilla v5.15.57.
> 
> Thanks for the quick response!

Great, can someone send a "real" patch for this then?

thanks,

greg k-h

Re: v5.15.57 regression - boot panic after retbleed backports with CONFIG_KPROBES_SANITY_TEST=y

[Paul Gortmaker]

[Re: v5.15.57 regression - boot panic after retbleed backports with CONFIG_KPROBES_SANITY_TEST=y] On 08/08/2022 (Mon 15:48) Greg Kroah-Hartman wrote:

> On Fri, Aug 05, 2022 at 08:11:00PM -0400, Paul Gortmaker wrote:
> > [Re: v5.15.57 regression - boot panic after retbleed backports with CONFIG_KPROBES_SANITY_TEST=y] On 05/08/2022 (Fri 18:13) Thadeu Lima de Souza Cascardo wrote:

[...]

> > > Can you try the patch below?
> > 
> > [    2.529263] rcu: Hierarchical SRCU implementation.
> > [    2.530393] Kprobe smoke test: started
> > [    2.555965] Kprobe smoke test: passed successfully
> > [    2.556454] smp: Bringing up secondary CPUs ...
> > 
> > As per above, the same spot in the kprobe test seems to manage to not
> > panic anymore and the remainder of the boot looks clean and normal.
> > 
> > I tested directly on vanilla v5.15.57.
> > 
> > Thanks for the quick response!
> 
> Great, can someone send a "real" patch for this then?

Cascardo,

Was this a final patch or just a "can you test this so we can better
understand the regression"  intermediate step?  I don't think anyone
wants to guess at that or at what you wanted to put in the commit log.

It would be nice to close this out vs. leaving it hanging out there.

Thanks,
Paul.
--

> 
> thanks,
> 
> greg k-h

Re: v5.15.57 regression - boot panic after retbleed backports with CONFIG_KPROBES_SANITY_TEST=y

[Thadeu Lima de Souza Cascardo]

On Tue, Aug 16, 2022 at 12:12:24AM -0400, Paul Gortmaker wrote:
> [Re: v5.15.57 regression - boot panic after retbleed backports with CONFIG_KPROBES_SANITY_TEST=y] On 08/08/2022 (Mon 15:48) Greg Kroah-Hartman wrote:
> 
> > On Fri, Aug 05, 2022 at 08:11:00PM -0400, Paul Gortmaker wrote:
> > > [Re: v5.15.57 regression - boot panic after retbleed backports with CONFIG_KPROBES_SANITY_TEST=y] On 05/08/2022 (Fri 18:13) Thadeu Lima de Souza Cascardo wrote:
> 
> [...]
> 
> > > > Can you try the patch below?
> > > 
> > > [    2.529263] rcu: Hierarchical SRCU implementation.
> > > [    2.530393] Kprobe smoke test: started
> > > [    2.555965] Kprobe smoke test: passed successfully
> > > [    2.556454] smp: Bringing up secondary CPUs ...
> > > 
> > > As per above, the same spot in the kprobe test seems to manage to not
> > > panic anymore and the remainder of the boot looks clean and normal.
> > > 
> > > I tested directly on vanilla v5.15.57.
> > > 
> > > Thanks for the quick response!
> > 
> > Great, can someone send a "real" patch for this then?
> 
> Cascardo,
> 
> Was this a final patch or just a "can you test this so we can better
> understand the regression"  intermediate step?  I don't think anyone
> wants to guess at that or at what you wanted to put in the commit log.
> 
> It would be nice to close this out vs. leaving it hanging out there.
> 
> Thanks,
> Paul.
> --

Not a final thing, I was catching up on other stuff before I could go back to
it, was planning on doing it this week. Just need to sort out the order of some
commits here to get to that end result.

Cascardo.

> 
> > 
> > thanks,
> > 
> > greg k-h

[PATCH 1/3] Revert "x86/ftrace: Use alternative RET encoding"

[Thadeu Lima de Souza Cascardo]

This reverts commit 3af2ebf798c52b20de827b9dfec13472ab4a7964.

This temporarily reverts the backport of upstream commit
1f001e9da6bbf482311e45e48f53c2bd2179e59c. It was not correct to copy the
ftrace stub as it would contain a relative jump to the return thunk which
would not apply to the context where it was being copied to, leading to
ftrace support to be broken.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
---
 arch/x86/kernel/ftrace.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index ceab28277546..5080f578236a 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -309,7 +309,7 @@ union ftrace_op_code_union {
 	} __attribute__((packed));
 };
 
-#define RET_SIZE		(IS_ENABLED(CONFIG_RETPOLINE) ? 5 : 1 + IS_ENABLED(CONFIG_SLS))
+#define RET_SIZE		1 + IS_ENABLED(CONFIG_SLS)
 
 static unsigned long
 create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
@@ -368,10 +368,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 
 	/* The trampoline ends with ret(q) */
 	retq = (unsigned long)ftrace_stub;
-	if (cpu_feature_enabled(X86_FEATURE_RETHUNK))
-		memcpy(ip, text_gen_insn(JMP32_INSN_OPCODE, ip, &__x86_return_thunk), JMP32_INSN_SIZE);
-	else
-		ret = copy_from_kernel_nofault(ip, (void *)retq, RET_SIZE);
+	ret = copy_from_kernel_nofault(ip, (void *)retq, RET_SIZE);
 	if (WARN_ON(ret < 0))
 		goto fail;
 
-- 
2.34.1

[PATCH 2/3] x86/ibt,ftrace: Make function-graph play nice

[Thadeu Lima de Souza Cascardo]

From: Peter Zijlstra <peterz@infradead.org>

commit e52fc2cf3f662828cc0d51c4b73bed73ad275fce upstream.

Return trampoline must not use indirect branch to return; while this
preserves the RSB, it is fundamentally incompatible with IBT. Instead
use a retpoline like ROP gadget that defeats IBT while not unbalancing
the RSB.

And since ftrace_stub is no longer a plain RET, don't use it to copy
from. Since RET is a trivial instruction, poke it directly.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
Link: https://lore.kernel.org/r/20220308154318.347296408@infradead.org
[cascardo: remove ENDBR]
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
---
 arch/x86/kernel/ftrace.c    |  9 ++-------
 arch/x86/kernel/ftrace_64.S | 19 +++++++++++++++----
 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 5080f578236a..8160d1dc6ed3 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -322,12 +322,12 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 	unsigned long offset;
 	unsigned long npages;
 	unsigned long size;
-	unsigned long retq;
 	unsigned long *ptr;
 	void *trampoline;
 	void *ip;
 	/* 48 8b 15 <offset> is movq <offset>(%rip), %rdx */
 	unsigned const char op_ref[] = { 0x48, 0x8b, 0x15 };
+	unsigned const char retq[] = { RET_INSN_OPCODE, INT3_INSN_OPCODE };
 	union ftrace_op_code_union op_ptr;
 	int ret;
 
@@ -365,12 +365,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 		goto fail;
 
 	ip = trampoline + size;
-
-	/* The trampoline ends with ret(q) */
-	retq = (unsigned long)ftrace_stub;
-	ret = copy_from_kernel_nofault(ip, (void *)retq, RET_SIZE);
-	if (WARN_ON(ret < 0))
-		goto fail;
+	memcpy(ip, retq, RET_SIZE);
 
 	/* No need to test direct calls on created trampolines */
 	if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index d6af81d1b788..6cc14a835991 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -181,7 +181,6 @@ SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL)
 
 /*
  * This is weak to keep gas from relaxing the jumps.
- * It is also used to copy the RET for trampolines.
  */
 SYM_INNER_LABEL_ALIGN(ftrace_stub, SYM_L_WEAK)
 	UNWIND_HINT_FUNC
@@ -335,7 +334,7 @@ SYM_FUNC_START(ftrace_graph_caller)
 SYM_FUNC_END(ftrace_graph_caller)
 
 SYM_FUNC_START(return_to_handler)
-	subq  $24, %rsp
+	subq  $16, %rsp
 
 	/* Save the return values */
 	movq %rax, (%rsp)
@@ -347,7 +346,19 @@ SYM_FUNC_START(return_to_handler)
 	movq %rax, %rdi
 	movq 8(%rsp), %rdx
 	movq (%rsp), %rax
-	addq $24, %rsp
-	JMP_NOSPEC rdi
+
+	addq $16, %rsp
+	/*
+	 * Jump back to the old return address. This cannot be JMP_NOSPEC rdi
+	 * since IBT would demand that contain ENDBR, which simply isn't so for
+	 * return addresses. Use a retpoline here to keep the RSB balanced.
+	 */
+	ANNOTATE_INTRA_FUNCTION_CALL
+	call .Ldo_rop
+	int3
+.Ldo_rop:
+	mov %rdi, (%rsp)
+	UNWIND_HINT_FUNC
+	RET
 SYM_FUNC_END(return_to_handler)
 #endif
-- 
2.34.1

[PATCH 3/3] x86/ftrace: Use alternative RET encoding

[Thadeu Lima de Souza Cascardo]

From: Peter Zijlstra <peterz@infradead.org>

commit 1f001e9da6bbf482311e45e48f53c2bd2179e59c upstream.

Use the return thunk in ftrace trampolines, if needed.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Josh Poimboeuf <jpoimboe@kernel.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
[cascardo: use memcpy(text_gen_insn) as there is no __text_gen_insn]
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
---
 arch/x86/kernel/ftrace.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 8160d1dc6ed3..b3c9ef01d6c0 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -309,7 +309,7 @@ union ftrace_op_code_union {
 	} __attribute__((packed));
 };
 
-#define RET_SIZE		1 + IS_ENABLED(CONFIG_SLS)
+#define RET_SIZE		(IS_ENABLED(CONFIG_RETPOLINE) ? 5 : 1 + IS_ENABLED(CONFIG_SLS))
 
 static unsigned long
 create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
@@ -365,7 +365,12 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 		goto fail;
 
 	ip = trampoline + size;
-	memcpy(ip, retq, RET_SIZE);
+
+	/* The trampoline ends with ret(q) */
+	if (cpu_feature_enabled(X86_FEATURE_RETHUNK))
+		memcpy(ip, text_gen_insn(JMP32_INSN_OPCODE, ip, &__x86_return_thunk), JMP32_INSN_SIZE);
+	else
+		memcpy(ip, retq, sizeof(retq));
 
 	/* No need to test direct calls on created trampolines */
 	if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
-- 
2.34.1

Re: [PATCH 1/3] Revert "x86/ftrace: Use alternative RET encoding"

[Greg KH]

On Tue, Aug 16, 2022 at 05:26:56AM -0300, Thadeu Lima de Souza Cascardo wrote:
> This reverts commit 3af2ebf798c52b20de827b9dfec13472ab4a7964.
> 
> This temporarily reverts the backport of upstream commit
> 1f001e9da6bbf482311e45e48f53c2bd2179e59c. It was not correct to copy the
> ftrace stub as it would contain a relative jump to the return thunk which
> would not apply to the context where it was being copied to, leading to
> ftrace support to be broken.
> 
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> ---
>  arch/x86/kernel/ftrace.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)

What stable tree(s) is this to be for?

Context is everything :)

thanks,

greg k-h

Re: [PATCH 1/3] Revert "x86/ftrace: Use alternative RET encoding"

[Thadeu Lima de Souza Cascardo]

On Tue, Aug 16, 2022 at 11:18:52AM +0200, Greg KH wrote:
> On Tue, Aug 16, 2022 at 05:26:56AM -0300, Thadeu Lima de Souza Cascardo wrote:
> > This reverts commit 3af2ebf798c52b20de827b9dfec13472ab4a7964.
> > 
> > This temporarily reverts the backport of upstream commit
> > 1f001e9da6bbf482311e45e48f53c2bd2179e59c. It was not correct to copy the
> > ftrace stub as it would contain a relative jump to the return thunk which
> > would not apply to the context where it was being copied to, leading to
> > ftrace support to be broken.
> > 
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > ---
> >  arch/x86/kernel/ftrace.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> What stable tree(s) is this to be for?
> 
> Context is everything :)
> 
> thanks,
> 
> greg k-h

Yeah, I was just thinking that I missed it, and whether I would be able to
respond to my own message before you did.  :-)

This series is targed at 5.15. I will look at 5.10 next.

Thanks.
Cascardo.

Re: [PATCH 1/3] Revert "x86/ftrace: Use alternative RET encoding"

[Greg KH]

On Tue, Aug 16, 2022 at 07:16:25AM -0300, Thadeu Lima de Souza Cascardo wrote:
> On Tue, Aug 16, 2022 at 11:18:52AM +0200, Greg KH wrote:
> > On Tue, Aug 16, 2022 at 05:26:56AM -0300, Thadeu Lima de Souza Cascardo wrote:
> > > This reverts commit 3af2ebf798c52b20de827b9dfec13472ab4a7964.
> > > 
> > > This temporarily reverts the backport of upstream commit
> > > 1f001e9da6bbf482311e45e48f53c2bd2179e59c. It was not correct to copy the
> > > ftrace stub as it would contain a relative jump to the return thunk which
> > > would not apply to the context where it was being copied to, leading to
> > > ftrace support to be broken.
> > > 
> > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > > ---
> > >  arch/x86/kernel/ftrace.c | 7 ++-----
> > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > What stable tree(s) is this to be for?
> > 
> > Context is everything :)
> > 
> > thanks,
> > 
> > greg k-h
> 
> Yeah, I was just thinking that I missed it, and whether I would be able to
> respond to my own message before you did.  :-)
> 
> This series is targed at 5.15. I will look at 5.10 next.

Thanks, I'll queue this up after the current 5.15-rc is released later
this week.

greg k-h

Re: v5.15.57 regression - boot panic after retbleed backports with CONFIG_KPROBES_SANITY_TEST=y

[Paul Gortmaker]

[Re: v5.15.57 regression - boot panic after retbleed backports with CONFIG_KPROBES_SANITY_TEST=y] On 16/08/2022 (Tue 04:29) Thadeu Lima de Souza Cascardo wrote:

> On Tue, Aug 16, 2022 at 12:12:24AM -0400, Paul Gortmaker wrote:

> > Cascardo,
> > 
> > Was this a final patch or just a "can you test this so we can better
> > understand the regression"  intermediate step?  I don't think anyone
> > wants to guess at that or at what you wanted to put in the commit log.
> > 
> > It would be nice to close this out vs. leaving it hanging out there.
> > 
> > Thanks,
> > Paul.
> > --
> 
> Not a final thing, I was catching up on other stuff before I could go back to
> it, was planning on doing it this week. Just need to sort out the order of some
> commits here to get to that end result.

Great - I didn't want to nag, but some new ftrace/kprobe regressions
were reported on a Yocto variant of v5.15 and I immediately thought of
this being the underlying cause and the breakage being more than just
the CONFIG_KPROBES_SANITY_TEST corner case I originally reported.

The good news is that I've tested with your new three pack and that
fixes both new issues.  I'll loop back and re-check the original issue
as well for completeness, but if you don't hear from me again, you can
assume that went well too.  So, if Greg wants to, feel free to add: 

Tested-by: Paul Gortmaker <paul.gortmaker@windriver.com>

Thanks again for fixing this.  Much appreciated.
Paul.
--

> 
> Cascardo.
> 
> > 
> > > 
> > > thanks,
> > > 
> > > greg k-h

Re: [PATCH 1/3] Revert "x86/ftrace: Use alternative RET encoding"

[Greg KH]

On Tue, Aug 16, 2022 at 05:26:56AM -0300, Thadeu Lima de Souza Cascardo wrote:
> This reverts commit 3af2ebf798c52b20de827b9dfec13472ab4a7964.

There is no commit with this id in the tree.  I think you mean e54fcb0812faebd147de72bd37ad87cc4951c68c

thanks,

greg k-h