This reverts commit e92626af3234708fe30f53b269d210d202b95206.
This commit broke patching of the return thunk jmp in the retpoline
sequence.
Before (broken sequence):
objdump -d -r arch/x86/lib/retpoline.o:
0000000000000000 <__x86_indirect_thunk_array>:
...
a: e9 d1 02 00 00 jmpq 2e0 <__x86_return_thunk>
live disassembly at runtime:
0xffffffff81d12a8a <+10>: jmpq 0xffffffff81d12d60
<__x86_return_thunk>
This jmp to the default return thunk should not happen after alternatives
patching.
After reverting this:
objdump -d -r arch/x86/lib/retpoline.o:
0000000000000000 <__x86_indirect_thunk_array>:
...
a: e9 00 00 00 00 jmpq f <__x86_indirect_thunk_array+0xf>
b: R_X86_64_PLT32 __x86_return_thunk-0x4
live disassembly at runtime:
0xffffffff81d12a8a <+10>: jmpq 0xffffffff81f0410b
<srso_alias_return_thunk>
This is correct as the jmp is written to the correct return sequence.
objtool (add_jump_destinations()) only recognizes return thunk jmps that have
relocation entries, which will not occur if the return thunk is in the
same section as the indirect thunks.
Signed-off-by: David Kaplan <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
---
arch/x86/kernel/vmlinux.lds.S | 3 +++
arch/x86/lib/retpoline.S | 2 ++
2 files changed, 5 insertions(+)
diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index 9cdb1a7332c4..54a5596adaa6 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -132,7 +132,10 @@ SECTIONS
LOCK_TEXT
KPROBES_TEXT
SOFTIRQENTRY_TEXT
+#ifdef CONFIG_RETPOLINE
*(.text..__x86.indirect_thunk)
+ *(.text..__x86.return_thunk)
+#endif
STATIC_CALL_TEXT
ALIGN_ENTRY_TEXT_BEGIN
diff --git a/arch/x86/lib/retpoline.S b/arch/x86/lib/retpoline.S
index db813113e637..3da768a71cf9 100644
--- a/arch/x86/lib/retpoline.S
+++ b/arch/x86/lib/retpoline.S
@@ -129,6 +129,8 @@ SYM_CODE_END(__x86_indirect_jump_thunk_array)
#ifdef CONFIG_RETHUNK
+ .section .text..__x86.return_thunk
+
#ifdef CONFIG_CPU_SRSO
/*
--
2.25.1
On Tue, Oct 10, 2023 at 12:10:18PM -0500, David Kaplan wrote:
> arch/x86/kernel/vmlinux.lds.S | 3 +++
> arch/x86/lib/retpoline.S | 2 ++
> 2 files changed, 5 insertions(+)
>
> diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
> index 9cdb1a7332c4..54a5596adaa6 100644
> --- a/arch/x86/kernel/vmlinux.lds.S
> +++ b/arch/x86/kernel/vmlinux.lds.S
> @@ -132,7 +132,10 @@ SECTIONS
> LOCK_TEXT
> KPROBES_TEXT
> SOFTIRQENTRY_TEXT
> +#ifdef CONFIG_RETPOLINE
> *(.text..__x86.indirect_thunk)
> + *(.text..__x86.return_thunk)
> +#endif
> STATIC_CALL_TEXT
>
> ALIGN_ENTRY_TEXT_BEGIN
> diff --git a/arch/x86/lib/retpoline.S b/arch/x86/lib/retpoline.S
> index db813113e637..3da768a71cf9 100644
> --- a/arch/x86/lib/retpoline.S
> +++ b/arch/x86/lib/retpoline.S
> @@ -129,6 +129,8 @@ SYM_CODE_END(__x86_indirect_jump_thunk_array)
>
> #ifdef CONFIG_RETHUNK
Perhaps elucidate the future reader with a comment here? Lest someone
tries removing it again.
>
> + .section .text..__x86.return_thunk
> +
> #ifdef CONFIG_CPU_SRSO
On Tue, Oct 10, 2023 at 07:48:33PM +0200, Peter Zijlstra wrote:
> On Tue, Oct 10, 2023 at 12:10:18PM -0500, David Kaplan wrote:
>
> > arch/x86/kernel/vmlinux.lds.S | 3 +++
> > arch/x86/lib/retpoline.S | 2 ++
> > 2 files changed, 5 insertions(+)
> >
> > diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
> > index 9cdb1a7332c4..54a5596adaa6 100644
> > --- a/arch/x86/kernel/vmlinux.lds.S
> > +++ b/arch/x86/kernel/vmlinux.lds.S
> > @@ -132,7 +132,10 @@ SECTIONS
> > LOCK_TEXT
> > KPROBES_TEXT
> > SOFTIRQENTRY_TEXT
> > +#ifdef CONFIG_RETPOLINE
> > *(.text..__x86.indirect_thunk)
> > + *(.text..__x86.return_thunk)
> > +#endif
> > STATIC_CALL_TEXT
> >
> > ALIGN_ENTRY_TEXT_BEGIN
> > diff --git a/arch/x86/lib/retpoline.S b/arch/x86/lib/retpoline.S
> > index db813113e637..3da768a71cf9 100644
> > --- a/arch/x86/lib/retpoline.S
> > +++ b/arch/x86/lib/retpoline.S
> > @@ -129,6 +129,8 @@ SYM_CODE_END(__x86_indirect_jump_thunk_array)
> >
> > #ifdef CONFIG_RETHUNK
>
> Perhaps elucidate the future reader with a comment here? Lest someone
> tries removing it again.
Yes, please.
Also we could make objtool properly detect the non-relocated jump
target. But this works for now.
--
Josh
On Tue, Oct 10, 2023 at 12:57:21PM -0700, Josh Poimboeuf wrote:
> Also we could make objtool properly detect the non-relocated jump
> target.
I was wondering about that... I guess it can compute the JMP target and
compare it to the address of __x86_return_thunk?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Oct 10, 2023 at 10:04:29PM +0200, Borislav Petkov wrote:
> On Tue, Oct 10, 2023 at 12:57:21PM -0700, Josh Poimboeuf wrote:
> > Also we could make objtool properly detect the non-relocated jump
> > target.
>
> I was wondering about that... I guess it can compute the JMP target and
> compare it to the address of __x86_return_thunk?
Fine, you twisted my arm ;-)
This seems to do the trick. Lemme write up a proper patch.
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e308d1ba664e..6cbc9812a36e 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1610,6 +1610,11 @@ static int add_jump_destinations(struct objtool_file *file)
return -1;
}
+ if (jump_dest->sym && jump_dest->sym->return_thunk) {
+ add_return_call(file, insn, true);
+ continue;
+ }
+
/*
* Cross-function jump.
*/
[AMD Official Use Only - General]
> -----Original Message-----
> From: Josh Poimboeuf <[email protected]>
> Sent: Tuesday, October 10, 2023 3:19 PM
> To: Borislav Petkov <[email protected]>
> Cc: Peter Zijlstra <[email protected]>; Kaplan, David
> <[email protected]>; [email protected]; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH 1/3] Revert "x86/retpoline: Remove
> .text..__x86.return_thunk section"
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Tue, Oct 10, 2023 at 10:04:29PM +0200, Borislav Petkov wrote:
> > On Tue, Oct 10, 2023 at 12:57:21PM -0700, Josh Poimboeuf wrote:
> > > Also we could make objtool properly detect the non-relocated jump
> > > target.
> >
> > I was wondering about that... I guess it can compute the JMP target
> > and compare it to the address of __x86_return_thunk?
>
> Fine, you twisted my arm ;-)
>
> This seems to do the trick. Lemme write up a proper patch.
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c index
> e308d1ba664e..6cbc9812a36e 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -1610,6 +1610,11 @@ static int add_jump_destinations(struct
> objtool_file *file)
> return -1;
> }
>
> + if (jump_dest->sym && jump_dest->sym->return_thunk) {
> + add_return_call(file, insn, true);
> + continue;
> + }
> +
> /*
> * Cross-function jump.
> */
This looks good to me in my testing so far.
--David Kaplan
On Tue, Oct 10, 2023 at 01:19:12PM -0700, Josh Poimboeuf wrote:
> On Tue, Oct 10, 2023 at 10:04:29PM +0200, Borislav Petkov wrote:
> > On Tue, Oct 10, 2023 at 12:57:21PM -0700, Josh Poimboeuf wrote:
> > > Also we could make objtool properly detect the non-relocated jump
> > > target.
> >
> > I was wondering about that... I guess it can compute the JMP target and
> > compare it to the address of __x86_return_thunk?
>
> Fine, you twisted my arm ;-)
>
> This seems to do the trick. Lemme write up a proper patch.
Here is said patch.
---8<---
From: Josh Poimboeuf <[email protected]>
Subject: [PATCH] objtool: Fix return thunk patching in retpolines
With CONFIG_RETHUNK enabled, the compiler replaces every RET with a tail
call to a return thunk ('JMP __x86_return_thunk'). Objtool annotates
all such return sites so they can be patched during boot by
apply_returns().
The implementation of __x86_return_thunk() is just a bare RET. It's
only meant to be used temporarily until apply_returns() patches all
return sites with either a JMP to another return thunk or an actual RET.
The following commit
e92626af3234 ("x86/retpoline: Remove .text..__x86.return_thunk section") retpolines
broke objtool's detection of return sites in retpolines. Since
retpolines and return thunks are now in the same section, the compiler
no longer uses relocations for the intra-section jumps between the
retpolines and the return thunk, causing objtool to overlook them.
As a result, none of the retpolines' return sites get patched. Each one
stays at 'JMP __x86_return_thunk', effectively a bare RET.
Fix it by teaching objtool to detect when a non-relocated jump target is
a return thunk.
Fixes: e92626af3234 ("x86/retpoline: Remove .text..__x86.return_thunk section")
Reported-by: David Kaplan <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
tools/objtool/check.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e308d1ba664e..556469db4239 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1610,6 +1610,15 @@ static int add_jump_destinations(struct objtool_file *file)
return -1;
}
+ /*
+ * Since retpolines are in the same section as the return
+ * thunk, they might not use a relocation when branching to it.
+ */
+ if (jump_dest->sym && jump_dest->sym->return_thunk) {
+ add_return_call(file, insn, true);
+ continue;
+ }
+
/*
* Cross-function jump.
*/
--
2.41.0
On Tue, Oct 10, 2023 at 02:22:54PM -0700, Josh Poimboeuf wrote:
> From: Josh Poimboeuf <[email protected]>
> Subject: [PATCH] objtool: Fix return thunk patching in retpolines
>
> With CONFIG_RETHUNK enabled, the compiler replaces every RET with a tail
> call to a return thunk ('JMP __x86_return_thunk'). Objtool annotates
> all such return sites so they can be patched during boot by
> apply_returns().
>
> The implementation of __x86_return_thunk() is just a bare RET. It's
> only meant to be used temporarily until apply_returns() patches all
> return sites with either a JMP to another return thunk or an actual RET.
>
> The following commit
>
> e92626af3234 ("x86/retpoline: Remove .text..__x86.return_thunk section") retpolines
>
> broke objtool's detection of return sites in retpolines. Since
> retpolines and return thunks are now in the same section, the compiler
> no longer uses relocations for the intra-section jumps between the
> retpolines and the return thunk, causing objtool to overlook them.
>
> As a result, none of the retpolines' return sites get patched. Each one
> stays at 'JMP __x86_return_thunk', effectively a bare RET.
>
> Fix it by teaching objtool to detect when a non-relocated jump target is
> a return thunk.
>
> Fixes: e92626af3234 ("x86/retpoline: Remove .text..__x86.return_thunk section")
> Reported-by: David Kaplan <[email protected]>
> Signed-off-by: Josh Poimboeuf <[email protected]>
> ---
> tools/objtool/check.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index e308d1ba664e..556469db4239 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -1610,6 +1610,15 @@ static int add_jump_destinations(struct objtool_file *file)
> return -1;
> }
>
> + /*
> + * Since retpolines are in the same section as the return
> + * thunk, they might not use a relocation when branching to it.
> + */
> + if (jump_dest->sym && jump_dest->sym->return_thunk) {
> + add_return_call(file, insn, true);
> + continue;
> + }
*urgh*... I mean, yes, that obviously works, but should we not also have
the retpoline thingy for consistency? That case makes less sense though
:/
Perhaps warn about this instead of fixing it? Forcing people to play the
section game?
I dunno.. no real strong opinions.
On Wed, Oct 11, 2023 at 09:41:42AM +0200, Peter Zijlstra wrote:
> *urgh*... I mean, yes, that obviously works, but should we not also have
> the retpoline thingy for consistency? That case makes less sense though
> :/
>
> Perhaps warn about this instead of fixing it? Forcing people to play the
> section game?
I like the conservative aspect of that: keep the separate sections and
warn if the return thunk is in the same section.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Oct 11, 2023 at 09:41:42AM +0200, Peter Zijlstra wrote:
> > +++ b/tools/objtool/check.c
> > @@ -1610,6 +1610,15 @@ static int add_jump_destinations(struct objtool_file *file)
> > return -1;
> > }
> >
> > + /*
> > + * Since retpolines are in the same section as the return
> > + * thunk, they might not use a relocation when branching to it.
> > + */
> > + if (jump_dest->sym && jump_dest->sym->return_thunk) {
> > + add_return_call(file, insn, true);
> > + continue;
> > + }
>
> *urgh*... I mean, yes, that obviously works, but should we not also have
> the retpoline thingy for consistency? That case makes less sense though
> :/
Consistency with what? The extra section seems pointless but maybe I'm
missing something.
--
Josh
On Wed, Oct 11, 2023 at 09:28:43AM -0700, Josh Poimboeuf wrote:
> On Wed, Oct 11, 2023 at 09:41:42AM +0200, Peter Zijlstra wrote:
> > > +++ b/tools/objtool/check.c
> > > @@ -1610,6 +1610,15 @@ static int add_jump_destinations(struct objtool_file *file)
> > > return -1;
> > > }
> > >
> > > + /*
> > > + * Since retpolines are in the same section as the return
> > > + * thunk, they might not use a relocation when branching to it.
> > > + */
> > > + if (jump_dest->sym && jump_dest->sym->return_thunk) {
> > > + add_return_call(file, insn, true);
> > > + continue;
> > > + }
> >
> > *urgh*... I mean, yes, that obviously works, but should we not also have
> > the retpoline thingy for consistency? That case makes less sense though
> > :/
>
> Consistency with what?
the reloc case; specifically, I was thinking something along these
lines:
if (jump-dest->sym && jump_dest->sym->retpoline_thunk) {
add_retpoline_call(file, insn);
continue;
}
Then both reloc and immediate versions are more or less the same.
> The extra section seems pointless but maybe I'm missing something.
By having the section things are better delineated I suppose, be it
retpolines or rethunks, all references should be to inside the section
(and thus have a reloc) while within the section there should never be a
reference to itself.
I'm not sure it's worth much, but then we can have the above two cases
issue a WARN instead of fixing it up.
I don't care too deeply, I can't make up my mind either way. But perhaps
keeping the section is easier on all the backports, it's easy to forget
a tiny objtool patch like this.
* Peter Zijlstra <[email protected]> wrote:
> I don't care too deeply, I can't make up my mind either way. But perhaps
> keeping the section is easier on all the backports, it's easy to forget a
> tiny objtool patch like this.
If the objtool fix has a Fixes tag that points to one of the major
mitigation commits, then it won't be forgotten.
Arguably the new objtool is more robust against what could happen, so that
patch is not going away - and with that patch mainline doesn't have to keep
the (now ...) pointless section.
Maybe change the commit order around: first add the objtool fix, then
remove the section, pointing back to the objtool SHA1 in the very next
commit, explaining that the objtool fix enables this change. That makes it
all backporting-proof as well.
Thanks,
Ingo
On Thu, Oct 12, 2023 at 12:35:13AM +0200, Peter Zijlstra wrote:
> On Wed, Oct 11, 2023 at 09:28:43AM -0700, Josh Poimboeuf wrote:
> > On Wed, Oct 11, 2023 at 09:41:42AM +0200, Peter Zijlstra wrote:
> > > > +++ b/tools/objtool/check.c
> > > > @@ -1610,6 +1610,15 @@ static int add_jump_destinations(struct objtool_file *file)
> > > > return -1;
> > > > }
> > > >
> > > > + /*
> > > > + * Since retpolines are in the same section as the return
> > > > + * thunk, they might not use a relocation when branching to it.
> > > > + */
> > > > + if (jump_dest->sym && jump_dest->sym->return_thunk) {
> > > > + add_return_call(file, insn, true);
> > > > + continue;
> > > > + }
> > >
> > > *urgh*... I mean, yes, that obviously works, but should we not also have
> > > the retpoline thingy for consistency? That case makes less sense though
> > > :/
> >
> > Consistency with what?
>
> the reloc case; specifically, I was thinking something along these
> lines:
>
> if (jump-dest->sym && jump_dest->sym->retpoline_thunk) {
> add_retpoline_call(file, insn);
> continue;
> }
>
> Then both reloc and immediate versions are more or less the same.
Ok, I see. If somebody were to do a {JMP,CALL}_NOSPEC somewhere in
retpoline.S, it would break similarly.
It doesn't hurt to add that to the patch, that way retpoline/rethunk
site detection is all handled the same.
> > The extra section seems pointless but maybe I'm missing something.
>
> By having the section things are better delineated I suppose, be it
> retpolines or rethunks, all references should be to inside the section
> (and thus have a reloc) while within the section there should never be
> a reference to itself.
As far as delineating things goes, a good argument could be made to
instead do that on the source code level. i.e., put the rethunk code in
rethunk.S rather than retpoline.S. Incidentally, that would also fix
this problem.
From an object code standpoint, objtool is the only one who cares about
the relocs. It's a good idea to make objtool more robust against
non-relocs regardless, as the reloc assumption could always be broken
later by LTO.
BTW, I wonder if we can also get rid of .text..__x86.indirect_thunk and
just put most of the retpoline/rethunk code in .text (other than than
the SRSO aliasing hacks which still need special placement).
--
Josh
With CONFIG_RETHUNK enabled, the compiler replaces every RET with a tail
call to a return thunk ('JMP __x86_return_thunk'). Objtool annotates
all such return sites so they can be patched during boot by
apply_returns().
The implementation of __x86_return_thunk() is just a bare RET. It's
only meant to be used temporarily until apply_returns() patches all
return sites with either a JMP to another return thunk or an actual RET.
The following commit
e92626af3234 ("x86/retpoline: Remove .text..__x86.return_thunk section") retpolines
broke objtool's detection of return sites in retpolines. Since
retpolines and return thunks are now in the same section, the compiler
no longer uses relocations for the intra-section jumps between the
retpolines and the return thunk, causing objtool to overlook them.
As a result, none of the retpolines' return sites get patched. Each one
stays at 'JMP __x86_return_thunk', effectively a bare RET.
Fix it by teaching objtool to detect when a non-relocated jump target is
a return thunk (or retpoline).
Fixes: e92626af3234 ("x86/retpoline: Remove .text..__x86.return_thunk section")
Reported-by: David Kaplan <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
tools/objtool/check.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e308d1ba664e..e94756e09ca9 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1610,6 +1610,22 @@ static int add_jump_destinations(struct objtool_file *file)
return -1;
}
+ /*
+ * An intra-TU jump in retpoline.o might not have a relocation
+ * for its jump dest, in which case the above
+ * add_{retpoline,return}_call() didn't happen.
+ */
+ if (jump_dest->sym && jump_dest->offset == jump_dest->sym->offset) {
+ if (jump_dest->sym->retpoline_thunk) {
+ add_retpoline_call(file, insn);
+ continue;
+ }
+ if (jump_dest->sym->return_thunk) {
+ add_return_call(file, insn, true);
+ continue;
+ }
+ }
+
/*
* Cross-function jump.
*/
--
2.41.0
The following commit has been merged into the x86/bugs branch of tip:
Commit-ID: 3ab1bb69862d4477e2ffa556075a251bd7328910
Gitweb: https://git.kernel.org/tip/3ab1bb69862d4477e2ffa556075a251bd7328910
Author: Josh Poimboeuf <[email protected]>
AuthorDate: Wed, 11 Oct 2023 19:47:37 -07:00
Committer: Ingo Molnar <[email protected]>
CommitterDate: Thu, 12 Oct 2023 08:21:29 +02:00
objtool: Fix return thunk patching in retpolines
With CONFIG_RETHUNK enabled, the compiler replaces every RET with a tail
call to a return thunk ('JMP __x86_return_thunk'). Objtool annotates
all such return sites so they can be patched during boot by
apply_returns().
The implementation of __x86_return_thunk() is just a bare RET. It's
only meant to be used temporarily until apply_returns() patches all
return sites with either a JMP to another return thunk or an actual RET.
The following commit:
e92626af3234 ("x86/retpoline: Remove .text..__x86.return_thunk section")
broke objtool's detection of return sites in retpolines. Since
retpolines and return thunks are now in the same section, the compiler
no longer uses relocations for the intra-section jumps between the
retpolines and the return thunk, causing objtool to overlook them.
As a result, none of the retpolines' return sites get patched. Each one
stays at 'JMP __x86_return_thunk', effectively a bare RET.
Fix it by teaching objtool to detect when a non-relocated jump target is
a return thunk (or retpoline).
Fixes: e92626af3234 ("x86/retpoline: Remove .text..__x86.return_thunk section")
Reported-by: David Kaplan <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Link: https://lore.kernel.org/r/20231012024737.eg5phclogp67ik6x@treble
---
tools/objtool/check.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e308d1b..e94756e 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1611,6 +1611,22 @@ static int add_jump_destinations(struct objtool_file *file)
}
/*
+ * An intra-TU jump in retpoline.o might not have a relocation
+ * for its jump dest, in which case the above
+ * add_{retpoline,return}_call() didn't happen.
+ */
+ if (jump_dest->sym && jump_dest->offset == jump_dest->sym->offset) {
+ if (jump_dest->sym->retpoline_thunk) {
+ add_retpoline_call(file, insn);
+ continue;
+ }
+ if (jump_dest->sym->return_thunk) {
+ add_return_call(file, insn, true);
+ continue;
+ }
+ }
+
+ /*
* Cross-function jump.
*/
if (insn_func(insn) && insn_func(jump_dest) &&
On Wed, Oct 11, 2023 at 07:27:58PM -0700, Josh Poimboeuf wrote:
> From an object code standpoint, objtool is the only one who cares about
> the relocs. It's a good idea to make objtool more robust against
> non-relocs regardless, as the reloc assumption could always be broken
> later by LTO.
AFAIK LTO runs before the actual link stage and doesn't resolve
inter-section references, but yeah, that might just be an implementation
detail.
Fair enough.
On Wed, Oct 11, 2023 at 07:47:37PM -0700, Josh Poimboeuf wrote:
> With CONFIG_RETHUNK enabled, the compiler replaces every RET with a tail
> call to a return thunk ('JMP __x86_return_thunk'). Objtool annotates
> all such return sites so they can be patched during boot by
> apply_returns().
>
> The implementation of __x86_return_thunk() is just a bare RET. It's
> only meant to be used temporarily until apply_returns() patches all
> return sites with either a JMP to another return thunk or an actual RET.
>
> The following commit
>
> e92626af3234 ("x86/retpoline: Remove .text..__x86.return_thunk section") retpolines
>
> broke objtool's detection of return sites in retpolines. Since
> retpolines and return thunks are now in the same section, the compiler
> no longer uses relocations for the intra-section jumps between the
> retpolines and the return thunk, causing objtool to overlook them.
>
> As a result, none of the retpolines' return sites get patched. Each one
> stays at 'JMP __x86_return_thunk', effectively a bare RET.
>
> Fix it by teaching objtool to detect when a non-relocated jump target is
> a return thunk (or retpoline).
>
> Fixes: e92626af3234 ("x86/retpoline: Remove .text..__x86.return_thunk section")
> Reported-by: David Kaplan <[email protected]>
> Signed-off-by: Josh Poimboeuf <[email protected]>
Acked-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> tools/objtool/check.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index e308d1ba664e..e94756e09ca9 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -1610,6 +1610,22 @@ static int add_jump_destinations(struct objtool_file *file)
> return -1;
> }
>
> + /*
> + * An intra-TU jump in retpoline.o might not have a relocation
> + * for its jump dest, in which case the above
> + * add_{retpoline,return}_call() didn't happen.
> + */
> + if (jump_dest->sym && jump_dest->offset == jump_dest->sym->offset) {
> + if (jump_dest->sym->retpoline_thunk) {
> + add_retpoline_call(file, insn);
> + continue;
> + }
> + if (jump_dest->sym->return_thunk) {
> + add_return_call(file, insn, true);
> + continue;
> + }
> + }
> +
> /*
> * Cross-function jump.
> */
> --
> 2.41.0
>
The following commit has been merged into the x86/bugs branch of tip:
Commit-ID: eadbe6606a85610c63e6cfaa9257dc7e3bbb901c
Gitweb: https://git.kernel.org/tip/eadbe6606a85610c63e6cfaa9257dc7e3bbb901c
Author: Josh Poimboeuf <[email protected]>
AuthorDate: Wed, 11 Oct 2023 19:47:37 -07:00
Committer: Ingo Molnar <[email protected]>
CommitterDate: Thu, 12 Oct 2023 19:43:51 +02:00
objtool: Fix return thunk patching in retpolines
With CONFIG_RETHUNK enabled, the compiler replaces every RET with a tail
call to a return thunk ('JMP __x86_return_thunk'). Objtool annotates
all such return sites so they can be patched during boot by
apply_returns().
The implementation of __x86_return_thunk() is just a bare RET. It's
only meant to be used temporarily until apply_returns() patches all
return sites with either a JMP to another return thunk or an actual RET.
The following commit:
e92626af3234 ("x86/retpoline: Remove .text..__x86.return_thunk section")
broke objtool's detection of return sites in retpolines. Since
retpolines and return thunks are now in the same section, the compiler
no longer uses relocations for the intra-section jumps between the
retpolines and the return thunk, causing objtool to overlook them.
As a result, none of the retpolines' return sites get patched. Each one
stays at 'JMP __x86_return_thunk', effectively a bare RET.
Fix it by teaching objtool to detect when a non-relocated jump target is
a return thunk (or retpoline).
Fixes: e92626af3234 ("x86/retpoline: Remove .text..__x86.return_thunk section")
Reported-by: David Kaplan <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Acked-by: Peter Zijlstra (Intel) <[email protected]>
Link: https://lore.kernel.org/r/20231012024737.eg5phclogp67ik6x@treble
---
tools/objtool/check.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e308d1b..e94756e 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1611,6 +1611,22 @@ static int add_jump_destinations(struct objtool_file *file)
}
/*
+ * An intra-TU jump in retpoline.o might not have a relocation
+ * for its jump dest, in which case the above
+ * add_{retpoline,return}_call() didn't happen.
+ */
+ if (jump_dest->sym && jump_dest->offset == jump_dest->sym->offset) {
+ if (jump_dest->sym->retpoline_thunk) {
+ add_retpoline_call(file, insn);
+ continue;
+ }
+ if (jump_dest->sym->return_thunk) {
+ add_return_call(file, insn, true);
+ continue;
+ }
+ }
+
+ /*
* Cross-function jump.
*/
if (insn_func(insn) && insn_func(jump_dest) &&
The following commit has been merged into the x86/bugs branch of tip:
Commit-ID: 34de4fe7d1326c5c27890df3297dffd4c7196b0e
Gitweb: https://git.kernel.org/tip/34de4fe7d1326c5c27890df3297dffd4c7196b0e
Author: Josh Poimboeuf <[email protected]>
AuthorDate: Wed, 11 Oct 2023 19:47:37 -07:00
Committer: Borislav Petkov (AMD) <[email protected]>
CommitterDate: Fri, 20 Oct 2023 12:51:41 +02:00
objtool: Fix return thunk patching in retpolines
With CONFIG_RETHUNK enabled, the compiler replaces every RET with a tail
call to a return thunk ('JMP __x86_return_thunk'). Objtool annotates
all such return sites so they can be patched during boot by
apply_returns().
The implementation of __x86_return_thunk() is just a bare RET. It's
only meant to be used temporarily until apply_returns() patches all
return sites with either a JMP to another return thunk or an actual RET.
Removing the .text..__x86.return_thunk section would break objtool's
detection of return sites in retpolines. Since retpolines and return
thunks would land in the same section, the compiler no longer uses
relocations for the intra-section jumps between the retpolines and the
return thunk, causing objtool to overlook them.
As a result, none of the retpolines' return sites would get patched.
Each one stays at 'JMP __x86_return_thunk', effectively a bare RET.
Fix it by teaching objtool to detect when a non-relocated jump target is
a return thunk (or retpoline).
[ bp: Massage the commit message now that the offending commit
removing the .text..__x86.return_thunk section has been zapped.
Still keep the objtool change here as it makes objtool more robust
wrt handling such intra-TU jumps without relocations, should some
toolchain and/or config generate them in the future. ]
Reported-by: David Kaplan <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Signed-off-by: Borislav Petkov (AMD) <[email protected]>
Acked-by: Peter Zijlstra (Intel) <[email protected]>
Link: https://lore.kernel.org/r/20231012024737.eg5phclogp67ik6x@treble
---
tools/objtool/check.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e308d1b..e94756e 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1611,6 +1611,22 @@ static int add_jump_destinations(struct objtool_file *file)
}
/*
+ * An intra-TU jump in retpoline.o might not have a relocation
+ * for its jump dest, in which case the above
+ * add_{retpoline,return}_call() didn't happen.
+ */
+ if (jump_dest->sym && jump_dest->offset == jump_dest->sym->offset) {
+ if (jump_dest->sym->retpoline_thunk) {
+ add_retpoline_call(file, insn);
+ continue;
+ }
+ if (jump_dest->sym->return_thunk) {
+ add_return_call(file, insn, true);
+ continue;
+ }
+ }
+
+ /*
* Cross-function jump.
*/
if (insn_func(insn) && insn_func(jump_dest) &&