Message-Id: <01AF458D-1EDC-47D1-A902-A686E2DF26AF@jrtc27.com>
Date:   Tue, 10 Jan 2023 20:48:03 +0000
From:   Jessica Clarke <jrtc27@...c27.com>
To:     Vineet Gupta <vineetg@...osinc.com>
Cc:     linux-riscv <linux-riscv@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        palmer@...osinc.com, Paul Walmsley <paul.walmsley@...ive.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Eric Biederman <ebiederm@...ssion.com>,
        Kees Cook <keescook@...omium.org>, Guo Ren <guoren@...nel.org>,
        Greentime Hu <greentime.hu@...ive.com>,
        Conor Dooley <conor.dooley@...rochip.com>, linux@...osinc.com,
        kernel test robot <lkp@...el.com>
Subject: Re: [PATCH] riscv: elf: add .riscv.attributes parsing

On 10 Jan 2023, at 20:18, Vineet Gupta <vineetg@...osinc.com> wrote:
> 
> This implements the elf loader hook to parse RV specific
> .riscv.attributes section. This section is inserted by compilers
> (gcc/llvm) with build related information such as -march organized as
> tag/value attribute pairs.
> 
> It identifies the various attribute tags (and corresponding values) as
> currently specified in the psABI specification.
> 
> This patch only implements the elf parsing mechanics, leaving out the
> recording/usage of the attributes to subsequent patches.
> 
> Reported-by: kernel test robot <lkp@...el.com>  # code under CONFIG_COMPAT
> Signed-off-by: Vineet Gupta <vineetg@...osinc.com>

This code is full of buffer overruns and uninitialised reads in the
presence of malicious files, and fails to check the version, vendor and
sub-subsection tag.

You also should handle more than one sub-subsection even if tools don’t
emit it today.

You also have an unaligned access for reading the sub-subsection’s data
length (maybe that’s ok in kernel land, but worth making sure).

Jess

