linux-kernel - Re: [PATCH v3 2/6] binfmt

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: <Z31T0dMgMucke5KS@e133380.arm.com>
Date: Tue, 7 Jan 2025 16:18:25 +0000
From: Dave Martin <Dave.Martin@....com>
To: Akihiko Odaki <akihiko.odaki@...nix.com>
Cc: Eric Biederman <ebiederm@...ssion.com>, Kees Cook <kees@...nel.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Mark Brown <broonie@...nel.org>, Baoquan He <bhe@...hat.com>,
	Vivek Goyal <vgoyal@...hat.com>, Dave Young <dyoung@...hat.com>,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org,
	linuxppc-dev@...ts.ozlabs.org, linux-s390@...r.kernel.org,
	kexec@...ts.infradead.org, binutils@...rceware.org,
	devel@...nix.com
Subject: Re: [PATCH v3 2/6] binfmt_elf: Use note name macros

On Tue, Jan 07, 2025 at 09:45:53PM +0900, Akihiko Odaki wrote:
> Use note name macros to match with the userspace's expectation.

Also (and more importantly) get rid of duplicated knowledge about the
mapping of note types to note names, so that elf.h is the authoritative
source of this information?

> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@...nix.com>
> Acked-by: Baoquan He <bhe@...hat.com>
> ---
>  fs/binfmt_elf.c       | 21 ++++++++++-----------
>  fs/binfmt_elf_fdpic.c |  8 ++++----
>  2 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 106f0e8af177..5b4a92e5e508 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c

[...]

> @@ -1538,7 +1538,7 @@ static int elf_fdpic_core_dump(struct coredump_params *cprm)
>  	do
>  		i += 2;
>  	while (auxv[i - 2] != AT_NULL);
> -	fill_note(&auxv_note, "CORE", NT_AUXV, i * sizeof(elf_addr_t), auxv);
> +	fill_note(&auxv_note, NN_AUXV, NT_AUXV, i * sizeof(elf_addr_t), auxv);
>  	thread_status_size += notesize(&auxv_note);
>  
>  	offset = sizeof(*elf);				/* ELF header */

Looking at this code, it appears that the right name is explicitly
taken from elf.h for a few specific notes, but for those that are
specified by the arch code (e.g., in struct user_regset entries) the
name is still guessed locally:

static int fill_thread_core_info(...) {

...

	fill_note(&t->notes[note_iter], is_fpreg ? "CORE" : "LINUX",
		note_type, ret, data);


It would be preferable to clean this up if we want elf.h to be the
authoritative source for the names.

It would be possible to add a .core_note_name entry in struct
user_regset, and define a helper macro to populate the note type and
name, something like the following:

struct user_regset {
	...
	unsigned int core_note_type;
+	unsigned int core_note_name;
};

#define USER_REGSET_NOTE_TYPE(type) \
	.core_note_type = NT_ ## type, \
	.core_note_name = NN_ ## name,

...and then replace every .core_note_type assignment with an invocation
of this macro.  A quick git grep should easily find all the affected
cases.


Alternatively, as discussed in the last review round, a helper could
be defined to get the name for a note type:

const char *elf_note_name(int Elf32_Word n_type)
{
	switch (n_type) {
	case NT_PRSTATUS:	return NN_PRSTATUS;
	case NT_PRFPREG:	return NN_PRFPREG;
	/* ...and all the rest..., then: */

	default:
		WARN();
		return "LINUX";
	}
}

This avoids the caller having to specify the name explicitly, but only
works if all the n_type values are unique for the note types that Linux
knows about (currently true).

Experimenting with this shows that GCC 11.4.0 (for example) doesn't do
a very good job with this switch, though, and it requires building
knowledge about irrelevant arch-specific note types into every kernel.
I think that extending struct user_regset is probably the better
approach -- though other people may disagree.

Cheers
---Dave