Skip to contentSkip to conversation

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

PATCH In review Paul Gortmaker 3 participants 14 messages Aug 5, 2022 – Aug 19, 2022

Subsystems (3)
Stable branch x86 ftrace
lore
Share & tools
lore

Paul Gortmaker

author
#0 lore raw

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 <[email protected]>
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 ]---
#1 lore raw
Quoted 60 lines · 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 <[email protected]>
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
Quoted 8 lines
    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.

arch/x86/kernel/ftrace.c +2 -7
index 74c2f88a43d0..6bb479ce1ae4 100644
@@ -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) {
author
#2 lore raw
Quoted 5 lines · [Re: v5.15.57 regression - boot panic after retbleed backports with…
Quoted 4 lines · 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:

[...]

Quoted 9 lines
Quoted 9 lines

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

[...]

Quoted 1 line

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.
--

Quoted 39 lines

Thanks.
Cascardo.

arch/x86/kernel/ftrace.c +2 -7
index 74c2f88a43d0..6bb479ce1ae4 100644
@@ -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) {
maintainer
#3 lore raw
Quoted 29 lines · Paul Gortmaker
Quoted 5 lines · [Re: v5.15.57 regression - boot panic after retbleed backports with…
Quoted 4 lines · 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:

[...]

Quoted 9 lines
Quoted 9 lines

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

[...]

Quoted 1 line

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 ...
Quoted 6 lines

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

author
#4 lore raw
Quoted 2 lines · [Re: v5.15.57 regression - boot panic after retbleed backports with…
Quoted 1 line · 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:

[...]

Quoted 7 lines
Quoted 7 lines
Quoted 1 line

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 ...
Quoted 8 lines
Quoted 6 lines

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.
--

Quoted 4 lines

thanks,

greg k-h

#5 lore raw
Quoted 29 lines · Paul Gortmaker
Quoted 2 lines · [Re: v5.15.57 regression - boot panic after retbleed backports with…
Quoted 1 line · 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:

[...]

Quoted 7 lines
Quoted 7 lines
Quoted 1 line

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 ...
Quoted 8 lines
Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 5 lines
Quoted 5 lines

thanks,

greg k-h

author Tested-by
#6 lore raw
Quoted 10 lines · [Re: v5.15.57 regression - boot panic after retbleed backports with…
Quoted 8 lines · Paul Gortmaker

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.

Quoted 7 lines
Quoted 3 lines

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 <[email protected]>

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

Quoted 8 lines

Cascardo.

Quoted 5 lines
Quoted 5 lines

thanks,

greg k-h

#7 lore raw

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

In reply to Paul Gortmaker

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 <[email protected]>
---
 arch/x86/kernel/ftrace.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
arch/x86/kernel/ftrace.c +2 -5
index ceab28277546..5080f578236a 100644
@@ -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;
Signature · 1 line
2.34.1
maintainer
#8 lore raw

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

Quoted 8 lines · 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.

Quoted 4 lines
Signed-off-by Thadeu Lima de Souza Cascardo <[email protected]>
---
 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

#9 lore raw

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

Quoted 14 lines · Greg KH
Quoted 8 lines · 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.

Quoted 4 lines
Signed-off-by Thadeu Lima de Souza Cascardo <[email protected]>
---
 arch/x86/kernel/ftrace.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
Quoted 7 lines

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.

maintainer
#10 lore raw

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

Quoted 19 lines · Thadeu Lima de Souza Cascardo
Quoted 18 lines · Greg KH
Quoted 8 lines · 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.

Quoted 4 lines
Signed-off-by Thadeu Lima de Souza Cascardo <[email protected]>
---
 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 :)

Quoted 8 lines
Quoted 3 lines

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

#11 lore raw

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

In reply to Thadeu Lima de Souza Cascardo

From: Peter Zijlstra <[email protected]>

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) <[email protected]>
Acked-by Josh Poimboeuf <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
[cascardo: remove ENDBR]
Signed-off-by Thadeu Lima de Souza Cascardo <[email protected]>
---
 arch/x86/kernel/ftrace.c    |  9 ++-------
 arch/x86/kernel/ftrace_64.S | 19 +++++++++++++++----
 2 files changed, 17 insertions(+), 11 deletions(-)
arch/x86/kernel/ftrace.c +2 -7
index 5080f578236a..8160d1dc6ed3 100644
@@ -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) {
arch/x86/kernel/ftrace_64.S +15 -4
index d6af81d1b788..6cc14a835991 100644
@@ -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
Signature · 1 line
2.34.1
#12 lore raw

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

In reply to Thadeu Lima de Souza Cascardo

From: Peter Zijlstra <[email protected]>

commit 1f001e9da6bbf482311e45e48f53c2bd2179e59c upstream.

Use the return thunk in ftrace trampolines, if needed.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
Reviewed-by Josh Poimboeuf <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
[cascardo: use memcpy(text_gen_insn) as there is no __text_gen_insn]
Signed-off-by Thadeu Lima de Souza Cascardo <[email protected]>
---
 arch/x86/kernel/ftrace.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
arch/x86/kernel/ftrace.c +7 -2
index 8160d1dc6ed3..b3c9ef01d6c0 100644
@@ -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) {
Signature · 1 line
2.34.1
maintainer
#13 lore raw

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

In reply to Thadeu Lima de Souza Cascardo
Quoted 1 line · Thadeu Lima de Souza Cascardo

This reverts commit 3af2ebf798c52b20de827b9dfec13472ab4a7964.

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

thanks,

greg k-h