arch/x86/xen/xen-asm.S | 6 +----- tools/objtool/arch/x86/decode.c | 14 ++------------ tools/objtool/check.c | 13 ++++++------- 3 files changed, 9 insertions(+), 24 deletions(-)
The !CONFIG_IA32_EMULATION version of xen_entry_SYSCALL_compat() ends
with a SYSCALL instruction, which in reality is a hypervisor call to
trigger an IRET.
Objtool doesn't know that, so it falls through to the next function,
triggering a false positive:
vmlinux.o: warning: objtool: xen_reschedule_interrupt+0x2a: RET before UNTRAIN
Fix that by adding UD2 after the SYSCALL to avoid the undefined behavior
and prevent the objtool fallthrough, and teach validate_unret() to stop
control flow on the UD2 like validate_branch() already does.
Unfortunately that's not the whole story. While that works for
validate_unret(), it breaks validate_branch() which terminates control
flow after the SYSCALL, triggering an unreachable instruction warning on
the UD2.
The real problem here is that INSN_CONTEXT_SWITCH is ambiguous. It can
represent both call semantics (SYSCALL, SYSENTER) and return semantics
(SYSRET, IRET, RETS, RETU). Those differ significantly: calls preserve
control flow whereas returns terminate it.
validate_branch() uses an arbitrary rule for INSN_CONTEXT_SWITCH that
almost works by accident: if in a function, keep going; otherwise stop.
It should instead be based on the semantics of the underlying
instruction.
INSN_CONTEXT_SWITCH's original purpose was to enable the "unsupported
instruction in callable function" warning. But that warning really has
no reason to exist. It has never found any bugs, and those instructions
are only in entry code anyway. So just get rid of it.
That in turn allows objtool to stop caring about SYSCALL or SYSENTER.
Their call semantic means they usually don't affect control flow in the
containing function/code, and can just be INSN_OTHER. The far
returns/jumps can also be ignored as those aren't used anywhere.
With SYSCALL and SYSENTER, INSN_CONTEXT_SWITCH now has a sane
well-defined return semantic.
Fixes: a2796dff62d6 ("x86/xen: don't do PV iret hypercall through hypercall page")
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Tested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
arch/x86/xen/xen-asm.S | 6 +-----
tools/objtool/arch/x86/decode.c | 14 ++------------
tools/objtool/check.c | 13 ++++++-------
3 files changed, 9 insertions(+), 24 deletions(-)
diff --git a/arch/x86/xen/xen-asm.S b/arch/x86/xen/xen-asm.S
index 109af12f7647..5bdae7839c08 100644
--- a/arch/x86/xen/xen-asm.S
+++ b/arch/x86/xen/xen-asm.S
@@ -38,9 +38,7 @@ SYM_FUNC_START(xen_hypercall_pv)
ANNOTATE_NOENDBR
push %rcx
push %r11
- UNWIND_HINT_SAVE
syscall
- UNWIND_HINT_RESTORE
pop %r11
pop %rcx
RET
@@ -226,9 +224,7 @@ SYM_CODE_END(xen_early_idt_handler_array)
push %rax
mov $__HYPERVISOR_iret, %eax
syscall /* Do the IRET. */
-#ifdef CONFIG_MITIGATION_SLS
- int3
-#endif
+ ud2 /* The SYSCALL should never return. */
.endm
SYM_CODE_START(xen_iret)
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index 33d861c04ebd..628c2c8a0f6a 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -535,10 +535,9 @@ int arch_decode_instruction(struct objtool_file *file, const struct section *sec
insn->type = INSN_JUMP_CONDITIONAL;
- } else if (op2 == 0x05 || op2 == 0x07 || op2 == 0x34 ||
- op2 == 0x35) {
+ } else if (op2 == 0x07) {
- /* sysenter, sysret */
+ /* sysret */
insn->type = INSN_CONTEXT_SWITCH;
} else if (op2 == 0x0b || op2 == 0xb9) {
@@ -672,10 +671,6 @@ int arch_decode_instruction(struct objtool_file *file, const struct section *sec
break;
}
- fallthrough;
-
- case 0xca: /* retf */
- case 0xcb: /* retf */
insn->type = INSN_CONTEXT_SWITCH;
break;
@@ -718,11 +713,6 @@ int arch_decode_instruction(struct objtool_file *file, const struct section *sec
if (has_notrack_prefix(&ins))
WARN("notrack prefix found at %s:0x%lx", sec->name, offset);
- } else if (modrm_reg == 5) {
-
- /* jmpf */
- insn->type = INSN_CONTEXT_SWITCH;
-
} else if (modrm_reg == 6) {
/* push from mem */
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 4a1f6c3169b3..e9cba4c56aa4 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3685,13 +3685,6 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
break;
case INSN_CONTEXT_SWITCH:
- if (func) {
- if (!next_insn || !next_insn->hint) {
- WARN_INSN(insn, "unsupported instruction in callable function");
- return 1;
- }
- break;
- }
return 0;
case INSN_STAC:
@@ -3886,6 +3879,9 @@ static int validate_unret(struct objtool_file *file, struct instruction *insn)
WARN_INSN(insn, "RET before UNTRAIN");
return 1;
+ case INSN_CONTEXT_SWITCH:
+ return 0;
+
case INSN_NOP:
if (insn->retpoline_safe)
return 0;
@@ -3895,6 +3891,9 @@ static int validate_unret(struct objtool_file *file, struct instruction *insn)
break;
}
+ if (insn->dead_end)
+ return 0;
+
if (!next) {
WARN_INSN(insn, "teh end!");
return 1;
--
2.48.1On Thu, Apr 03, 2025 at 11:48:13AM -0700, Josh Poimboeuf wrote: > The real problem here is that INSN_CONTEXT_SWITCH is ambiguous. It can > represent both call semantics (SYSCALL, SYSENTER) and return semantics > (SYSRET, IRET, RETS, RETU). Those differ significantly: calls preserve > control flow whereas returns terminate it. Does that not rather suggest we should perhaps have INSN_SYSCALL / INSN_SYSRET to replace the single ambiguous thing?
On Fri, Apr 04, 2025 at 12:49:38PM +0200, Peter Zijlstra wrote: > On Thu, Apr 03, 2025 at 11:48:13AM -0700, Josh Poimboeuf wrote: > > > The real problem here is that INSN_CONTEXT_SWITCH is ambiguous. It can > > represent both call semantics (SYSCALL, SYSENTER) and return semantics > > (SYSRET, IRET, RETS, RETU). Those differ significantly: calls preserve > > control flow whereas returns terminate it. > > Does that not rather suggest we should perhaps have INSN_SYSCALL / > INSN_SYSRET to replace the single ambiguous thing? Is there any reason to have INSN_SYSCALL in the first place? -- Josh
On 04.04.25 16:46, Josh Poimboeuf wrote: > On Fri, Apr 04, 2025 at 12:49:38PM +0200, Peter Zijlstra wrote: >> On Thu, Apr 03, 2025 at 11:48:13AM -0700, Josh Poimboeuf wrote: >> >>> The real problem here is that INSN_CONTEXT_SWITCH is ambiguous. It can >>> represent both call semantics (SYSCALL, SYSENTER) and return semantics >>> (SYSRET, IRET, RETS, RETU). Those differ significantly: calls preserve >>> control flow whereas returns terminate it. >> >> Does that not rather suggest we should perhaps have INSN_SYSCALL / >> INSN_SYSRET to replace the single ambiguous thing? > > Is there any reason to have INSN_SYSCALL in the first place? > xen_hypercall_pv() needs a syscall which will return after the call of the hypervisor. xen_iret() is a special case where the syscall won't return. Whether objtool has a need for special casing it is another question I don't feel qualified to answer. Juergen
On Fri, Apr 04, 2025 at 07:46:52AM -0700, Josh Poimboeuf wrote: > On Fri, Apr 04, 2025 at 12:49:38PM +0200, Peter Zijlstra wrote: > > On Thu, Apr 03, 2025 at 11:48:13AM -0700, Josh Poimboeuf wrote: > > > > > The real problem here is that INSN_CONTEXT_SWITCH is ambiguous. It can > > > represent both call semantics (SYSCALL, SYSENTER) and return semantics > > > (SYSRET, IRET, RETS, RETU). Those differ significantly: calls preserve > > > control flow whereas returns terminate it. > > > > Does that not rather suggest we should perhaps have INSN_SYSCALL / > > INSN_SYSRET to replace the single ambiguous thing? > > Is there any reason to have INSN_SYSCALL in the first place? This xen hyperclal thing?
On Fri, Apr 04, 2025 at 04:54:12PM +0200, Peter Zijlstra wrote: > On Fri, Apr 04, 2025 at 07:46:52AM -0700, Josh Poimboeuf wrote: > > On Fri, Apr 04, 2025 at 12:49:38PM +0200, Peter Zijlstra wrote: > > > On Thu, Apr 03, 2025 at 11:48:13AM -0700, Josh Poimboeuf wrote: > > > > > > > The real problem here is that INSN_CONTEXT_SWITCH is ambiguous. It can > > > > represent both call semantics (SYSCALL, SYSENTER) and return semantics > > > > (SYSRET, IRET, RETS, RETU). Those differ significantly: calls preserve > > > > control flow whereas returns terminate it. > > > > > > Does that not rather suggest we should perhaps have INSN_SYSCALL / > > > INSN_SYSRET to replace the single ambiguous thing? > > > > Is there any reason to have INSN_SYSCALL in the first place? > > This xen hyperclal thing? SYSCALL does a hypercall. It usually returns. Which means we don't need to care about it, except for the one case where it "calls" IRET. That's the one with a UD2. Similar to replacing unreachable() with BUG(). -- Josh
On 03.04.25 20:48, Josh Poimboeuf wrote:
> The !CONFIG_IA32_EMULATION version of xen_entry_SYSCALL_compat() ends
> with a SYSCALL instruction, which in reality is a hypervisor call to
> trigger an IRET.
>
> Objtool doesn't know that, so it falls through to the next function,
> triggering a false positive:
>
> vmlinux.o: warning: objtool: xen_reschedule_interrupt+0x2a: RET before UNTRAIN
>
> Fix that by adding UD2 after the SYSCALL to avoid the undefined behavior
> and prevent the objtool fallthrough, and teach validate_unret() to stop
> control flow on the UD2 like validate_branch() already does.
>
> Unfortunately that's not the whole story. While that works for
> validate_unret(), it breaks validate_branch() which terminates control
> flow after the SYSCALL, triggering an unreachable instruction warning on
> the UD2.
>
> The real problem here is that INSN_CONTEXT_SWITCH is ambiguous. It can
> represent both call semantics (SYSCALL, SYSENTER) and return semantics
> (SYSRET, IRET, RETS, RETU). Those differ significantly: calls preserve
> control flow whereas returns terminate it.
>
> validate_branch() uses an arbitrary rule for INSN_CONTEXT_SWITCH that
> almost works by accident: if in a function, keep going; otherwise stop.
> It should instead be based on the semantics of the underlying
> instruction.
>
> INSN_CONTEXT_SWITCH's original purpose was to enable the "unsupported
> instruction in callable function" warning. But that warning really has
> no reason to exist. It has never found any bugs, and those instructions
> are only in entry code anyway. So just get rid of it.
>
> That in turn allows objtool to stop caring about SYSCALL or SYSENTER.
> Their call semantic means they usually don't affect control flow in the
> containing function/code, and can just be INSN_OTHER. The far
> returns/jumps can also be ignored as those aren't used anywhere.
>
> With SYSCALL and SYSENTER, INSN_CONTEXT_SWITCH now has a sane
> well-defined return semantic.
>
> Fixes: a2796dff62d6 ("x86/xen: don't do PV iret hypercall through hypercall page")
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Tested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
For the xen part:
Reviewed-by: Juergen Gross <jgross@suse.com>
Juergen
On 03/04/2025 7:48 pm, Josh Poimboeuf wrote:
> The !CONFIG_IA32_EMULATION version of xen_entry_SYSCALL_compat() ends
> with a SYSCALL instruction, which in reality is a hypervisor call to
> trigger an IRET.
>
> Objtool doesn't know that, so it falls through to the next function,
> triggering a false positive:
>
> vmlinux.o: warning: objtool: xen_reschedule_interrupt+0x2a: RET before UNTRAIN
>
> Fix that by adding UD2 after the SYSCALL to avoid the undefined behavior
> and prevent the objtool fallthrough, and teach validate_unret() to stop
> control flow on the UD2 like validate_branch() already does.
>
> Unfortunately that's not the whole story. While that works for
> validate_unret(), it breaks validate_branch() which terminates control
> flow after the SYSCALL, triggering an unreachable instruction warning on
> the UD2.
>
> The real problem here is that INSN_CONTEXT_SWITCH is ambiguous. It can
> represent both call semantics (SYSCALL, SYSENTER) and return semantics
> (SYSRET, IRET, RETS, RETU). Those differ significantly: calls preserve
> control flow whereas returns terminate it.
>
> validate_branch() uses an arbitrary rule for INSN_CONTEXT_SWITCH that
> almost works by accident: if in a function, keep going; otherwise stop.
> It should instead be based on the semantics of the underlying
> instruction.
>
> INSN_CONTEXT_SWITCH's original purpose was to enable the "unsupported
> instruction in callable function" warning. But that warning really has
> no reason to exist. It has never found any bugs, and those instructions
> are only in entry code anyway. So just get rid of it.
>
> That in turn allows objtool to stop caring about SYSCALL or SYSENTER.
> Their call semantic means they usually don't affect control flow in the
> containing function/code, and can just be INSN_OTHER. The far
> returns/jumps can also be ignored as those aren't used anywhere.
>
> With SYSCALL and SYSENTER, INSN_CONTEXT_SWITCH now has a sane
> well-defined return semantic.
Do you mean "without" here?
>
> Fixes: a2796dff62d6 ("x86/xen: don't do PV iret hypercall through hypercall page")
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Tested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
Thankyou for all your help on this one.
> diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
> index 33d861c04ebd..628c2c8a0f6a 100644
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -535,10 +535,9 @@ int arch_decode_instruction(struct objtool_file *file, const struct section *sec
>
> insn->type = INSN_JUMP_CONDITIONAL;
>
> - } else if (op2 == 0x05 || op2 == 0x07 || op2 == 0x34 ||
> - op2 == 0x35) {
> + } else if (op2 == 0x07) {
>
> - /* sysenter, sysret */
> + /* sysret */
> insn->type = INSN_CONTEXT_SWITCH;
Linux doesn't use SYSEXIT, but it's conceptually like SYSRET/ERETx so
perhaps worth keeping the 0x35 here?
~Andrew
On Thu, Apr 03, 2025 at 07:57:42PM +0100, Andrew Cooper wrote:
> On 03/04/2025 7:48 pm, Josh Poimboeuf wrote:
> > With SYSCALL and SYSENTER, INSN_CONTEXT_SWITCH now has a sane
> > well-defined return semantic.
>
> Do you mean "without" here?
I was just testing to see if anybody actually read all the way to the
bottom. Congratulations, you passed the test!
> > diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
> > index 33d861c04ebd..628c2c8a0f6a 100644
> > --- a/tools/objtool/arch/x86/decode.c
> > +++ b/tools/objtool/arch/x86/decode.c
> > @@ -535,10 +535,9 @@ int arch_decode_instruction(struct objtool_file *file, const struct section *sec
> >
> > insn->type = INSN_JUMP_CONDITIONAL;
> >
> > - } else if (op2 == 0x05 || op2 == 0x07 || op2 == 0x34 ||
> > - op2 == 0x35) {
> > + } else if (op2 == 0x07) {
> >
> > - /* sysenter, sysret */
> > + /* sysret */
> > insn->type = INSN_CONTEXT_SWITCH;
>
> Linux doesn't use SYSEXIT, but it's conceptually like SYSRET/ERETx so
> perhaps worth keeping the 0x35 here?
In theory yes, but objtool will never support x86-32. Note I also
removed retf and jmpf, I'm thinking it's simpler to just stick to the
instructions we actually use.
--
Josh
On 03/04/2025 8:05 pm, Josh Poimboeuf wrote:
> On Thu, Apr 03, 2025 at 07:57:42PM +0100, Andrew Cooper wrote:
>> On 03/04/2025 7:48 pm, Josh Poimboeuf wrote:
>>> With SYSCALL and SYSENTER, INSN_CONTEXT_SWITCH now has a sane
>>> well-defined return semantic.
>> Do you mean "without" here?
> I was just testing to see if anybody actually read all the way to the
> bottom. Congratulations, you passed the test!
A likely story, methinks...
>
>>> diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
>>> index 33d861c04ebd..628c2c8a0f6a 100644
>>> --- a/tools/objtool/arch/x86/decode.c
>>> +++ b/tools/objtool/arch/x86/decode.c
>>> @@ -535,10 +535,9 @@ int arch_decode_instruction(struct objtool_file *file, const struct section *sec
>>>
>>> insn->type = INSN_JUMP_CONDITIONAL;
>>>
>>> - } else if (op2 == 0x05 || op2 == 0x07 || op2 == 0x34 ||
>>> - op2 == 0x35) {
>>> + } else if (op2 == 0x07) {
>>>
>>> - /* sysenter, sysret */
>>> + /* sysret */
>>> insn->type = INSN_CONTEXT_SWITCH;
>> Linux doesn't use SYSEXIT, but it's conceptually like SYSRET/ERETx so
>> perhaps worth keeping the 0x35 here?
> In theory yes, but objtool will never support x86-32. Note I also
> removed retf and jmpf, I'm thinking it's simpler to just stick to the
> instructions we actually use.
>
Perhaps, but they'll now become INSN_OTHER, won't they?
If they're instructions genuinely expected never to encounter, wouldn't
it be better to make a hard error rather than to add another fallthrough
case?
~Andrew
On Thu, Apr 03, 2025 at 08:15:45PM +0100, Andrew Cooper wrote:
> On 03/04/2025 8:05 pm, Josh Poimboeuf wrote:
> > On Thu, Apr 03, 2025 at 07:57:42PM +0100, Andrew Cooper wrote:
> >> On 03/04/2025 7:48 pm, Josh Poimboeuf wrote:
> >>> diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
> >>> index 33d861c04ebd..628c2c8a0f6a 100644
> >>> --- a/tools/objtool/arch/x86/decode.c
> >>> +++ b/tools/objtool/arch/x86/decode.c
> >>> @@ -535,10 +535,9 @@ int arch_decode_instruction(struct objtool_file *file, const struct section *sec
> >>>
> >>> insn->type = INSN_JUMP_CONDITIONAL;
> >>>
> >>> - } else if (op2 == 0x05 || op2 == 0x07 || op2 == 0x34 ||
> >>> - op2 == 0x35) {
> >>> + } else if (op2 == 0x07) {
> >>>
> >>> - /* sysenter, sysret */
> >>> + /* sysret */
> >>> insn->type = INSN_CONTEXT_SWITCH;
> >> Linux doesn't use SYSEXIT, but it's conceptually like SYSRET/ERETx so
> >> perhaps worth keeping the 0x35 here?
> > In theory yes, but objtool will never support x86-32. Note I also
> > removed retf and jmpf, I'm thinking it's simpler to just stick to the
> > instructions we actually use.
> >
>
> Perhaps, but they'll now become INSN_OTHER, won't they?
>
> If they're instructions genuinely expected never to encounter, wouldn't
> it be better to make a hard error rather than to add another fallthrough
> case?
Fair enough. Maybe I'll just leave them as INSN_CONTEXT_SWITCH, that'd
be easier than adding new assertions.
--
Josh
© 2016 - 2026 Red Hat, Inc.