linux-kernel - Re: [PATCH v3] 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 for Android: free password hash cracker in your pocket

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

Message-ID: <txsy3gkg5h5rdy3iy557ibk3tztpoxfszl7cxobzr4zhvgys2r@svmjohvqrknn>
Date: Sun, 6 Apr 2025 12:29:19 -0700
From: Josh Poimboeuf <jpoimboe@...nel.org>
To: Ingo Molnar <mingo@...nel.org>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org, 
	Peter Zijlstra <peterz@...radead.org>, Juergen Gross <jgross@...e.com>, 
	Andrew Cooper <andrew.cooper3@...rix.com>
Subject: Re: [PATCH v3] objtool: Fix SYSCALL instruction handling and
 INSN_CONTEXT_SWITCH

On Sun, Apr 06, 2025 at 07:41:50PM +0200, Ingo Molnar wrote:
> * Josh Poimboeuf <jpoimboe@...nel.org> wrote:
> > The !CONFIG_IA32_EMULATION version of xen_entry_SYSCALL_compat() ends
> > with a SYSCALL instruction.  In Xen, SYSCALL is a hypercall.  Usually
> > that would return, but in this case it's a hypercall to IRET, which
> > doesn't return.
> > 
> > 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 removed, INSN_CONTEXT_SWITCH now has a sane
> > well-defined return semantic.  Rename it to INSN_EXCEPTION_RETURN to
> > better reflect its meaning.
> 
> I think this should be split up into a series of ~3 patches. :-/

Yeah, you're probably right.  Let me see how I can split that up.

-- 
Josh