Re: [PATCH] binfmt_elf: Extract .note.gnu.property from an ELF file

On Thu, 2019-05-02 at 15:29 +0100, Dave Martin wrote:

On Thu, May 02, 2019 at 12:10:04PM +0100, Dave Martin wrote:

quoted

On Wed, May 01, 2019 at 02:12:17PM -0700, Yu-cheng Yu wrote:

[...]

quoted

quoted

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > > index

7d09d125f148..40aa4a4fd64d 100644

--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c

@@ -1076,6 +1076,19 @@ static int load_elf_binary(struct linux_binprm

*bprm)
 		goto out_free_dentry;
 	}
 
+	if (interpreter) {
+		retval = arch_setup_property(&loc->interp_elf_ex,
+					     interp_elf_phdata,
+					     interpreter, true);
+	} else {
+		retval = arch_setup_property(&loc->elf_ex,
+					     elf_phdata,
+					     bprm->file, false);
+	}

This will be too late for arm64, since we need to twiddle the mmap prot
flags for the executable's pages based on the detected properties.

Can we instead move this much earlier, letting the arch code stash
something in arch_state that can be consumed later on?

This also has the advantage that we can report errors to the execve()
caller before passing the point of no return (i.e., flush_old_exec()).

I will look into that.

[...]

quoted

quoted

diff --git a/fs/gnu_property.c b/fs/gnu_property.c

[...]

quoted

quoted

+int get_gnu_property(void *ehdr_p, void *phdr_p, struct file *f,
+		     u32 pr_type, u32 *property)
+{
+	struct elf64_hdr *ehdr64 = ehdr_p;
+	int err = 0;
+
+	*property = 0;
+
+	if (ehdr64->e_ident[EI_CLASS] == ELFCLASS64) {
+		struct elf64_phdr *phdr64 = phdr_p;
+
+		err = scan_segments_64(f, phdr64, ehdr64->e_phnum,
+				       pr_type, property);
+		if (err < 0)
+			goto out;
+	} else {
+#ifdef CONFIG_COMPAT
+		struct elf32_hdr *ehdr32 = ehdr_p;
+
+		if (ehdr32->e_ident[EI_CLASS] == ELFCLASS32) {
+			struct elf32_phdr *phdr32 = phdr_p;
+
+			err = scan_segments_32(f, phdr32, ehdr32-

quoted

e_phnum,

+					       pr_type, property);
+			if (err < 0)
+				goto out;
+		}
+#else
+	WARN_ONCE(1, "Exec of 32-bit app, but CONFIG_COMPAT is not
enabled.\n");
+	return -ENOTSUPP;
+#endif
+	}

We have already made a ton of assumptions about the ELF class by this
point, and we don't seem to check it explicitly elsewhere, so it is a
bit weird to police it specifically here.

Can we simply pass the assumed ELF class as a parameter instead?

Yes.

Yu-cheng