linux-kernel - Re: [PATCH v2 01/12] objtool: Generic annotation infrastructure

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: <20241116093331.GG22801@noisy.programming.kicks-ass.net>
Date: Sat, 16 Nov 2024 10:33:31 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: Josh Poimboeuf <jpoimboe@...hat.com>
Cc: seanjc@...gle.com, pbonzini@...hat.com, tglx@...utronix.de,
	linux-kernel@...r.kernel.org, x86@...nel.org, kvm@...r.kernel.org,
	jthoughton@...gle.com
Subject: Re: [PATCH v2 01/12] objtool: Generic annotation infrastructure

On Fri, Nov 15, 2024 at 10:38:28AM -0800, Josh Poimboeuf wrote:
> On Mon, Nov 11, 2024 at 12:59:36PM +0100, Peter Zijlstra wrote:
> > +#define ASM_ANNOTATE(x)						\
> > +	"911:\n\t"						\
> > +	".pushsection .discard.annotate,\"M\",@progbits,8\n\t"	\
> > +	".long 911b - .\n\t"					\
> > +	".long " __stringify(x) "\n\t"				\
> > +	".popsection\n\t"
> 
> Why mergeable and progbits?

In order to get sh_entsize ?

> > +static int read_annotate(struct objtool_file *file, void (*func)(int type, struct instruction *insn))
> > +{
> > +	struct section *rsec, *sec;
> > +	struct instruction *insn;
> > +	struct reloc *reloc;
> > +	int type;
> > +
> > +	rsec = find_section_by_name(file->elf, ".rela.discard.annotate");
> > +	if (!rsec)
> > +		return 0;
> > +
> > +	sec = find_section_by_name(file->elf, ".discard.annotate");
> > +	if (!sec)
> > +		return 0;
> 
> Instead of looking for .rela.discard.annotate you can just get it from
> sec->rsec.

Oh, indeed.

> > +
> > +	if (sec->sh.sh_entsize != 8) {
> > +		static bool warn = false;
> 
> "warned" ?

Sure.

> > +		if (!warn) {
> > +			WARN("%s: dodgy linker, sh_entsize != 8", sec->name);
> > +			warn = true;
> > +		}
> 
> Any reason not to make this a fatal error?

lld is currently suffering from this, it would get us build failures on
llvm builds. Once that's fixed, then yes, this should become fatal.

  https://github.com/ClangBuiltLinux/linux/issues/2057

> > +		sec->sh.sh_entsize = 8;
> > +	}
> > +
> > +	for_each_reloc(rsec, reloc) {
> > +		insn = find_insn(file, reloc->sym->sec,
> > +				 reloc->sym->offset + reloc_addend(reloc));
> > +		if (!insn) {
> > +			WARN("bad .discard.annotate entry: %d", reloc_idx(reloc));
> > +			return -1;
> > +		}
> 
> Would be nice to print the type here as well.

Sure.

> > @@ -2670,6 +2714,8 @@ static int decode_sections(struct objtoo
> >  	if (ret)
> >  		return ret;
> >  
> > +	ret = read_annotate(file, __annotate_nop);
> > +
> 
> 'ret' is ignored here (not that it matters much as this goes away in the
> next patch)

Right..