Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 14 Apr 2022 09:53:18 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Josh Poimboeuf <jpoimboe@redhat.com>
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: <YlfS7twQVCHGgtCV@hirez.programming.kicks-ass.net>
References: <cover.1649891421.git.jpoimboe@redhat.com>
 <44a73f724b51c4a994edc43536b7a7ee5e972b40.1649891421.git.jpoimboe@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <44a73f724b51c4a994edc43536b7a7ee5e972b40.1649891421.git.jpoimboe@redhat.com>
Precedence: bulk

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..


> -		/* 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.

> +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.

> +	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.

> +		if (!sec->reloc)
> +			continue;
> +
> +		list_for_each_entry(reloc, &sec->reloc->reloc_list, list)
> +			warnings += validate_ibt_data_reloc(file, reloc);
> +	}
> +
> +	return warnings;
>  }
>  
>  static int validate_reachable_instructions(struct objtool_file *file)
> -- 
> 2.34.1
>