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

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

[Yu-cheng Yu]

An ELF file's .note.gnu.property indicates features the executable file
can support.  For example, the property GNU_PROPERTY_X86_FEATURE_1_AND
indicates the file supports GNU_PROPERTY_X86_FEATURE_1_IBT and/or
GNU_PROPERTY_X86_FEATURE_1_SHSTK.

This patch was part of the Control-flow Enforcement series; the original
patch is here: https://lkml.org/lkml/2018/11/20/205.  Dave Martin responded
that ARM recently introduced new features to NT_GNU_PROPERTY_TYPE_0 with
properties closely modelled on GNU_PROPERTY_X86_FEATURE_1_AND, and it is
logical to split out the generic part.  Here it is.

With this patch, if an arch needs to setup features from ELF properties,
it needs CONFIG_ARCH_USE_GNU_PROPERTY to be set, and a specific
arch_setup_property().

For example, for X86_64:

int arch_setup_property(void *ehdr, void *phdr, struct file *f, bool inter)
{
	int r;
	uint32_t property;

	r = get_gnu_property(ehdr, phdr, f, GNU_PROPERTY_X86_FEATURE_1_AND,
			     &property);
	...
}

Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
---
 fs/Kconfig.binfmt        |   4 +
 fs/Makefile              |   1 +
 fs/binfmt_elf.c          |  13 ++
 fs/gnu_property.c        | 363 +++++++++++++++++++++++++++++++++++++++
 include/linux/elf.h      |  12 ++
 include/uapi/linux/elf.h |   8 +
 6 files changed, 401 insertions(+)
 create mode 100644 fs/gnu_property.c

diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt
index b795f8da81f3..175a1f58e785 100644
--- a/fs/Kconfig.binfmt
+++ b/fs/Kconfig.binfmt
@@ -35,6 +35,10 @@ config COMPAT_BINFMT_ELF
 config ARCH_BINFMT_ELF_STATE
 	bool
 
+config ARCH_USE_GNU_PROPERTY
+	bool
+	depends on 64BIT
+
 config BINFMT_ELF_FDPIC
 	bool "Kernel support for FDPIC ELF binaries"
 	default y if !BINFMT_ELF
diff --git a/fs/Makefile b/fs/Makefile
index 427fec226fae..8a35abbebf8b 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_BINFMT_ELF)	+= binfmt_elf.o
 obj-$(CONFIG_COMPAT_BINFMT_ELF)	+= compat_binfmt_elf.o
 obj-$(CONFIG_BINFMT_ELF_FDPIC)	+= binfmt_elf_fdpic.o
 obj-$(CONFIG_BINFMT_FLAT)	+= binfmt_flat.o
