Re: [PATCH] objtool: Fix SYSCALL instruction handling and INSN_CONTEXT_SWITCH

["Jürgen Groß" <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 2378 bytes --]

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]