Re: [PATCH 10/18] objtool: Extricate ibt from stack validation

[Josh Poimboeuf <jpoimboe@redhat.com>]

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