linux-kernel - Re: [PATCH 09/14] x86/ibt: Implement IBT+

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: <20240928131216.GB19439@noisy.programming.kicks-ass.net>
Date: Sat, 28 Sep 2024 15:12:16 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Josh Poimboeuf <jpoimboe@...nel.org>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org, alyssa.milburn@...el.com,
	scott.d.constable@...el.com, joao@...rdrivepizza.com,
	andrew.cooper3@...rix.com, jose.marchesi@...cle.com,
	hjl.tools@...il.com, ndesaulniers@...gle.com,
	samitolvanen@...gle.com, nathan@...nel.org, ojeda@...nel.org,
	kees@...nel.org, alexei.starovoitov@...il.com
Subject: Re: [PATCH 09/14] x86/ibt: Implement IBT+

On Fri, Sep 27, 2024 at 06:07:33PM -0700, Josh Poimboeuf wrote:
> On Fri, Sep 27, 2024 at 09:49:05PM +0200, Peter Zijlstra wrote:
> > +static void *translate_call_dest(void *dest, bool call)
> > +{
> > +	if (cpu_feature_enabled(X86_FEATURE_CALL_DEPTH)) {
> > +		if (!call)
> > +			return dest;
> 
> A tail call is considered a call by virtue of the function name.  Change
> the "call" arg to "tail" to make it clearer.

Sure.

> > +++ b/scripts/Makefile.lib
> > @@ -269,6 +269,7 @@ objtool-args-$(CONFIG_HAVE_JUMP_LABEL_HA
> >  objtool-args-$(CONFIG_HAVE_NOINSTR_HACK)		+= --hacks=noinstr
> >  objtool-args-$(CONFIG_MITIGATION_CALL_DEPTH_TRACKING)	+= --direct-call
> >  objtool-args-$(CONFIG_X86_KERNEL_IBT)			+= --ibt
> > +objtool-args-$(CONFIG_X86_KERNEL_IBT_PLUS)		+= --direct-call
> 
> Only add '--direct-call' once:
> 
> objtool-args-$(or $(CONFIG_MITIGATION_CALL_DEPTH_TRACKING),$(CONFIG_X86_KERNEL_IBT_PLUS)) += --direct-call

Heh. I don't do enough Makefile to ever remember how they work :/

> >  objtool-args-$(CONFIG_FINEIBT)				+= --cfi
> >  objtool-args-$(CONFIG_FTRACE_MCOUNT_USE_OBJTOOL)	+= --mcount
> >  ifdef CONFIG_FTRACE_MCOUNT_USE_OBJTOOL
> 
> > --- a/tools/objtool/check.c
> > +++ b/tools/objtool/check.c
> > @@ -1455,7 +1455,7 @@ static void annotate_call_site(struct ob
> >  		return;
> >  	}
> >  
> > -	if (!insn->sec->init && !insn->_call_dest->embedded_insn) {
> > +	if (!insn->_call_dest->embedded_insn) {
> 
> Why did we have the 'init' check to start with?  Presumably init memory
> gets freed after the call dest patching so this is not a problem?

Ah, all this came from the calldepth tracking stuff. And we didn't care
about init code, that runs before userspace and so RSB overflows aren't
much of a problem.

But with this here thing, we very much also want to patch the init code
to not land on an ENDBR that will be turned into a UD1 during init :-)