Re: [RFC PATCH 2/2] ELF: Add ELF program property parsing support

[Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx>]

On Tue, 2019-08-20 at 10:57 +0100, Dave Martin wrote:
> ELF program properties will needed for detecting whether to enable
> optional architecture or ABI features for a new ELF process.
> 
> For now, there are no generic properties that we care about, so do
> nothing unless CONFIG_ARCH_USE_GNU_PROPERTY=y.
> 
> Otherwise, the presence of properties using the PT_PROGRAM_PROPERTY
> phdrs entry (if any), and notify each property to the arch code.
> 
> For now, the added code is not used.
> 
> Signed-off-by: Dave Martin <Dave.Martin@xxxxxxx>
> ---
>  fs/binfmt_elf.c          | 109
> +++++++++++++++++++++++++++++++++++++++++++++++
>  fs/compat_binfmt_elf.c   |   4 ++
>  include/linux/elf.h      |  21 +++++++++
>  include/uapi/linux/elf.h |   4 ++
>  4 files changed, 138 insertions(+)
> 
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index d4e11b2..52f4b96 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -39,12 +39,18 @@
>  #include <linux/sched/coredump.h>
>  #include <linux/sched/task_stack.h>
>  #include <linux/sched/cputime.h>
> +#include <linux/sizes.h>
> +#include <linux/types.h>
>  #include <linux/cred.h>
>  #include <linux/dax.h>
>  #include <linux/uaccess.h>
>  #include <asm/param.h>
>  #include <asm/page.h>
>  
> +#ifndef ELF_COMPAT
> +#define ELF_COMPAT 0
> +#endif
> +
>  #ifndef user_long_t
>  #define user_long_t long
>  #endif
> @@ -690,6 +696,93 @@ static unsigned long randomize_stack_top(unsigned long
> stack_top)
>  #endif
>  }
>  
> +static int parse_elf_property(const void **prop, size_t *notesz,
> +			      struct arch_elf_state *arch)
> +{
> +	const struct gnu_property *pr = *prop;
> +	size_t sz = *notesz;
> +	int ret;
> +	size_t step;
> +
> +	BUG_ON(sz < sizeof(*pr));
> +
> +	if (sizeof(*pr) > sz)
> +		return -EIO;
> +
> +	if (pr->pr_datasz > sz - sizeof(*pr))
> +		return -EIO;
> +
> +	step = round_up(sizeof(*pr) + pr->pr_datasz, elf_gnu_property_align);
> +	if (step > sz)
> +		return -EIO;
> +
> +	ret = arch_parse_elf_property(pr->pr_type, *prop + sizeof(*pr),
> +				      pr->pr_datasz, ELF_COMPAT, arch);
> +	if (ret)
> +		return ret;
> +
> +	*prop += step;
> +	*notesz -= step;
> +	return 0;
> +}
> +
> +#define NOTE_DATA_SZ SZ_1K
> +#define GNU_PROPERTY_TYPE_0_NAME "GNU"
> +#define NOTE_NAME_SZ (sizeof(GNU_PROPERTY_TYPE_0_NAME))
> +
> +static int parse_elf_properties(struct file *f, const struct elf_phdr *phdr,
> +				struct arch_elf_state *arch)
> +{
> +	ssize_t n;
> +	loff_t pos = phdr->p_offset;
> +	union {
> +		struct elf_note nhdr;
> +		char data[NOTE_DATA_SZ];
> +	} note;
> +	size_t off, notesz;
> +	const void *prop;
> +	int ret;
> +
> +	if (!IS_ENABLED(ARCH_USE_GNU_PROPERTY))
> +		return 0;
> +
> +	BUG_ON(phdr->p_type != PT_GNU_PROPERTY);
> +
> +	/* If the properties are crazy large, that's too bad (for now): */
> +	if (phdr->p_filesz > sizeof(note))
> +		return -ENOEXEC;
> +	n = kernel_read(f, &note, phdr->p_filesz, &pos);
> +
> +	BUILD_BUG_ON(sizeof(note) < sizeof(note.nhdr) + NOTE_NAME_SZ);
> +	if (n < 0 || n < sizeof(note.nhdr) + NOTE_NAME_SZ)
> +		return -EIO;
> +
> +	if (note.nhdr.n_type != NT_GNU_PROPERTY_TYPE_0 ||
> +	    note.nhdr.n_namesz != NOTE_NAME_SZ ||
> +	    strncmp(note.data + sizeof(note.nhdr),
> +		    GNU_PROPERTY_TYPE_0_NAME, n - sizeof(note.nhdr)))
> +		return -EIO;
> +
> +	off = round_up(sizeof(note.nhdr) + NOTE_NAME_SZ,
> +		       elf_gnu_property_align);
> +	if (off > n)
> +		return -EIO;
> +
> +	prop = (const struct gnu_property *)(note.data + off);
> +	notesz = n - off;
> +	if (note.nhdr.n_descsz > notesz)
> +		return -EIO;
> +
> +	while (notesz) {
> +		BUG_ON(((char *)prop - note.data) % elf_gnu_property_align);
> +		ret = parse_elf_property(&prop, &notesz, arch);

Properties need to be in ascending order.  Can we keep track of it from here.
Also, can we replace BUG_ON with returning an error.

Thanks,
Yu-cheng