Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Thu, 14 Apr 2022 08:44:49 -0700
From:   Josh Poimboeuf <jpoimboe@redhat.com>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     x86@kernel.org, linux-kernel@vger.kernel.org,
        Miroslav Benes <mbenes@suse.cz>
Subject: Re: [PATCH 10/18] objtool: Extricate ibt from stack validation
Message-ID: <20220414154449.5moa7xsczwybbqhd@treble>
References: <cover.1649891421.git.jpoimboe@redhat.com>
 <44a73f724b51c4a994edc43536b7a7ee5e972b40.1649891421.git.jpoimboe@redhat.com>
 <YlfS7twQVCHGgtCV@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <YlfS7twQVCHGgtCV@hirez.programming.kicks-ass.net>
Precedence: bulk

On Thu, Apr 14, 2022 at 09:53:18AM +0200, Peter Zijlstra wrote:
> On Wed, Apr 13, 2022 at 04:19:45PM -0700, Josh Poimboeuf wrote:
> > Extricate ibt from validate_branch() in preparation for making stack
> > validation optional.
> 
> It does a bit more..

Indeed.

> > -		/* already done in validate_branch() */
> > -		if (sec->sh.sh_flags & SHF_EXECINSTR)
> > -			continue;
> >  
> > -		if (!sec->reloc)
> >  			continue;
> >  
> > -		if (!strncmp(sec->name, ".orc", 4))
> > -			continue;
> >  
> > -		if (!strncmp(sec->name, ".discard", 8))
> >  			continue;
> >  
> > -		if (!strncmp(sec->name, ".debug", 6))
> >  			continue;
> >  
> > -		if (!strcmp(sec->name, "_error_injection_whitelist"))
> >  			continue;
> >  
> > -		if (!strcmp(sec->name, "_kprobe_blacklist"))
> >  			continue;
> >  
> > -		is_data = strstr(sec->name, ".data") || strstr(sec->name, ".rodata");
> >  
> > -		list_for_each_entry(reloc, &sec->reloc->reloc_list, list) {
> > -			struct instruction *dest;
> >  
> > -			dest = validate_ibt_reloc(file, reloc);
> > -			if (is_data && dest && !dest->noendbr)
> > -				warn_noendbr("data ", sec, reloc->offset, dest);
> > -		}
> 
> So this iterates all sections and excludes a bunch, and only reports
> fail for .data/.rodata.

Oops.

> > +static int validate_ibt(struct objtool_file *file)
> > +{
> > +	struct section *sec;
> > +	struct reloc *reloc;
> > +	struct instruction *insn;
> > +	int warnings = 0;
> > +
> > +	for_each_insn(file, insn)
> > +		warnings += validate_ibt_insn(file, insn);
> 
> So I specifically didn't do this because I wanted to reduce the amount
> of loops we do over those instructions. But yeah, if you really want to
> allow --ibt without --stack-validate (but why?) then I suppose so.
> 
> Esp. for the vmlinux.o case, iterating all insn can quickly add up to
> significant time.

I didn't look at the performance, but if it's a problem then we can
eventually look at combining several of the for_each_insn() features
into a single loop: retpolines, ibt, reachability check, maybe even some
of decode_sections().

> 
> > +	for_each_sec(file, sec) {
> > +
> > +		if (!strstr(sec->name, ".data") && !strstr(sec->name, ".rodata"))
> > +			continue;
> 
> But this only iterates .data/.rodata.
> 
> That's not the same, specifically, it'll not iterate stuff like ksymtab
> that contains the EXPORT_SYMBOL* crud. The result being that we can now
> seal EXPORT'ed symbols, which will make modules really sad.
> 
> There's also the .initcall sections, sealing initcalls typcally ends really
> badly.
> 
> And there might be a few others I forgot about.

Ok.  That was subtle, it needs a comment or two.  I had the distinct
feeling I was introducing a bug, then I got distracted ;-)

Doesn't the compiler give those special cases ENDBR anyway?  Just
wondering why we avoid the warning for those.

-- 
Josh