+obj-$(CONFIG_ARCH_USE_GNU_PROPERTY) += gnu_property.o
 
 obj-$(CONFIG_FS_MBCACHE)	+= mbcache.o
 obj-$(CONFIG_FS_POSIX_ACL)	+= posix_acl.o
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);
+	}
+
+	if (retval < 0)
+		goto out_free_dentry;
+
 	if (elf_interpreter) {
 		unsigned long interp_map_addr = 0;
 
diff --git a/fs/gnu_property.c b/fs/gnu_property.c
new file mode 100644
index 000000000000..656ea3951840
--- /dev/null
+++ b/fs/gnu_property.c
@@ -0,0 +1,363 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Extract an ELF file's .note.gnu.property.
+ *
+ * The path from the ELF header to the note section is the following:
+ * elfhdr->elf_phdr->elf_note->property[].
+ */
+
+#include <uapi/linux/elf-em.h>
+#include <linux/processor.h>
+#include <linux/binfmts.h>
+#include <linux/elf.h>
+#include <linux/slab.h>
+#include <linux/fs.h>
+#include <linux/uaccess.h>
+#include <linux/string.h>
+#include <linux/compat.h>
+
+/*
+ * The .note.gnu.property layout:
+ *
+ *	struct elf_note {
+ *		u32 n_namesz; --> sizeof(n_name[]); always (4)
+ *		u32 n_ndescsz;--> sizeof(property[])
+ *		u32 n_type;   --> always NT_GNU_PROPERTY_TYPE_0
+ *	};
+ *	char n_name[4]; --> always 'GNU\0'
+ *
+ *	struct {
+ *		struct gnu_property {
+ *			u32 pr_type;
+ *			u32 pr_datasz;
+ *		};
+ *		u8 pr_data[pr_datasz];
+ *	}[];
+ */
+
+#define BUF_SIZE (PAGE_SIZE / 4)
+
+struct gnu_property {
+	u32 pr_type;
+	u32 pr_datasz;
+};
+
+typedef bool (test_item_fn)(void *buf, u32 *arg, u32 type);
+typedef void *(next_item_fn)(void *buf, u32 *arg, u32 type);
+
+static inline bool test_note_type(void *buf, u32 *align, u32 note_type)
+{
+	struct elf_note *n = buf;
+
+	return ((n->n_type == note_type) && (n->n_namesz == 4) &&
+		(memcmp(n + 1, "GNU", 4) == 0));
+}
+
+static inline void *next_note(void *buf, u32 *align, u32 note_type)
+{
+	struct elf_note *n = buf;
+	u64 size;
+
+	if (check_add_overflow((u64)sizeof(*n), (u64)n->n_namesz, &size))
+		return NULL;
+
+	size = round_up(size, *align);
+
+	if (check_add_overflow(size, (u64)n->n_descsz, &size))
+		return NULL;
+
+	size = round_up(size, *align);
+
+	if (buf + size < buf)
+		return NULL;
+	else
+		return (buf + size);
+}
+
+static inline bool test_property(void *buf, u32 *max_type, u32 pr_type)
+{
+	struct gnu_property *pr = buf;
+
+	/*
+	 * Property types must be in ascending order.
+	 * Keep track of the max when testing each.
+	 */
+	if (pr->pr_type > *max_type)
+		*max_type = pr->pr_type;
+
+	return (pr->pr_type == pr_type);
+}
+
+static inline void *next_property(void *buf, u32 *max_type, u32 pr_type)
+{
+	struct gnu_property *pr = buf;
+
+	if ((buf + sizeof(*pr) +  pr->pr_datasz < buf) ||
+	    (pr->pr_type > pr_type) ||
+	    (pr->pr_type > *max_type))
+		return NULL;
+	else
+		return (buf + sizeof(*pr) + pr->pr_datasz);
+}
+
+/*
+ * Scan 'buf' for a pattern; return true if found.
+ * *pos is the distance from the beginning of buf to where
+ * the searched item or the next item is located.
+ */
+static int scan(u8 *buf, u32 buf_size, int item_size, test_item_fn test_item,
+		next_item_fn next_item, u32 *arg, u32 type, u32 *pos)
+{
+	int found = 0;
+	u8 *p, *max;
+
+	max = buf + buf_size;
+	if (max < buf)
+		return 0;
+
+	p = buf;
+
+	while ((p + item_size < max) && (p + item_size > buf)) {
+		if (test_item(p, arg, type)) {
+			found = 1;
+			break;
+		}
+
+		p = next_item(p, arg, type);
+	}
+
+	*pos = (p + item_size <= buf) ? 0 : (u32)(p - buf);
+	return found;
+}
+
+/*
+ * Search an NT_GNU_PROPERTY_TYPE_0 for the property of 'pr_type'.
+ */
+static int find_property(struct file *file, unsigned long desc_size,
+			 loff_t file_offset, u8 *buf,
+			 u32 pr_type, u32 *property)
+{
+	u32 buf_pos;
+	unsigned long read_size;
+	unsigned long done;
+	int found = 0;
+	int ret = 0;
+	u32 last_pr = 0;
+
+	*property = 0;
+	buf_pos = 0;
+
+	for (done = 0; done < desc_size; done += buf_pos) {
+		read_size = desc_size - done;
+		if (read_size > BUF_SIZE)
+			read_size = BUF_SIZE;
+
+		ret = kernel_read(file, buf, read_size, &file_offset);
+
+		if (ret != read_size)
+			return (ret < 0) ? ret : -EIO;
+
+		ret = 0;
+		found = scan(buf, read_size, sizeof(struct gnu_property),
+			     test_property, next_property,
+			     &last_pr, pr_type, &buf_pos);
+
+		if ((!buf_pos) || found)
+			break;
+
+		file_offset += buf_pos - read_size;
+	}
+
+	if (found) {
+		struct gnu_property *pr =
+			(struct gnu_property *)(buf + buf_pos);
+
+		if (pr->pr_datasz == 4) {
+			u32 *max =  (u32 *)(buf + read_size);
+			u32 *data = (u32 *)((u8 *)pr + sizeof(*pr));
+
+			if (data + 1 <= max) {
+				*property = *data;
+			} else {
+				file_offset += buf_pos - read_size;
+				file_offset += sizeof(*pr);
+				ret = kernel_read(file, property, 4,
+						  &file_offset);
+			}
+		}
+	}
+
+	return ret;
+}
+
+/*
+ * Search a PT_NOTE segment for NT_GNU_PROPERTY_TYPE_0.
+ */
+static int find_note_type_0(struct file *file, loff_t file_offset,
+			    unsigned long note_size, u32 align,
+			    u32 pr_type, u32 *property)
+{
+	u8 *buf;
+	u32 buf_pos;
+	unsigned long read_size;
+	unsigned long done;
+	int found = 0;
+	int ret = 0;
+
+	buf = kmalloc(BUF_SIZE, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	*property = 0;
+	buf_pos = 0;
+
+	for (done = 0; done < note_size; done += buf_pos) {
+		read_size = note_size - done;
+		if (read_size > BUF_SIZE)
+			read_size = BUF_SIZE;
+
+		ret = kernel_read(file, buf, read_size, &file_offset);
+
+		if (ret != read_size) {
+			ret = (ret < 0) ? ret : -EIO;
+			kfree(buf);
+			return ret;
+		}
+
+		/*
+		 * item_size = sizeof(struct elf_note) + elf_note.n_namesz.
+		 * n_namesz is 4 for the note type we look for.
+		 */
+		ret = scan(buf, read_size, sizeof(struct elf_note) + 4,
+			      test_note_type, next_note,
+			      &align, NT_GNU_PROPERTY_TYPE_0, &buf_pos);
+
+		file_offset += buf_pos - read_size;
+
+		if (ret && !found) {
+			struct elf_note *n =
+				(struct elf_note *)(buf + buf_pos);
+			u64 start = round_up(sizeof(*n) + n->n_namesz, align);
+			u64 total = 0;
+
+			if (check_add_overflow(start, (u64)n->n_descsz, &total)) {
+				ret = -EINVAL;
+				break;
+			}
+			total = round_up(total, align);
+
+			ret = find_property(file, n->n_descsz,
+					    file_offset + start,
+					    buf, pr_type, property);
+			found++;
+			file_offset += total;
+			buf_pos += total;
+		} else if (!buf_pos || ret) {
+			ret = 0;
+			*property = 0;
+			break;
+		}
+	}
+
+	kfree(buf);
+	return ret;
+}
+
+/*
+ * Look at an ELF file's PT_NOTE segments, then NT_GNU_PROPERTY_TYPE_0, then
+ * the property of pr_type.
+ *
+ * Input:
+ *	file: the file to search;
+ *	phdr: the file's elf header;
+ *	phnum: number of entries in phdr;
+ *	pr_type: the property type.
+ *
+ * Output:
+ *	The property found.
+ *
+ * Return:
+ *	Zero or error.
+ */
+static int scan_segments_64(struct file *file, struct elf64_phdr *phdr,
+			    int phnum, u32 pr_type, u32 *property)
+{
+	int i;
+	int err = 0;
+
+	for (i = 0; i < phnum; i++, phdr++) {
+		if ((phdr->p_type != PT_NOTE) || (phdr->p_align != 8))
+			continue;
+
+		/*
+		 * Search the PT_NOTE segment for NT_GNU_PROPERTY_TYPE_0.
+		 */
+		err = find_note_type_0(file, phdr->p_offset, phdr->p_filesz,
+				       phdr->p_align, pr_type, property);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+#ifdef CONFIG_COMPAT
+static int scan_segments_32(struct file *file, struct elf32_phdr *phdr,
+			    int phnum, u32 pr_type, u32 *property)
+{
+	int i;
+	int err = 0;
+
+	for (i = 0; i < phnum; i++, phdr++) {
+		if ((phdr->p_type != PT_NOTE) || (phdr->p_align != 4))
+			continue;
+
+		/*
+		 * Search the PT_NOTE segment for NT_GNU_PROPERTY_TYPE_0.
+		 */
+		err = find_note_type_0(file, phdr->p_offset, phdr->p_filesz,
+				       phdr->p_align, pr_type, property);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+#endif
+
+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->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
+	}
+
+out:
+	return err;
+}
diff --git a/include/linux/elf.h b/include/linux/elf.h
index e3649b3e970e..c15febebe7f2 100644
--- a/include/linux/elf.h
+++ b/include/linux/elf.h
@@ -56,4 +56,16 @@ static inline int elf_coredump_extra_notes_write(struct coredump_params *cprm) {
 extern int elf_coredump_extra_notes_size(void);
 extern int elf_coredump_extra_notes_write(struct coredump_params *cprm);
 #endif
+
+#ifdef CONFIG_ARCH_USE_GNU_PROPERTY
+extern int arch_setup_property(void *ehdr, void *phdr, struct file *f,
+			       bool interp);
+extern int get_gnu_property(void *ehdr_p, void *phdr_p, struct file *f,
+			    u32 pr_type, u32 *feature);
+#else
+static inline int arch_setup_property(void *ehdr, void *phdr, struct file *f,
+				      bool interp) { return 0; }
+static inline int get_gnu_property(void *ehdr_p, void *phdr_p, struct file *f,
+				   u32 pr_type, u32 *feature) { return 0; }
+#endif
 #endif /* _LINUX_ELF_H */
diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
index 34c02e4290fe..7b7603a44cbc 100644
--- a/include/uapi/linux/elf.h
+++ b/include/uapi/linux/elf.h
@@ -372,6 +372,7 @@ typedef struct elf64_shdr {
 #define NT_PRFPREG	2
 #define NT_PRPSINFO	3
 #define NT_TASKSTRUCT	4
+#define NT_GNU_PROPERTY_TYPE_0 5
 #define NT_AUXV		6
 /*
  * Note to userspace developers: size of NT_SIGINFO note may increase
@@ -443,4 +444,11 @@ typedef struct elf64_note {
   Elf64_Word n_type;	/* Content type */
 } Elf64_Nhdr;
 
+/* .note.gnu.property types */
+#define GNU_PROPERTY_X86_FEATURE_1_AND		(0xc0000002)
+
+/* Bits of GNU_PROPERTY_X86_FEATURE_1_AND */
+#define GNU_PROPERTY_X86_FEATURE_1_IBT		(0x00000001)
+#define GNU_PROPERTY_X86_FEATURE_1_SHSTK	(0x00000002)
+
 #endif /* _UAPI_LINUX_ELF_H */
-- 
2.17.1

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

[Matthew Wilcox]

On Wed, May 01, 2019 at 02:12:17PM -0700, Yu-cheng Yu wrote:
> +++ b/fs/Kconfig.binfmt
> @@ -35,6 +35,10 @@ config COMPAT_BINFMT_ELF
>  config ARCH_BINFMT_ELF_STATE
>  	bool
>  
> +config ARCH_USE_GNU_PROPERTY
> +	bool
> +	depends on 64BIT

I don't think this is right.  I think you should get rid of the depends line
and instead select the symbol from each of argh64 and x86 Kconfig files.

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

[Yu-cheng Yu]

On Wed, 2019-05-01 at 14:37 -0700, Matthew Wilcox wrote:
> On Wed, May 01, 2019 at 02:12:17PM -0700, Yu-cheng Yu wrote:
> > +++ b/fs/Kconfig.binfmt
> > @@ -35,6 +35,10 @@ config COMPAT_BINFMT_ELF
> >  config ARCH_BINFMT_ELF_STATE
> >  	bool
> >  
> > +config ARCH_USE_GNU_PROPERTY
> > +	bool
> > +	depends on 64BIT
> 
> I don't think this is right.  I think you should get rid of the depends line
> and instead select the symbol from each of argh64 and x86 Kconfig files.

That makes sense.  Thanks!

Yu-cheng

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

[Dave Martin]

On Wed, May 01, 2019 at 02:12:17PM -0700, Yu-cheng Yu wrote:
> An ELF file's .note.gnu.property indicates features the executable file
> can support.  For example, the property GNU_PROPERTY_X86_FEATURE_1_AND
> indicates the file supports GNU_PROPERTY_X86_FEATURE_1_IBT and/or
> GNU_PROPERTY_X86_FEATURE_1_SHSTK.
> 
> This patch was part of the Control-flow Enforcement series; the original
> patch is here: https://lkml.org/lkml/2018/11/20/205.  Dave Martin responded
> that ARM recently introduced new features to NT_GNU_PROPERTY_TYPE_0 with
> properties closely modelled on GNU_PROPERTY_X86_FEATURE_1_AND, and it is
> logical to split out the generic part.  Here it is.
> 
> With this patch, if an arch needs to setup features from ELF properties,
> it needs CONFIG_ARCH_USE_GNU_PROPERTY to be set, and a specific
> arch_setup_property().
> 
> For example, for X86_64:
> 
> int arch_setup_property(void *ehdr, void *phdr, struct file *f, bool inter)
> {
> 	int r;
> 	uint32_t property;
> 
> 	r = get_gnu_property(ehdr, phdr, f, GNU_PROPERTY_X86_FEATURE_1_AND,
> 			     &property);
> 	...
> }

Thanks, this is timely for me.  I should be able to build the needed
arm64 support pretty quickly around this now.

[Cc'ing libc-alpha for the elf.h question -- see (2)]


A couple of questions before I look in more detail:

1) Can we rely on PT_GNU_PROPERTY being present in the phdrs to describe
the NT_GNU_PROPERTY_TYPE_0 note?  If so, we can avoid trying to parse
irrelevant PT_NOTE segments.


2) Are there standard types for things like the program property header?
If not, can we add something in elf.h?  We should try to coordinate with
libc on that.  Something like

typedef __u32 Elf_Word;

typedef struct {
	Elf_Word pr_type;
	Elf_Word pr_datasz;
} Elf_Gnu_Prophdr;

(i.e., just the header part from [1], with a more specific name -- which
I just made up).


Given the fragmented nature and draft status of the specs -- and
differing opiniions about the sizes and alignments of certain things --
it could be useful to have this explicitly in the kernel.  Some
documentation as to _precisely_ what we accept may also be a good idea.


3) It looks like we have to go and re-parse all the notes for every
property requested by the arch code.

For now there is only one property requested anyway, so this is probably
not too bad.  But could we flip things around so that we have some
CONFIG_ARCH_WANTS_ELF_GNU_PROPERTY (say), and have the ELF core code
call into the arch backend for each property found?

The arch could provide some hook

	int arch_elf_has_gnu_property(const Elf_Gnu_Prophdr *prop,
					const void *data);

to consume the properties as they are found.

This would effectively replace the arch_setup_property() hook you
currently have.

Cheers
---Dave

[1] https://github.com/hjl-tools/linux-abi/wiki/Linux-Extensions-to-gABI

> 
> Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
>  fs/Kconfig.binfmt        |   4 +
>  fs/Makefile              |   1 +
>  fs/binfmt_elf.c          |  13 ++
>  fs/gnu_property.c        | 363 +++++++++++++++++++++++++++++++++++++++
>  include/linux/elf.h      |  12 ++
>  include/uapi/linux/elf.h |   8 +
>  6 files changed, 401 insertions(+)
>  create mode 100644 fs/gnu_property.c
> 
> diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt
> index b795f8da81f3..175a1f58e785 100644
> --- a/fs/Kconfig.binfmt
> +++ b/fs/Kconfig.binfmt
> @@ -35,6 +35,10 @@ config COMPAT_BINFMT_ELF
>  config ARCH_BINFMT_ELF_STATE
>  	bool
>  
> +config ARCH_USE_GNU_PROPERTY
> +	bool
> +	depends on 64BIT
> +
>  config BINFMT_ELF_FDPIC
>  	bool "Kernel support for FDPIC ELF binaries"
>  	default y if !BINFMT_ELF
> diff --git a/fs/Makefile b/fs/Makefile
> index 427fec226fae..8a35abbebf8b 100644
> --- a/fs/Makefile
> +++ b/fs/Makefile
> @@ -44,6 +44,7 @@ obj-$(CONFIG_BINFMT_ELF)	+= binfmt_elf.o
>  obj-$(CONFIG_COMPAT_BINFMT_ELF)	+= compat_binfmt_elf.o
>  obj-$(CONFIG_BINFMT_ELF_FDPIC)	+= binfmt_elf_fdpic.o
>  obj-$(CONFIG_BINFMT_FLAT)	+= binfmt_flat.o
> +obj-$(CONFIG_ARCH_USE_GNU_PROPERTY) += gnu_property.o
>  
>  obj-$(CONFIG_FS_MBCACHE)	+= mbcache.o
>  obj-$(CONFIG_FS_POSIX_ACL)	+= posix_acl.o
> 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);
> +	}
> +
> +	if (retval < 0)
> +		goto out_free_dentry;
> +
>  	if (elf_interpreter) {
>  		unsigned long interp_map_addr = 0;
>  
> diff --git a/fs/gnu_property.c b/fs/gnu_property.c
> new file mode 100644
> index 000000000000..656ea3951840
> --- /dev/null
> +++ b/fs/gnu_property.c
> @@ -0,0 +1,363 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Extract an ELF file's .note.gnu.property.
> + *
> + * The path from the ELF header to the note section is the following:
> + * elfhdr->elf_phdr->elf_note->property[].
> + */
> +
> +#include <uapi/linux/elf-em.h>
> +#include <linux/processor.h>
> +#include <linux/binfmts.h>
> +#include <linux/elf.h>
> +#include <linux/slab.h>
> +#include <linux/fs.h>
> +#include <linux/uaccess.h>
> +#include <linux/string.h>
> +#include <linux/compat.h>
> +
> +/*
> + * The .note.gnu.property layout:
> + *
> + *	struct elf_note {
> + *		u32 n_namesz; --> sizeof(n_name[]); always (4)
> + *		u32 n_ndescsz;--> sizeof(property[])
> + *		u32 n_type;   --> always NT_GNU_PROPERTY_TYPE_0
> + *	};
> + *	char n_name[4]; --> always 'GNU\0'
> + *
> + *	struct {
> + *		struct gnu_property {
> + *			u32 pr_type;
> + *			u32 pr_datasz;
> + *		};
> + *		u8 pr_data[pr_datasz];
> + *	}[];
> + */
> +
> +#define BUF_SIZE (PAGE_SIZE / 4)
> +
> +struct gnu_property {
> +	u32 pr_type;
> +	u32 pr_datasz;
> +};
> +
> +typedef bool (test_item_fn)(void *buf, u32 *arg, u32 type);
> +typedef void *(next_item_fn)(void *buf, u32 *arg, u32 type);
> +
> +static inline bool test_note_type(void *buf, u32 *align, u32 note_type)
> +{
> +	struct elf_note *n = buf;
> +
> +	return ((n->n_type == note_type) && (n->n_namesz == 4) &&
> +		(memcmp(n + 1, "GNU", 4) == 0));
> +}
> +
> +static inline void *next_note(void *buf, u32 *align, u32 note_type)
> +{
> +	struct elf_note *n = buf;
> +	u64 size;
> +
> +	if (check_add_overflow((u64)sizeof(*n), (u64)n->n_namesz, &size))
> +		return NULL;
> +
> +	size = round_up(size, *align);
> +
> +	if (check_add_overflow(size, (u64)n->n_descsz, &size))
> +		return NULL;
> +
> +	size = round_up(size, *align);
> +
> +	if (buf + size < buf)
> +		return NULL;
> +	else
> +		return (buf + size);
> +}
> +
> +static inline bool test_property(void *buf, u32 *max_type, u32 pr_type)
> +{
> +	struct gnu_property *pr = buf;
> +
> +	/*
> +	 * Property types must be in ascending order.
> +	 * Keep track of the max when testing each.
> +	 */
> +	if (pr->pr_type > *max_type)
> +		*max_type = pr->pr_type;
> +
> +	return (pr->pr_type == pr_type);
> +}
> +
> +static inline void *next_property(void *buf, u32 *max_type, u32 pr_type)
> +{
> +	struct gnu_property *pr = buf;
> +
> +	if ((buf + sizeof(*pr) +  pr->pr_datasz < buf) ||
> +	    (pr->pr_type > pr_type) ||
> +	    (pr->pr_type > *max_type))
> +		return NULL;
> +	else
> +		return (buf + sizeof(*pr) + pr->pr_datasz);
> +}
> +
> +/*
> + * Scan 'buf' for a pattern; return true if found.
> + * *pos is the distance from the beginning of buf to where
> + * the searched item or the next item is located.
> + */
> +static int scan(u8 *buf, u32 buf_size, int item_size, test_item_fn test_item,
> +		next_item_fn next_item, u32 *arg, u32 type, u32 *pos)
> +{
> +	int found = 0;
> +	u8 *p, *max;
> +
> +	max = buf + buf_size;
> +	if (max < buf)
> +		return 0;
> +
> +	p = buf;
> +
> +	while ((p + item_size < max) && (p + item_size > buf)) {
> +		if (test_item(p, arg, type)) {
> +			found = 1;
> +			break;
> +		}
> +
> +		p = next_item(p, arg, type);
> +	}
> +
> +	*pos = (p + item_size <= buf) ? 0 : (u32)(p - buf);
> +	return found;
> +}
> +
> +/*
> + * Search an NT_GNU_PROPERTY_TYPE_0 for the property of 'pr_type'.
> + */
> +static int find_property(struct file *file, unsigned long desc_size,
> +			 loff_t file_offset, u8 *buf,
> +			 u32 pr_type, u32 *property)
> +{
> +	u32 buf_pos;
> +	unsigned long read_size;
> +	unsigned long done;
> +	int found = 0;
> +	int ret = 0;
> +	u32 last_pr = 0;
> +
> +	*property = 0;
> +	buf_pos = 0;
> +
> +	for (done = 0; done < desc_size; done += buf_pos) {
> +		read_size = desc_size - done;
> +		if (read_size > BUF_SIZE)
> +			read_size = BUF_SIZE;
> +
> +		ret = kernel_read(file, buf, read_size, &file_offset);
> +
> +		if (ret != read_size)
> +			return (ret < 0) ? ret : -EIO;
> +
> +		ret = 0;
> +		found = scan(buf, read_size, sizeof(struct gnu_property),
> +			     test_property, next_property,
> +			     &last_pr, pr_type, &buf_pos);
> +
> +		if ((!buf_pos) || found)
> +			break;
> +
> +		file_offset += buf_pos - read_size;
> +	}
> +
> +	if (found) {
> +		struct gnu_property *pr =
> +			(struct gnu_property *)(buf + buf_pos);
> +
> +		if (pr->pr_datasz == 4) {
> +			u32 *max =  (u32 *)(buf + read_size);
> +			u32 *data = (u32 *)((u8 *)pr + sizeof(*pr));
> +
> +			if (data + 1 <= max) {
> +				*property = *data;
> +			} else {
> +				file_offset += buf_pos - read_size;
> +				file_offset += sizeof(*pr);
> +				ret = kernel_read(file, property, 4,
> +						  &file_offset);
> +			}
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +/*
> + * Search a PT_NOTE segment for NT_GNU_PROPERTY_TYPE_0.
> + */
> +static int find_note_type_0(struct file *file, loff_t file_offset,
> +			    unsigned long note_size, u32 align,
> +			    u32 pr_type, u32 *property)
> +{
> +	u8 *buf;
> +	u32 buf_pos;
> +	unsigned long read_size;
> +	unsigned long done;
> +	int found = 0;
> +	int ret = 0;
> +
> +	buf = kmalloc(BUF_SIZE, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	*property = 0;
> +	buf_pos = 0;
> +
> +	for (done = 0; done < note_size; done += buf_pos) {
> +		read_size = note_size - done;
> +		if (read_size > BUF_SIZE)
> +			read_size = BUF_SIZE;
> +
> +		ret = kernel_read(file, buf, read_size, &file_offset);
> +
> +		if (ret != read_size) {
> +			ret = (ret < 0) ? ret : -EIO;
> +			kfree(buf);
> +			return ret;
> +		}
> +
> +		/*
> +		 * item_size = sizeof(struct elf_note) + elf_note.n_namesz.
> +		 * n_namesz is 4 for the note type we look for.
> +		 */
> +		ret = scan(buf, read_size, sizeof(struct elf_note) + 4,
> +			      test_note_type, next_note,
> +			      &align, NT_GNU_PROPERTY_TYPE_0, &buf_pos);
> +
> +		file_offset += buf_pos - read_size;
> +
> +		if (ret && !found) {
> +			struct elf_note *n =
> +				(struct elf_note *)(buf + buf_pos);
> +			u64 start = round_up(sizeof(*n) + n->n_namesz, align);
> +			u64 total = 0;
> +
> +			if (check_add_overflow(start, (u64)n->n_descsz, &total)) {
> +				ret = -EINVAL;
> +				break;
> +			}
> +			total = round_up(total, align);
> +
> +			ret = find_property(file, n->n_descsz,
> +					    file_offset + start,
> +					    buf, pr_type, property);
> +			found++;
> +			file_offset += total;
> +			buf_pos += total;
> +		} else if (!buf_pos || ret) {
> +			ret = 0;
> +			*property = 0;
> +			break;
> +		}
> +	}
> +
> +	kfree(buf);
> +	return ret;
> +}
> +
> +/*
> + * Look at an ELF file's PT_NOTE segments, then NT_GNU_PROPERTY_TYPE_0, then
> + * the property of pr_type.
> + *
> + * Input:
> + *	file: the file to search;
> + *	phdr: the file's elf header;
> + *	phnum: number of entries in phdr;
> + *	pr_type: the property type.
> + *
> + * Output:
> + *	The property found.
> + *
> + * Return:
> + *	Zero or error.
> + */
> +static int scan_segments_64(struct file *file, struct elf64_phdr *phdr,
> +			    int phnum, u32 pr_type, u32 *property)
> +{
> +	int i;
> +	int err = 0;
> +
> +	for (i = 0; i < phnum; i++, phdr++) {
> +		if ((phdr->p_type != PT_NOTE) || (phdr->p_align != 8))
> +			continue;
> +
> +		/*
> +		 * Search the PT_NOTE segment for NT_GNU_PROPERTY_TYPE_0.
> +		 */
> +		err = find_note_type_0(file, phdr->p_offset, phdr->p_filesz,
> +				       phdr->p_align, pr_type, property);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_COMPAT
> +static int scan_segments_32(struct file *file, struct elf32_phdr *phdr,
> +			    int phnum, u32 pr_type, u32 *property)
> +{
> +	int i;
> +	int err = 0;
> +
> +	for (i = 0; i < phnum; i++, phdr++) {
> +		if ((phdr->p_type != PT_NOTE) || (phdr->p_align != 4))
> +			continue;
> +
> +		/*
> +		 * Search the PT_NOTE segment for NT_GNU_PROPERTY_TYPE_0.
> +		 */
> +		err = find_note_type_0(file, phdr->p_offset, phdr->p_filesz,
> +				       phdr->p_align, pr_type, property);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +#endif
> +
> +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->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
> +	}
> +
> +out:
> +	return err;
> +}
> diff --git a/include/linux/elf.h b/include/linux/elf.h
> index e3649b3e970e..c15febebe7f2 100644
> --- a/include/linux/elf.h
> +++ b/include/linux/elf.h
> @@ -56,4 +56,16 @@ static inline int elf_coredump_extra_notes_write(struct coredump_params *cprm) {
>  extern int elf_coredump_extra_notes_size(void);
>  extern int elf_coredump_extra_notes_write(struct coredump_params *cprm);
>  #endif
> +
> +#ifdef CONFIG_ARCH_USE_GNU_PROPERTY
> +extern int arch_setup_property(void *ehdr, void *phdr, struct file *f,
> +			       bool interp);
> +extern int get_gnu_property(void *ehdr_p, void *phdr_p, struct file *f,
> +			    u32 pr_type, u32 *feature);
> +#else
> +static inline int arch_setup_property(void *ehdr, void *phdr, struct file *f,
> +				      bool interp) { return 0; }
> +static inline int get_gnu_property(void *ehdr_p, void *phdr_p, struct file *f,
> +				   u32 pr_type, u32 *feature) { return 0; }
> +#endif
>  #endif /* _LINUX_ELF_H */
> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
> index 34c02e4290fe..7b7603a44cbc 100644
> --- a/include/uapi/linux/elf.h
> +++ b/include/uapi/linux/elf.h
> @@ -372,6 +372,7 @@ typedef struct elf64_shdr {
>  #define NT_PRFPREG	2
>  #define NT_PRPSINFO	3
>  #define NT_TASKSTRUCT	4
> +#define NT_GNU_PROPERTY_TYPE_0 5
>  #define NT_AUXV		6
>  /*
>   * Note to userspace developers: size of NT_SIGINFO note may increase
> @@ -443,4 +444,11 @@ typedef struct elf64_note {
>    Elf64_Word n_type;	/* Content type */
>  } Elf64_Nhdr;
>  
> +/* .note.gnu.property types */
> +#define GNU_PROPERTY_X86_FEATURE_1_AND		(0xc0000002)
> +
> +/* Bits of GNU_PROPERTY_X86_FEATURE_1_AND */
> +#define GNU_PROPERTY_X86_FEATURE_1_IBT		(0x00000001)
> +#define GNU_PROPERTY_X86_FEATURE_1_SHSTK	(0x00000002)
> +
>  #endif /* _UAPI_LINUX_ELF_H */
> -- 
> 2.17.1
> 

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

[Dave Martin]

On Thu, May 02, 2019 at 12:10:04PM +0100, Dave Martin wrote:
> On Wed, May 01, 2019 at 02:12:17PM -0700, Yu-cheng Yu wrote:
> > An ELF file's .note.gnu.property indicates features the executable file
> > can support.  For example, the property GNU_PROPERTY_X86_FEATURE_1_AND
> > indicates the file supports GNU_PROPERTY_X86_FEATURE_1_IBT and/or
> > GNU_PROPERTY_X86_FEATURE_1_SHSTK.
> > 
> > This patch was part of the Control-flow Enforcement series; the original
> > patch is here: https://lkml.org/lkml/2018/11/20/205.  Dave Martin responded
> > that ARM recently introduced new features to NT_GNU_PROPERTY_TYPE_0 with
> > properties closely modelled on GNU_PROPERTY_X86_FEATURE_1_AND, and it is
> > logical to split out the generic part.  Here it is.
> > 
> > With this patch, if an arch needs to setup features from ELF properties,
> > it needs CONFIG_ARCH_USE_GNU_PROPERTY to be set, and a specific
> > arch_setup_property().
> > 
> > For example, for X86_64:
> > 
> > int arch_setup_property(void *ehdr, void *phdr, struct file *f, bool inter)
> > {
> > 	int r;
> > 	uint32_t property;
> > 
> > 	r = get_gnu_property(ehdr, phdr, f, GNU_PROPERTY_X86_FEATURE_1_AND,
> > 			     &property);
> > 	...
> > }

[...]

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

[...]

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

[...]

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

[...]

Cheers
---DavE

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

[Yu-cheng Yu]

On Thu, 2019-05-02 at 12:10 +0100, Dave Martin wrote:
> On Wed, May 01, 2019 at 02:12:17PM -0700, Yu-cheng Yu wrote:
> > An ELF file's .note.gnu.property indicates features the executable file
> > can support.  For example, the property GNU_PROPERTY_X86_FEATURE_1_AND
> > indicates the file supports GNU_PROPERTY_X86_FEATURE_1_IBT and/or
> > GNU_PROPERTY_X86_FEATURE_1_SHSTK.

[...]
> A couple of questions before I look in more detail:
> 
> 1) Can we rely on PT_GNU_PROPERTY being present in the phdrs to describe
> the NT_GNU_PROPERTY_TYPE_0 note?  If so, we can avoid trying to parse
> irrelevant PT_NOTE segments.

Some older linkers can create multiples of NT_GNU_PROPERTY_TYPE_0.  The code
scans all PT_NOTE segments to ensure there is only one NT_GNU_PROPERTY_TYPE_0. 
If there are multiples, then all are considered invalid.

> 
> 
> 2) Are there standard types for things like the program property header?
> If not, can we add something in elf.h?  We should try to coordinate with
> libc on that.  Something like
> 
> typedef __u32 Elf_Word;
> 
> typedef struct {
> 	Elf_Word pr_type;
> 	Elf_Word pr_datasz;
> } Elf_Gnu_Prophdr;
> 
> (i.e., just the header part from [1], with a more specific name -- which
> I just made up).

Yes, I will fix that.

[...]
> 3) It looks like we have to go and re-parse all the notes for every
> property requested by the arch code.

As explained above, it is necessary to scan all PT_NOTE segments.  But there
should be only one NT_GNU_PROPERTY_TYPE_0 in an ELF file.  Once that is found,
perhaps we can store it somewhere, or call into the arch code as you mentioned
below.  I will look into that.

> 
> For now there is only one property requested anyway, so this is probably
> not too bad.  But could we flip things around so that we have some
> CONFIG_ARCH_WANTS_ELF_GNU_PROPERTY (say), and have the ELF core code
> call into the arch backend for each property found?
> 
> The arch could provide some hook
> 
> 	int arch_elf_has_gnu_property(const Elf_Gnu_Prophdr *prop,
> 					const void *data);
> 
> to consume the properties as they are found.
> 
> This would effectively replace the arch_setup_property() hook you
> currently have.
> 
> Cheers
> ---Dave
> 
> [1] https://github.com/hjl-tools/linux-abi/wiki/Linux-Extensions-to-gABI

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

[Yu-cheng Yu]

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:
> > On Wed, May 01, 2019 at 02:12:17PM -0700, Yu-cheng Yu wrote:
> 
> [...]
> 
> > > 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.

> 
> [...]
> 
> > > diff --git a/fs/gnu_property.c b/fs/gnu_property.c
> 
> [...]
> 
> > > +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-
> > > >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

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

[Dave Martin]

On Thu, May 02, 2019 at 08:47:06AM -0700, Yu-cheng Yu wrote:
> On Thu, 2019-05-02 at 12:10 +0100, Dave Martin wrote:
> > On Wed, May 01, 2019 at 02:12:17PM -0700, Yu-cheng Yu wrote:
> > > An ELF file's .note.gnu.property indicates features the executable file
> > > can support.  For example, the property GNU_PROPERTY_X86_FEATURE_1_AND
> > > indicates the file supports GNU_PROPERTY_X86_FEATURE_1_IBT and/or
> > > GNU_PROPERTY_X86_FEATURE_1_SHSTK.
> 
> [...]
> > A couple of questions before I look in more detail:
> > 
> > 1) Can we rely on PT_GNU_PROPERTY being present in the phdrs to describe
> > the NT_GNU_PROPERTY_TYPE_0 note?  If so, we can avoid trying to parse
> > irrelevant PT_NOTE segments.
> 
> Some older linkers can create multiples of NT_GNU_PROPERTY_TYPE_0.  The code
> scans all PT_NOTE segments to ensure there is only one NT_GNU_PROPERTY_TYPE_0. 
> If there are multiples, then all are considered invalid.

I'm concerned that in the arm64 case we would waste some effort by
scanning multiple notes.

Could we do something like iterating over the phdrs, and if we find
exactly one PT_GNU_PROPERTY then use that, else fall back to scanning
all PT_NOTEs?

> > 2) Are there standard types for things like the program property header?
> > If not, can we add something in elf.h?  We should try to coordinate with
> > libc on that.  Something like
> > 
> > typedef __u32 Elf_Word;
> > 
> > typedef struct {
> > 	Elf_Word pr_type;
> > 	Elf_Word pr_datasz;
> > } Elf_Gnu_Prophdr;
> > 
> > (i.e., just the header part from [1], with a more specific name -- which
> > I just made up).
> 
> Yes, I will fix that.
> 
> [...]
> > 3) It looks like we have to go and re-parse all the notes for every
> > property requested by the arch code.
> 
> As explained above, it is necessary to scan all PT_NOTE segments.  But there
> should be only one NT_GNU_PROPERTY_TYPE_0 in an ELF file.  Once that is found,
> perhaps we can store it somewhere, or call into the arch code as you mentioned
> below.  I will look into that.

Just to get something working on arm64, I'm working on some hacks that
move things around a bit -- I'll post when I have something.

Did you have any view on my other point, below?

Cheers
---Dave

> > For now there is only one property requested anyway, so this is probably
> > not too bad.  But could we flip things around so that we have some
> > CONFIG_ARCH_WANTS_ELF_GNU_PROPERTY (say), and have the ELF core code
> > call into the arch backend for each property found?
> > 
> > The arch could provide some hook
> > 
> > 	int arch_elf_has_gnu_property(const Elf_Gnu_Prophdr *prop,
> > 					const void *data);
> > 
> > to consume the properties as they are found.
> > 
> > This would effectively replace the arch_setup_property() hook you
> > currently have.
> > 
> > Cheers
> > ---Dave
> > 
> > [1] https://github.com/hjl-tools/linux-abi/wiki/Linux-Extensions-to-gABI
> 

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

[Yu-cheng Yu]

On Thu, 2019-05-02 at 17:14 +0100, Dave Martin wrote:
> On Thu, May 02, 2019 at 08:47:06AM -0700, Yu-cheng Yu wrote:
> > On Thu, 2019-05-02 at 12:10 +0100, Dave Martin wrote:
> > > On Wed, May 01, 2019 at 02:12:17PM -0700, Yu-cheng Yu wrote:
> > > > An ELF file's .note.gnu.property indicates features the executable file
> > > > can support.  For example, the property GNU_PROPERTY_X86_FEATURE_1_AND
> > > > indicates the file supports GNU_PROPERTY_X86_FEATURE_1_IBT and/or
> > > > GNU_PROPERTY_X86_FEATURE_1_SHSTK.
> > 
> > [...]
> > > A couple of questions before I look in more detail:
> > > 
> > > 1) Can we rely on PT_GNU_PROPERTY being present in the phdrs to describe
> > > the NT_GNU_PROPERTY_TYPE_0 note?  If so, we can avoid trying to parse
> > > irrelevant PT_NOTE segments.
> > 
> > Some older linkers can create multiples of NT_GNU_PROPERTY_TYPE_0.  The code
> > scans all PT_NOTE segments to ensure there is only one
> > NT_GNU_PROPERTY_TYPE_0. 
> > If there are multiples, then all are considered invalid.
> 
> I'm concerned that in the arm64 case we would waste some effort by
> scanning multiple notes.
> 
> Could we do something like iterating over the phdrs, and if we find
> exactly one PT_GNU_PROPERTY then use that, else fall back to scanning
> all PT_NOTEs?

That makes sense to me, but the concern is that we don't know the
PT_GNU_PROPERTY the only one.  This probably needs to be discussed with more
people.

> > > 2) Are there standard types for things like the program property header?
> > > If not, can we add something in elf.h?  We should try to coordinate with
> > > libc on that.  Something like
> > > 
> > > typedef __u32 Elf_Word;
> > > 
> > > typedef struct {
> > > 	Elf_Word pr_type;
> > > 	Elf_Word pr_datasz;
> > > } Elf_Gnu_Prophdr;
> > > 
> > > (i.e., just the header part from [1], with a more specific name -- which
> > > I just made up).
> > 
> > Yes, I will fix that.
> > 
> > [...]
> > > 3) It looks like we have to go and re-parse all the notes for every
> > > property requested by the arch code.
> > 
> > As explained above, it is necessary to scan all PT_NOTE segments.  But there
> > should be only one NT_GNU_PROPERTY_TYPE_0 in an ELF file.  Once that is
> > found,
> > perhaps we can store it somewhere, or call into the arch code as you
> > mentioned
> > below.  I will look into that.
> 
> Just to get something working on arm64, I'm working on some hacks that
> move things around a bit -- I'll post when I have something.
> 
> Did you have any view on my other point, below?

