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: <20241115183828.6cs64mpbp5cqtce4@jpoimboe>
Date: Fri, 15 Nov 2024 10:38:28 -0800
From: Josh Poimboeuf <jpoimboe@...hat.com>
To: Peter Zijlstra <peterz@...radead.org>
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 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?

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


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

"warned" ?

> +		if (!warn) {
> +			WARN("%s: dodgy linker, sh_entsize != 8", sec->name);
> +			warn = true;
> +		}

Any reason not to make this a fatal error?

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

> @@ -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)

-- 
Josh