riscv: elf: add .riscv.attributes parsing

Commit Message

Vineet Gupta Jan. 10, 2023, 8:18 p.m. UTC

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 <[e-mail]>  # code under CONFIG_COMPAT
Signed-off-by: Vineet Gupta <[e-mail]>
---
 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

Comments

Jessica Clarke Jan. 10, 2023, 8:48 p.m. UTC | #1

On 10 Jan 2023, at 20:18, Vineet Gupta <[e-mail]> 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 <[e-mail]>  # code under CONFIG_COMPAT
> Signed-off-by: Vineet Gupta <[e-mail]>

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
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Vineet Gupta Jan. 10, 2023, 9:50 p.m. UTC | #2

On 1/10/23 12:48, Jessica Clarke wrote:
> On 10 Jan 2023, at 20:18, Vineet Gupta <[e-mail]> 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 <[e-mail]>  # code under CONFIG_COMPAT
>> Signed-off-by: Vineet Gupta <[e-mail]>
> This code is full of buffer overruns and uninitialised reads in the
> presence of malicious files,

While the outer loop is bound, indeed the internal pointer increments 
could get oob.
I don't recall seeing existing explicit "safe" pointer, so thinking of 
cooking something up.
The conceptual idea is to replace

     p += 4

with

    PTR_INC(p, 4, p_max)

And similarly replace

     while (*p++ != '\0')

with

     while (*p != '\0')
           PTR_INC(p, 1, p_max)

Is that sufficient or you had something else in mind.


> and fails to check the version, vendor and sub-subsection tag.

That is now added.

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

Just to be on same page, a sub-section implies the following

       uint32:len, NTBS:vendor-name, uint8: Tag_file, uint32:data-len, 
<tag><value>....

If so, the code does support multiple of these

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

True, I've added get_unaligned_le32 for those now

Thx,
-Vineet

Conor Dooley Jan. 10, 2023, 10:04 p.m. UTC | #3

Hey Vineet,

While you're at it with Jess' concerns, couple nitpicks for you.
May as well say them now rather than while a v2 comes around.

On Tue, Jan 10, 2023 at 12:18:41PM -0800, Vineet Gupta wrote:

> Reported-by: kernel test robot <[e-mail]>  # code under CONFIG_COMPAT

You can drop this, even if it reported against a private branch AFAIU,
just like its complaints about patches. As Greg would say, LKP didn't
report a feature!

> diff --git a/arch/riscv/kernel/elf-attr.c b/arch/riscv/kernel/elf-attr.c
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023-24 Rivos Inc.

s/-24//


> +static u64
> +decode_uleb128(unsigned char **dpp)

Surely that fits inside 80?

> +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) {
> +

While I'm already passing through, checkpatch isn't the biggest fan of
your whitespace after open braces:

https://gist.github.com/conor-pwbot/a572e395763916c7716cab9c870df4f3

> +		unsigned char *vendor_start;
> +		u32 len;
> +
> +		/*
> +		 * Organized as "vendor" sub-section(s).
> +		 * Current only 1 specified "riscv"

Is it worth having a comment like this that may rapidly go out of date?

Cheers,
Conor.

Vineet Gupta Jan. 10, 2023, 10:16 p.m. UTC | #4

On 1/10/23 14:04, Conor Dooley wrote:
> Hey Vineet,
>
> While you're at it with Jess' concerns, couple nitpicks for you.
> May as well say them now rather than while a v2 comes around.

Thx for quick peek.

> On Tue, Jan 10, 2023 at 12:18:41PM -0800, Vineet Gupta wrote:
>
>> Reported-by: kernel test robot <[e-mail]>  # code under CONFIG_COMPAT
> You can drop this, even if it reported against a private branch AFAIU,
> just like its complaints about patches. As Greg would say, LKP didn't
> report a feature!

OK. Personally I tend to add Tested-by (vs. Reported-by for the same 
reasons) to still give them the credit for finding some issue.
I can certainly drop it.

>
>> diff --git a/arch/riscv/kernel/elf-attr.c b/arch/riscv/kernel/elf-attr.c
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2023-24 Rivos Inc.
> s/-24//

Fixed.

>
>> +static u64
>> +decode_uleb128(unsigned char **dpp)
> Surely that fits inside 80?

Fixed.

>> +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) {
>> +
> While I'm already passing through, checkpatch isn't the biggest fan of
> your whitespace after open braces:
>
> https://gist.github.com/conor-pwbot/a572e395763916c7716cab9c870df4f3

I swear this patch was checkpatch clean. Fixed now anyways.

>
>> +		unsigned char *vendor_start;
>> +		u32 len;
>> +
>> +		/*
>> +		 * Organized as "vendor" sub-section(s).
>> +		 * Current only 1 specified "riscv"
> Is it worth having a comment like this that may rapidly go out of date?

Rapid may be an overstatement given this is a psABI thing. Besides for a 
new vendor subsection, more code will need to be added to sanity check etc.
But I can drop this from comment and instead mention this in the changelog.

Thx,
-Vineet

Conor Dooley Jan. 10, 2023, 10:29 p.m. UTC | #5

On Tue, Jan 10, 2023 at 02:16:58PM -0800, Vineet Gupta wrote:
> On 1/10/23 14:04, Conor Dooley wrote:
> > On Tue, Jan 10, 2023 at 12:18:41PM -0800, Vineet Gupta wrote:
> > > Reported-by: kernel test robot <[e-mail]>  # code under CONFIG_COMPAT
> > You can drop this, even if it reported against a private branch AFAIU,
> > just like its complaints about patches. As Greg would say, LKP didn't
> > report a feature!
> 
> OK. Personally I tend to add Tested-by (vs. Reported-by for the same
> reasons) to still give them the credit for finding some issue.
> I can certainly drop it.

What I've seen Greg say is that you don't add "Reported-by" if someone
tells you your patch doesn't compile, so why would you for the build
robots.

Jessica Clarke Jan. 10, 2023, 11:21 p.m. UTC | #6

On 10 Jan 2023, at 21:50, Vineet Gupta <[e-mail]> wrote:
> On 1/10/23 12:48, Jessica Clarke wrote:
>> On 10 Jan 2023, at 20:18, Vineet Gupta <[e-mail]> 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 <[e-mail]>  # code under CONFIG_COMPAT
>>> Signed-off-by: Vineet Gupta <[e-mail]>
>> This code is full of buffer overruns and uninitialised reads in the
>> presence of malicious files,
> 
> While the outer loop is bound, indeed the internal pointer increments could get oob.
> I don't recall seeing existing explicit "safe" pointer, so thinking of cooking something up.
> The conceptual idea is to replace
> 
>     p += 4
> 
> with
> 
>    PTR_INC(p, 4, p_max)
> 
> And similarly replace
> 
>     while (*p++ != '\0')
> 
> with
> 
>     while (*p != '\0')
>           PTR_INC(p, 1, p_max)
> 
> Is that sufficient or you had something else in mind.

I’d be very careful about obfuscating control flow but how you
implement it is between you and other kernel developers, which I don’t
count myself as.

>> and fails to check the version, vendor and sub-subsection tag.
> 
> That is now added.
> 
>> You also should handle more than one sub-subsection even if tools don’t
>> emit it today.
> 
> Just to be on same page, a sub-section implies the following
> 
>       uint32:len, NTBS:vendor-name, uint8: Tag_file, uint32:data-len, <tag><value>....
> 
> If so, the code does support multiple of these

That’s a sub-section, but I said sub-subsection.

Jess

>> 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).
> 
> True, I've added get_unaligned_le32 for those now
> 
> Thx,
> -Vineet

Vineet Gupta Jan. 10, 2023, 11:45 p.m. UTC | #7

On 1/10/23 15:21, Jessica Clarke wrote:
>>> You also should handle more than one sub-subsection even if tools don’t
>>> emit it today.
>> Just to be on same page, a sub-section implies the following
>>
>>        uint32:len, NTBS:vendor-name, uint8: Tag_file, uint32:data-len, <tag><value>....
>>
>> If so, the code does support multiple of these
> That’s a sub-section, but I said sub-subsection.

The psABI documentation that we reviewed yesterday only mentions 
Tag_File = 1 and doesn't mention the term sub-subsection at all.
We need to be consistent between code and the new documentation or just 
add reference to ARM documentation which covers everything and write the 
general purpose parser.

-Vineet

Jessica Clarke Jan. 11, 2023, 12:29 a.m. UTC | #8

On 10 Jan 2023, at 23:45, Vineet Gupta <[e-mail]> wrote:
> On 1/10/23 15:21, Jessica Clarke wrote:
>>>> You also should handle more than one sub-subsection even if tools don’t
>>>> emit it today.
>>> Just to be on same page, a sub-section implies the following
>>> 
>>>       uint32:len, NTBS:vendor-name, uint8: Tag_file, uint32:data-len, <tag><value>....
>>> 
>>> If so, the code does support multiple of these
>> That’s a sub-section, but I said sub-subsection.
> 
> The psABI documentation that we reviewed yesterday only mentions Tag_File = 1 and doesn't mention the term sub-subsection at all.
> We need to be consistent between code and the new documentation or just add reference to ARM documentation which covers everything and write the general purpose parser.

Documentation you wrote and hasn’t been approved by anyone else. Where
I said that we need to document that multiple sub-subsections are
allowed.

Jess

Eric W. Biederman Jan. 11, 2023, 4:53 p.m. UTC | #9

Vineet Gupta <[e-mail]> writes:

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

Why does the kernel care?

Unless I am mistaken the kind of information you are describing is
typically dealt with by the dynamic linker, rather than the kernel.

As a general strategy I think it is smart to leave as much to
userspace and the dynamic linker and possible intead of asking
the kernel's elf loader to care.  That allows more rapid
prototyping and reduces the impact of parsing bugs.

Eric



> Reported-by: kernel test robot <[e-mail]>  # code under CONFIG_COMPAT
> Signed-off-by: Vineet Gupta <[e-mail]>
> ---
>  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;
> +}

Vineet Gupta Jan. 12, 2023, 8:22 p.m. UTC | #10

On 1/11/23 08:53, Eric W. Biederman wrote:
> Vineet Gupta <[e-mail]> writes:
>
>> 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.
> Why does the kernel care?
>
> Unless I am mistaken the kind of information you are describing is
> typically dealt with by the dynamic linker, rather than the kernel.
>
> As a general strategy I think it is smart to leave as much to
> userspace and the dynamic linker and possible intead of asking
> the kernel's elf loader to care.

That is true in general. But in some cases we might have to do stuff 
before kick starting usermode such as setting up a shadow stack with 
special permissions etc.

> That allows more rapid
> prototyping and reduces the impact of parsing bugs.

Patch

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;
+}