That should work.  I will also make some changes for that.

> 
> Cheers
> ---Dave
> 
> > > For now there is only one property requested anyway, so this is probably
> > > not too bad.  But could we flip things around so that we have some
> > > CONFIG_ARCH_WANTS_ELF_GNU_PROPERTY (say), and have the ELF core code
> > > call into the arch backend for each property found?
> > > 
> > > The arch could provide some hook
> > > 
> > > 	int arch_elf_has_gnu_property(const Elf_Gnu_Prophdr *prop,
> > > 					const void *data);
> > > 
> > > to consume the properties as they are found.
> > > 
> > > This would effectively replace the arch_setup_property() hook you
> > > currently have.
> > > 
> > > Cheers
> > > ---Dave
> > > 
> > > [1] https://github.com/hjl-tools/linux-abi/wiki/Linux-Extensions-to-gABI

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

[Andy Lutomirski]

On Thu, May 2, 2019 at 4:10 AM Dave Martin <Dave.Martin@arm.com> wrote:
>
> On Wed, May 01, 2019 at 02:12:17PM -0700, Yu-cheng Yu wrote:
> > An ELF file's .note.gnu.property indicates features the executable file
> > can support.  For example, the property GNU_PROPERTY_X86_FEATURE_1_AND
> > indicates the file supports GNU_PROPERTY_X86_FEATURE_1_IBT and/or
> > GNU_PROPERTY_X86_FEATURE_1_SHSTK.
> >
> > This patch was part of the Control-flow Enforcement series; the original
> > patch is here: https://lkml.org/lkml/2018/11/20/205.  Dave Martin responded
> > that ARM recently introduced new features to NT_GNU_PROPERTY_TYPE_0 with
> > properties closely modelled on GNU_PROPERTY_X86_FEATURE_1_AND, and it is
> > logical to split out the generic part.  Here it is.
> >
> > With this patch, if an arch needs to setup features from ELF properties,
> > it needs CONFIG_ARCH_USE_GNU_PROPERTY to be set, and a specific
> > arch_setup_property().
> >
> > For example, for X86_64:
> >
> > int arch_setup_property(void *ehdr, void *phdr, struct file *f, bool inter)
> > {
> >       int r;
> >       uint32_t property;
> >
> >       r = get_gnu_property(ehdr, phdr, f, GNU_PROPERTY_X86_FEATURE_1_AND,
> >                            &property);
> >       ...
> > }
>
> Thanks, this is timely for me.  I should be able to build the needed
> arm64 support pretty quickly around this now.
>
> [Cc'ing libc-alpha for the elf.h question -- see (2)]
>
>
> A couple of questions before I look in more detail:
>
> 1) Can we rely on PT_GNU_PROPERTY being present in the phdrs to describe
> the NT_GNU_PROPERTY_TYPE_0 note?  If so, we can avoid trying to parse
> irrelevant PT_NOTE segments.
>
>
> 2) Are there standard types for things like the program property header?
> If not, can we add something in elf.h?  We should try to coordinate with
> libc on that.  Something like
>