> ---
> arch/riscv/Kconfig           |   1 +
> arch/riscv/include/asm/elf.h |  11 +++
> arch/riscv/kernel/Makefile   |   1 +
> arch/riscv/kernel/elf-attr.c | 150 +++++++++++++++++++++++++++++++++++
> 4 files changed, 163 insertions(+)
> create mode 100644 arch/riscv/kernel/elf-attr.c
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index e2b656043abf..f7e0ab05a2d2 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -12,6 +12,7 @@ config 32BIT
> 
> config RISCV
> 	def_bool y
> +	select ARCH_BINFMT_ELF_STATE
> 	select ARCH_CLOCKSOURCE_INIT
> 	select ARCH_ENABLE_HUGEPAGE_MIGRATION if HUGETLB_PAGE && MIGRATION
> 	select ARCH_ENABLE_SPLIT_PMD_PTLOCK if PGTABLE_LEVELS > 2
> diff --git a/arch/riscv/include/asm/elf.h b/arch/riscv/include/asm/elf.h
> index e7acffdf21d2..7ab8bd0ec330 100644
> --- a/arch/riscv/include/asm/elf.h
> +++ b/arch/riscv/include/asm/elf.h
> @@ -116,6 +116,17 @@ do {							\
> 		*(struct user_regs_struct *)regs;	\
> } while (0);
> 
> +struct arch_elf_state {
> +};
> +
> +#define INIT_ARCH_ELF_STATE {}
> +
> +extern int arch_elf_pt_proc(void *ehdr, void *phdr, struct file *elf,
> +			    bool is_interp, struct arch_elf_state *state);
> +
> +extern int arch_check_elf(void *ehdr, bool has_interpreter, void *interp_ehdr,
> +			  struct arch_elf_state *state);
> +
> #ifdef CONFIG_COMPAT
> 
> #define SET_PERSONALITY(ex)					\
> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> index 4cf303a779ab..eff6d845ac9d 100644
> --- a/arch/riscv/kernel/Makefile
> +++ b/arch/riscv/kernel/Makefile
> @@ -50,6 +50,7 @@ obj-y	+= riscv_ksyms.o
> obj-y	+= stacktrace.o
> obj-y	+= cacheinfo.o
> obj-y	+= patch.o
> +obj-y	+= elf-attr.o
> obj-y	+= probes/
> obj-$(CONFIG_MMU) += vdso.o vdso/
> 
> diff --git a/arch/riscv/kernel/elf-attr.c b/arch/riscv/kernel/elf-attr.c
> new file mode 100644
> index 000000000000..148d720f97de
> --- /dev/null
> +++ b/arch/riscv/kernel/elf-attr.c
> @@ -0,0 +1,150 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023-24 Rivos Inc.
> + */
> +
> +#include <linux/binfmts.h>
> +#include <linux/elf.h>
> +
> +#undef pr_fmt
> +#define pr_fmt(fmt) "rv-elf-attr: " fmt
> +
> +#define PT_RISCV_ATTRIBUTES		0x70000003
> +
> +#define RV_ATTR_TAG_stack_align		4
> +#define RV_ATTR_TAG_arch		5
> +#define RV_ATTR_TAG_unaligned_access	6
> +
> +#define RV_ATTR_SEC_SZ			SZ_1K
> +
> +static void rv_elf_attr_int(u64 tag, u64 val)
> +{
> +}
> +
> +static void rv_elf_attr_str(u64 tag, const char *str)
> +{
> +}
> +
> +static u64
> +decode_uleb128(unsigned char **dpp)
> +{
> +	unsigned char *bp = *dpp;
> +	unsigned char byte;
> +	unsigned int shift = 0;
> +	u64 result = 0;
> +
> +	while (1) {
> +		byte = *bp++;
> +		result |= (byte & 0x7f) << shift;
> +		if ((byte & 0x80) == 0)
> +			break;
> +		shift += 7;
> +	}
> +	*dpp = bp;
> +	return result;
> +}
> +
> +/*
> + * Parse .riscv.attributes elf section to get the various compile time
> + * attributes such as -march, unaligned access and so no.
> + */
> +static int rv_parse_elf_attributes(struct file *f, const struct elf_phdr *phdr,
> +				   struct arch_elf_state *state)
> +{
> +	unsigned char buf[RV_ATTR_SEC_SZ];
> +	unsigned char *p;
> +	ssize_t n;
> +	int ret = 0;
> +	loff_t pos;
> +
> +	pr_debug("Section .riscv.attributes found\n");
> +
> +	/* Assume a reasonable size for now */
> +	if (phdr->p_filesz > sizeof(buf))
> +		return -ENOEXEC;
> +
> +	memset(buf, 0, RV_ATTR_SEC_SZ);
> +	pos = phdr->p_offset;
> +	n = kernel_read(f, &buf, phdr->p_filesz, &pos);
> +
> +	if (n < 0)
> +		return -EIO;
> +
> +	p = buf;
> +	p++;				/* format-version (1B) */
> +
> +	while ((p - buf) < n) {
> +
> +		unsigned char *vendor_start;
> +		u32 len;
> +
> +		/*
> +		 * Organized as "vendor" sub-section(s).
> +		 * Current only 1 specified "riscv"
> +		 */
> +
> +		p += 4;			/* sub-section length (4B) */
> +		while (*p++ != '\0')	/* vendor name string */
> +			;
> +		p++;			/* Tag_File (1B) */
> +		len = *(u32 *)p;	/* data length (4B) */
> +		p += 4;
> +
> +		len -= 5;		/* data length includes Tag and self length */
> +		vendor_start = p;
> +		while ((p - vendor_start) < len) {
> +
> +			u64 tag = decode_uleb128(&p);
> +			unsigned char *val_str;
> +			u64 val_n;
> +
> +			switch (tag) {
> +			case RV_ATTR_TAG_stack_align:
> +				val_n = decode_uleb128(&p);
> +				break;
> +
> +			case RV_ATTR_TAG_unaligned_access:
> +				val_n = decode_uleb128(&p);
> +				pr_debug("Tag_RISCV_unaligned_access =%llu\n", val_n);
> +				rv_elf_attr_int(tag, val_n);
> +				break;
> +
> +			case RV_ATTR_TAG_arch:
> +				val_str = p;
> +				while (*p++ != '\0')
> +					;
> +				pr_debug("Tag_RISCV_arch =[%s]\n", val_str);
> +				rv_elf_attr_str(tag, val_str);
> +				break;
> +
> +			default:
> +				val_n = decode_uleb128(&p);
> +				pr_debug("skipping Unrecognized Tag [%llu]=%llu\n", tag, val_n);
> +				break;
> +			}
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +/*
> + * Hook invoked from common elf loader to parse any arch specific elf segments
> + */
> +int arch_elf_pt_proc(void *_ehdr, void *_phdr, struct file *elf,
> +		     bool is_interp, struct arch_elf_state *state)
> +{
> +	struct elf_phdr *phdr = _phdr;
> +	int ret = 0;
> +
> +	if (phdr->p_type == PT_RISCV_ATTRIBUTES && !is_interp)
> +		ret = rv_parse_elf_attributes(elf, phdr, state);
> +
> +	return ret;
> +}
> +
> +int arch_check_elf(void *_ehdr, bool has_interpreter, void *_interp_ehdr,
> +		   struct arch_elf_state *state)
> +{
> +	return 0;
> +}
> -- 
> 2.34.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists