linux-kernel - Re: [PATCH] objtool: Fix SYSCALL instruction handling and INSN_CONTEXT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2970eb7e-030d-4257-ad2f-64e378037dd8@citrix.com>
Date: Thu, 3 Apr 2025 19:57:42 +0100
From: Andrew Cooper <andrew.cooper3@...rix.com>
To: Josh Poimboeuf <jpoimboe@...nel.org>, x86@...nel.org
Cc: linux-kernel@...r.kernel.org, Peter Zijlstra <peterz@...radead.org>,
 Ingo Molnar <mingo@...nel.org>, Juergen Gross <jgross@...e.com>
Subject: Re: [PATCH] objtool: Fix SYSCALL instruction handling and
 INSN_CONTEXT_SWITCH

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@...rix.com>
> Tested-by: Andrew Cooper <andrew.cooper3@...rix.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@...nel.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