Where did PT_GNU_PROPERTY come from?  Are there actual docs for it?
Can someone here tell us what the actual semantics of this new ELF
thingy are?  From some searching, it seems like it's kind of an ELF
note but kind of not.  An actual description would be fantastic.

Also, I don't think there's any actual requirement that the upstream
kernel recognize existing CET-enabled RHEL 8 binaries as being
CET-enabled.  I tend to think that RHEL 8 jumped the gun here.  While
the upstream kernel should make some reasonble effort to make sure
that RHEL 8 binaries will continue to run, I don't see why we need to
go out of our way to keep the full set of mitigations available for
binaries that were developed against a non-upstream kernel.

In fact, if we handle the legacy bitmap differently from RHEL 8, we
may *have* to make sure that we don't recognize existing RHEL 8
binaries as CET-enabled.

Sigh.

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

[Yu-cheng Yu]

On Wed, 2019-06-26 at 10:14 -0700, Andy Lutomirski wrote:
> On Thu, May 2, 2019 at 4:10 AM Dave Martin <Dave.Martin@arm.com> wrote:
> > 
> > On Wed, May 01, 2019 at 02:12:17PM -0700, Yu-cheng Yu wrote:
> > > An ELF file's .note.gnu.property indicates features the executable file
> > > can support.  For example, the property GNU_PROPERTY_X86_FEATURE_1_AND
> > > indicates the file supports GNU_PROPERTY_X86_FEATURE_1_IBT and/or
> > > GNU_PROPERTY_X86_FEATURE_1_SHSTK.
> > > 
[...]
> 
> Where did PT_GNU_PROPERTY come from?  Are there actual docs for it?
> Can someone here tell us what the actual semantics of this new ELF
> thingy are?  From some searching, it seems like it's kind of an ELF
> note but kind of not.  An actual description would be fantastic.
> 
> Also, I don't think there's any actual requirement that the upstream
> kernel recognize existing CET-enabled RHEL 8 binaries as being
> CET-enabled.  I tend to think that RHEL 8 jumped the gun here.  While
> the upstream kernel should make some reasonble effort to make sure
> that RHEL 8 binaries will continue to run, I don't see why we need to
> go out of our way to keep the full set of mitigations available for
> binaries that were developed against a non-upstream kernel.
> 
> In fact, if we handle the legacy bitmap differently from RHEL 8, we
> may *have* to make sure that we don't recognize existing RHEL 8
> binaries as CET-enabled.

We have worked out the issue.  Linux will look at only PT_GNU_PROPERTY, which is
a shortcut pointing directly to .note.gnu.property.  I have an updated patch,
and will send it out (although it is not yet perfect).

The Linux gABI extension draft is here: https://github.com/hjl-tools/linux-abi/w
iki/linux-abi-draft.pdf.

Yu-cheng

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

[Dave Martin]

On Wed, Jun 26, 2019 at 10:14:07AM -0700, Andy Lutomirski wrote:
> On Thu, May 2, 2019 at 4:10 AM Dave Martin <Dave.Martin@arm.com> wrote:

[...]

> > A couple of questions before I look in more detail:
> >
> > 1) Can we rely on PT_GNU_PROPERTY being present in the phdrs to describe
> > the NT_GNU_PROPERTY_TYPE_0 note?  If so, we can avoid trying to parse
> > irrelevant PT_NOTE segments.
> >
> >
> > 2) Are there standard types for things like the program property header?
> > If not, can we add something in elf.h?  We should try to coordinate with
> > libc on that.  Something like
> >
> 
> Where did PT_GNU_PROPERTY come from?  Are there actual docs for it?
> Can someone here tell us what the actual semantics of this new ELF
> thingy are?  From some searching, it seems like it's kind of an ELF
> note but kind of not.  An actual description would be fantastic.

https://github.com/hjl-tools/linux-abi/wiki/linux-abi-draft.pdf

I don't know _when_ it was added, and the description is minimal, but
it's there.

(I'd say it's fairly obvious how it should be used, but it could do with
some clarification...)

> Also, I don't think there's any actual requirement that the upstream
> kernel recognize existing CET-enabled RHEL 8 binaries as being
> CET-enabled.  I tend to think that RHEL 8 jumped the gun here.  While
> the upstream kernel should make some reasonble effort to make sure
> that RHEL 8 binaries will continue to run, I don't see why we need to
> go out of our way to keep the full set of mitigations available for
> binaries that were developed against a non-upstream kernel.

If that's an accpetable approach, it should certainly make our life
easier.

> In fact, if we handle the legacy bitmap differently from RHEL 8, we
> may *have* to make sure that we don't recognize existing RHEL 8
> binaries as CET-enabled.

Can't comment on that.  If the existing RHEL 8 binaries strictly don't
have the PT_GNU_PROPERTY phdr, then this might serve a dual purpose ...
otherwise, x86 might need some additional annotation for new binaries.

I'll leave it for others to comment.

Cheers
---Dave

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

[Florian Weimer]

* Andy Lutomirski:

> Also, I don't think there's any actual requirement that the upstream
> kernel recognize existing CET-enabled RHEL 8 binaries as being
> CET-enabled.  I tend to think that RHEL 8 jumped the gun here.

The ABI was supposed to be finalized and everyone involved thought it
had been reviewed by the GNU gABI community and other interested
parties.  It had been included in binutils for several releases.

From my point of view, the kernel is just a consumer of the ABI.  The
kernel would not change an instruction encoding if it doesn't like it
for some reason, either.

> While the upstream kernel should make some reasonble effort to make
> sure that RHEL 8 binaries will continue to run, I don't see why we
> need to go out of our way to keep the full set of mitigations
> available for binaries that were developed against a non-upstream
> kernel.

They were developed against the ABI specification.

I do not have a strong opinion what the kernel should do going forward.
I just want to make clear what happened.

Thanks,
Florian

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

[Andy Lutomirski]

On Thu, Jun 27, 2019 at 2:39 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Andy Lutomirski:
>
> > Also, I don't think there's any actual requirement that the upstream
> > kernel recognize existing CET-enabled RHEL 8 binaries as being
> > CET-enabled.  I tend to think that RHEL 8 jumped the gun here.
>
> The ABI was supposed to be finalized and everyone involved thought it
> had been reviewed by the GNU gABI community and other interested
> parties.  It had been included in binutils for several releases.
>
> From my point of view, the kernel is just a consumer of the ABI.  The
> kernel would not change an instruction encoding if it doesn't like it
> for some reason, either.

I read the only relevant gABI thing I could find easily, and it seems
to document the "gnu property" thing.  I have no problem with that.

>
> > While the upstream kernel should make some reasonble effort to make
> > sure that RHEL 8 binaries will continue to run, I don't see why we
> > need to go out of our way to keep the full set of mitigations
> > available for binaries that were developed against a non-upstream
> > kernel.
>
> They were developed against the ABI specification.
>
> I do not have a strong opinion what the kernel should do going forward.
> I just want to make clear what happened.

I admit that I'm not really clear on exactly what RHEL 8 shipped.
Some of this stuff is very much an ELF ABI that belongs to the
toolchain, but some if it is kernel API.  For example, the IBT legacy
bitmap API is very much in flux, and I don't think anything credible
has been submitted for upstream inclusion.  Does RHEL 8's glibc
attempt to cope with the case where some libraries are CET-compatible
and some are not?  If so, how does this work?  What, if any, services
does the RHEL 8 kernel provide in this direction?