LinuxLists.cc - [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

On Fri, 2019-06-07 at 09:58 +0200, Peter Zijlstra wrote:
> On Thu, Jun 06, 2019 at 01:06:41PM -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.
> >
> > 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 <[email protected]>
> > Signed-off-by: Yu-cheng Yu <[email protected]>
>
> Did HJ write this patch as suggested by that SoB chain? If so, you lost
> a From: line on top, if not, the SoB thing is invalid.

I will fix that.

Yu-cheng

2019-06-07 19:00:47

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

On Thu, Jun 06, 2019 at 01:06:41PM -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.
>
> 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);
> ...
> }

Although this code works for the simple case, I have some concerns about
some aspects of the implementation here. There appear to be some bounds
checking / buffer overrun issues, and the code seems quite complex.

Maybe this patch tries too hard to be compatible with toolchains that do
silly things such as embedding huge notes in an executable, or mixing
NT_GNU_PROPERTY_TYPE_0 in a single PT_NOTE with a load of junk not
relevant to the loader. I wonder whether Linux can dictate what
interpretation(s) of the ELF specs it is prepared to support, rather than
trying to support absolutely anything.

I've commented on some potential issues below, but my review isn't
exhaustive -- I may also have simply not understood the code in some
cases, so I apologise in advance for that!

I've also marked a few coding style nits that make the code harder to
read than necessary (but this is partly a matter of taste).

Comments below.

Cheers
---Dave

>
> Signed-off-by: H.J. Lu <[email protected]>
> Signed-off-by: Yu-cheng Yu <[email protected]>
> ---
> fs/Kconfig.binfmt | 3 +
> fs/Makefile | 1 +
> fs/binfmt_elf.c | 13 ++
> fs/gnu_property.c | 351 +++++++++++++++++++++++++++++++++++++++
> include/linux/elf.h | 12 ++
> include/uapi/linux/elf.h | 14 ++
> 6 files changed, 394 insertions(+)
> create mode 100644 fs/gnu_property.c
>
> diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt
> index f87ddd1b6d72..397138ab305b 100644
> --- a/fs/Kconfig.binfmt
> +++ b/fs/Kconfig.binfmt
> @@ -36,6 +36,9 @@ config COMPAT_BINFMT_ELF
> config ARCH_BINFMT_ELF_STATE
> bool
>
> +config ARCH_USE_GNU_PROPERTY
> + bool
> +
> 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 c9aea23aba56..b69f18c14e09 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 8264b468f283..c3ea73787e93 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -1080,6 +1080,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 (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..9c4d1d5ebf00
> --- /dev/null
> +++ b/fs/gnu_property.c
> @@ -0,0 +1,351 @@
> +/* 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)

Nit: magic number in disguise. What does the size of ELF notes have
to do with the page size?

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

sizeof(*n) is a small integer under our control, and n->n_namesz is a
u32.

So, I'm not sure how we would overflow 64 bits here, although if we can
get arbitrarily close to ~(u64)0 then:

> +
> + size = round_up(size, *align);

this can overflow too.

> +
> + if (check_add_overflow(size, (u64)n->n_descsz, &size))
> + return NULL;
> +
> + size = round_up(size, *align);

Similarly here.

> +
> + if (buf + size < buf)

Isn't this undefined behaviour of it overflows? If so, the compiler can
probably delete the check entirely, making it useless. Does UBSAN warn
about it?

> + return NULL;
> + else
> + return (buf + size);

Nit: Unnecessary () (There are surplus () all over this patch; I won't
comment on them all.)

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

Is this worthwhile? In general we don't try very hard to check that the
ELF file is well-formed.

Ideally we could search by binary chop, but the property size is
variable, so the sortedness is useless to us (yay).

> +
> + return (pr->pr_type == pr_type);
> +}
> +
> +static inline void *next_property(void *buf, u32 *max_type, u32 pr_type)

Nit: does this need to be inline? The compiler's guess is usually good
enough...

> +{
> + struct gnu_property *pr = buf;
> +
> + if ((buf + sizeof(*pr) + pr->pr_datasz < buf) ||

Nit: random extra space, redundant (), etc.

> + (pr->pr_type > pr_type) ||
> + (pr->pr_type > *max_type))
> + return NULL;
> + else
> + return (buf + sizeof(*pr) + pr->pr_datasz);

We can exceed the underlying buffer bounds here, which is technically
undefined behaviour.

I suspect we may be relying on similar tricks all over the kernel, but
IT MAy be best avoided anyway.

If we always pass in the buffer base pointer and the size of the buffer, say

static int next_property(void *buf, size_t *offset,
size_t bufsz, ...)

then we may be able to use direct comparisons that can't overflow
rather than relying on potentially undefined behaviour. For example:

size_t o = *offset;

if (o > bufsz || sizeof (*pr) > bufsz - o)
return -1;

pr = buf + o;
if (pr->pr_type > pr_type || pr->pr_type > *max_type)
return -1;

if (pr->pr_datasz > bufsz - o - sizeof (*pr))
return -1;

*offset = o + sizeof (*pr) + pr->pr_datasz;
return 0;

(There may be neater ways to do this.)

> +}
> +
> +/*
> + * 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)

See comment about undefined behaviour above.

Also, I'm not sure this check adds anything. We know buf_size is
<= BUF_SIZE (though we could stick a WARN_ON() here and bail out if we
want to make absolutely sure).

If buf is always the base pointer returned by kmalloc(BUF_SIZE), then
I think buf_size can never go outside its bounds?

> + return 0;
> +
> + p = buf;
> +
> + while ((p + item_size < max) && (p + item_size > buf)) {

^ <= ? ^ undefined behaviour?

> + if (test_item(p, arg, type)) {
> + found = 1;
> + break;
> + }
> +
> + p = next_item(p, arg, type);
> + }
> +
> + *pos = (p + item_size <= buf) ? 0 : (u32)(p - buf);

Can this be written more simply, say:

if (p + item_size > buf)
*pos += p - buf;

Also, since next_property() adds pr_datasz onto buf, could we get
unlucky and wrap past (void *)~0UL? Then (u32)(p - buf) may be giant.
Not sure whether this breaks code elsewhere.

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

Do we really need to alloc/free this once per note?

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

Do we really need this complexity? How big are the notes realistically
going to be?

Since a file with bloated notes is going to be inefficient to exec
anyway if we have to scan all the way through them, would it be better
just to choke on it and force the toolchain to do something more
sensible?

This in one reason why it would be good for the kernel to require
PT_GNU_PROPERTY if possible, so we know the precise offset and size
without having to search...

> +
> + 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;
> +}
> +
> +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;
> +}
> +
> +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 {
> + 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;
> + }
> + }
> +
> +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..316177ce9e76 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

Should this be in a separate block. This required n_name = "GNU",
whereas the rest are "LINUX" notes AFAIK: it's really a separate
namespace.

I think the gap between 4 and 6 may be just coincidence: glibc's elf.h
already has NT_PLATFORM here (whatever that is).

> #define NT_AUXV 6
> /*
> * Note to userspace developers: size of NT_SIGINFO note may increase
> @@ -443,4 +444,17 @@ typedef struct elf64_note {
> Elf64_Word n_type; /* Content type */
> } Elf64_Nhdr;
>
> +/* NT_GNU_PROPERTY_TYPE_0 header */
> +struct gnu_property {
> + __u32 pr_type;
> + __u32 pr_datasz;
> +};
> +
> +/* .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)
> +

Redundant (). The rest of the file doesn't have them; can we conform to
the prevailing style there?

> #endif /* _UAPI_LINUX_ELF_H */
> --
> 2.17.1
>

2019-06-10 16:41:12

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

On Fri, 2019-06-07 at 19:01 +0100, Dave Martin wrote:
> On Thu, Jun 06, 2019 at 01:06:41PM -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.
> >
> > 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);
> > ...
> > }
>
> Although this code works for the simple case, I have some concerns about
> some aspects of the implementation here. There appear to be some bounds
> checking / buffer overrun issues, and the code seems quite complex.
>
> Maybe this patch tries too hard to be compatible with toolchains that do
> silly things such as embedding huge notes in an executable, or mixing
> NT_GNU_PROPERTY_TYPE_0 in a single PT_NOTE with a load of junk not
> relevant to the loader. I wonder whether Linux can dictate what
> interpretation(s) of the ELF specs it is prepared to support, rather than
> trying to support absolutely anything.

To me, looking at PT_GNU_PROPERTY and not trying to support anything is a
logical choice. And it breaks only a limited set of toolchains.

I will simplify the parser and leave this patch as-is for anyone who wants to
back-port. Are there any objections or concerns?

Yu-cheng

2019-06-10 16:59:00

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

On Mon, Jun 10, 2019 at 09:29:04AM -0700, Yu-cheng Yu wrote:
> On Fri, 2019-06-07 at 19:01 +0100, Dave Martin wrote:
> > On Thu, Jun 06, 2019 at 01:06:41PM -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.
> > >
> > > 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);
> > > ...
> > > }
> >
> > Although this code works for the simple case, I have some concerns about
> > some aspects of the implementation here. There appear to be some bounds
> > checking / buffer overrun issues, and the code seems quite complex.
> >
> > Maybe this patch tries too hard to be compatible with toolchains that do
> > silly things such as embedding huge notes in an executable, or mixing
> > NT_GNU_PROPERTY_TYPE_0 in a single PT_NOTE with a load of junk not
> > relevant to the loader. I wonder whether Linux can dictate what
> > interpretation(s) of the ELF specs it is prepared to support, rather than
> > trying to support absolutely anything.
>
> To me, looking at PT_GNU_PROPERTY and not trying to support anything is a
> logical choice. And it breaks only a limited set of toolchains.
>
> I will simplify the parser and leave this patch as-is for anyone who wants to
> back-port. Are there any objections or concerns?

No objection from me ;) But I'm biased.

Hopefully this change should allow substantial simplification. For one
thing, PT_GNU_PROPERTY tells its file offset and size directly in its
phdrs entry. That should save us a lot of effort on the kernel side.

Cheers
---Dave

2019-06-10 17:26:06

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

* Yu-cheng Yu:

> To me, looking at PT_GNU_PROPERTY and not trying to support anything is a
> logical choice. And it breaks only a limited set of toolchains.
>
> I will simplify the parser and leave this patch as-is for anyone who wants to
> back-port. Are there any objections or concerns?

Red Hat Enterprise Linux 8 does not use PT_GNU_PROPERTY and is probably
the largest collection of CET-enabled binaries that exists today.

My hope was that we would backport the upstream kernel patches for CET,
port the glibc dynamic loader to the new kernel interface, and be ready
to run with CET enabled in principle (except that porting userspace
libraries such as OpenSSL has not really started upstream, so many
processes where CET is particularly desirable will still run without
it).

I'm not sure if it is a good idea to port the legacy support if it's not
part of the mainline kernel because it comes awfully close to creating
our own private ABI.

Thanks,
Florian

2019-06-11 12:24:13

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

On Mon, Jun 10, 2019 at 07:24:43PM +0200, Florian Weimer wrote:
> * Yu-cheng Yu:
>
> > To me, looking at PT_GNU_PROPERTY and not trying to support anything is a
> > logical choice. And it breaks only a limited set of toolchains.
> >
> > I will simplify the parser and leave this patch as-is for anyone who wants to
> > back-port. Are there any objections or concerns?
>
> Red Hat Enterprise Linux 8 does not use PT_GNU_PROPERTY and is probably
> the largest collection of CET-enabled binaries that exists today.

For clarity, RHEL is actively parsing these properties today?

> My hope was that we would backport the upstream kernel patches for CET,
> port the glibc dynamic loader to the new kernel interface, and be ready
> to run with CET enabled in principle (except that porting userspace
> libraries such as OpenSSL has not really started upstream, so many
> processes where CET is particularly desirable will still run without
> it).
>
> I'm not sure if it is a good idea to port the legacy support if it's not
> part of the mainline kernel because it comes awfully close to creating
> our own private ABI.

I guess we can aim to factor things so that PT_NOTE scanning is
available as a fallback on arches for which the absence of
PT_GNU_PROPERTY is not authoritative.

Can we argue that the lack of PT_GNU_PROPERTY is an ABI bug, fix it
for new binaries and hence limit the efforts we go to to support
theoretical binaries that lack the phdrs entry?

If we can make practical simplifications to the parsing, such as
limiting the maximum PT_NOTE size that we will search for the program
properties to 1K (say), or requiring NT_NOTE_GNU_PROPERTY_TYPE_0 to sit
by itself in a single PT_NOTE then that could help minimse the exec
overheads and the number of places for bugs to hide in the kernel.

What we can do here depends on what the tools currently do and what
binaries are out there.

Cheers
---Dave

2019-06-12 00:00:52

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

On Tue, 2019-06-11 at 12:41 +0100, Dave Martin wrote:
> On Mon, Jun 10, 2019 at 07:24:43PM +0200, Florian Weimer wrote:
> > * Yu-cheng Yu:
> >
> > > To me, looking at PT_GNU_PROPERTY and not trying to support anything is a
> > > logical choice. And it breaks only a limited set of toolchains.
> > >
> > > I will simplify the parser and leave this patch as-is for anyone who wants
> > > to
> > > back-port. Are there any objections or concerns?
> >
> > Red Hat Enterprise Linux 8 does not use PT_GNU_PROPERTY and is probably
> > the largest collection of CET-enabled binaries that exists today.
>
> For clarity, RHEL is actively parsing these properties today?
>
> > My hope was that we would backport the upstream kernel patches for CET,
> > port the glibc dynamic loader to the new kernel interface, and be ready
> > to run with CET enabled in principle (except that porting userspace
> > libraries such as OpenSSL has not really started upstream, so many
> > processes where CET is particularly desirable will still run without
> > it).
> >
> > I'm not sure if it is a good idea to port the legacy support if it's not
> > part of the mainline kernel because it comes awfully close to creating
> > our own private ABI.
>
> I guess we can aim to factor things so that PT_NOTE scanning is
> available as a fallback on arches for which the absence of
> PT_GNU_PROPERTY is not authoritative.

We can probably check PT_GNU_PROPERTY first, and fallback (based on ld-linux
version?) to PT_NOTE scanning?

Yu-cheng

2019-06-12 09:37:12

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

On Tue, Jun 11, 2019 at 12:31:34PM -0700, Yu-cheng Yu wrote:
> On Tue, 2019-06-11 at 12:41 +0100, Dave Martin wrote:
> > On Mon, Jun 10, 2019 at 07:24:43PM +0200, Florian Weimer wrote:
> > > * Yu-cheng Yu:
> > >
> > > > To me, looking at PT_GNU_PROPERTY and not trying to support anything is a
> > > > logical choice. And it breaks only a limited set of toolchains.
> > > >
> > > > I will simplify the parser and leave this patch as-is for anyone who wants
> > > > to
> > > > back-port. Are there any objections or concerns?
> > >
> > > Red Hat Enterprise Linux 8 does not use PT_GNU_PROPERTY and is probably
> > > the largest collection of CET-enabled binaries that exists today.
> >
> > For clarity, RHEL is actively parsing these properties today?
> >
> > > My hope was that we would backport the upstream kernel patches for CET,
> > > port the glibc dynamic loader to the new kernel interface, and be ready
> > > to run with CET enabled in principle (except that porting userspace
> > > libraries such as OpenSSL has not really started upstream, so many
> > > processes where CET is particularly desirable will still run without
> > > it).
> > >
> > > I'm not sure if it is a good idea to port the legacy support if it's not
> > > part of the mainline kernel because it comes awfully close to creating
> > > our own private ABI.
> >
> > I guess we can aim to factor things so that PT_NOTE scanning is
> > available as a fallback on arches for which the absence of
> > PT_GNU_PROPERTY is not authoritative.
>
> We can probably check PT_GNU_PROPERTY first, and fallback (based on ld-linux
> version?) to PT_NOTE scanning?

For arm64, we can check for PT_GNU_PROPERTY and then give up
unconditionally.

For x86, we would fall back to PT_NOTE scanning, but this will add a bit
of cost to binaries that don't have NT_GNU_PROPERTY_TYPE_0. The ld.so
version doesn't tell you what ELF ABI a given executable conforms to.

Since this sounds like it's largely a distro-specific issue, maybe there
could be a Kconfig option to turn the fallback PT_NOTE scanning on?

Cheers
---Dave

2019-06-12 19:12:45

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

On Wed, 2019-06-12 at 10:32 +0100, Dave Martin wrote:
> On Tue, Jun 11, 2019 at 12:31:34PM -0700, Yu-cheng Yu wrote:
> > On Tue, 2019-06-11 at 12:41 +0100, Dave Martin wrote:
> > > On Mon, Jun 10, 2019 at 07:24:43PM +0200, Florian Weimer wrote:
> > > > * Yu-cheng Yu:
> > > >
> > > > > To me, looking at PT_GNU_PROPERTY and not trying to support anything
> > > > > is a
> > > > > logical choice. And it breaks only a limited set of toolchains.
> > > > >
> > > > > I will simplify the parser and leave this patch as-is for anyone who
> > > > > wants
> > > > > to
> > > > > back-port. Are there any objections or concerns?
> > > >
> > > > Red Hat Enterprise Linux 8 does not use PT_GNU_PROPERTY and is probably
> > > > the largest collection of CET-enabled binaries that exists today.
> > >
> > > For clarity, RHEL is actively parsing these properties today?
> > >
> > > > My hope was that we would backport the upstream kernel patches for CET,
> > > > port the glibc dynamic loader to the new kernel interface, and be ready
> > > > to run with CET enabled in principle (except that porting userspace
> > > > libraries such as OpenSSL has not really started upstream, so many
> > > > processes where CET is particularly desirable will still run without
> > > > it).
> > > >
> > > > I'm not sure if it is a good idea to port the legacy support if it's not
> > > > part of the mainline kernel because it comes awfully close to creating
> > > > our own private ABI.
> > >
> > > I guess we can aim to factor things so that PT_NOTE scanning is
> > > available as a fallback on arches for which the absence of
> > > PT_GNU_PROPERTY is not authoritative.
> >
> > We can probably check PT_GNU_PROPERTY first, and fallback (based on ld-linux
> > version?) to PT_NOTE scanning?
>
> For arm64, we can check for PT_GNU_PROPERTY and then give up
> unconditionally.
>
> For x86, we would fall back to PT_NOTE scanning, but this will add a bit
> of cost to binaries that don't have NT_GNU_PROPERTY_TYPE_0. The ld.so
> version doesn't tell you what ELF ABI a given executable conforms to.
>
> Since this sounds like it's largely a distro-specific issue, maybe there
> could be a Kconfig option to turn the fallback PT_NOTE scanning on?

Yes, I will make it a Kconfig option.

Yu-cheng

2019-06-13 15:17:25

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

On Wed, Jun 12, 2019 at 12:04:01PM -0700, Yu-cheng Yu wrote:
> On Wed, 2019-06-12 at 10:32 +0100, Dave Martin wrote:
> > On Tue, Jun 11, 2019 at 12:31:34PM -0700, Yu-cheng Yu wrote:
> > > On Tue, 2019-06-11 at 12:41 +0100, Dave Martin wrote:
> > > > On Mon, Jun 10, 2019 at 07:24:43PM +0200, Florian Weimer wrote:
> > > > > * Yu-cheng Yu:
> > > > >
> > > > > > To me, looking at PT_GNU_PROPERTY and not trying to support anything
> > > > > > is a
> > > > > > logical choice. And it breaks only a limited set of toolchains.
> > > > > >
> > > > > > I will simplify the parser and leave this patch as-is for anyone who
> > > > > > wants
> > > > > > to
> > > > > > back-port. Are there any objections or concerns?
> > > > >
> > > > > Red Hat Enterprise Linux 8 does not use PT_GNU_PROPERTY and is probably
> > > > > the largest collection of CET-enabled binaries that exists today.
> > > >
> > > > For clarity, RHEL is actively parsing these properties today?
> > > >
> > > > > My hope was that we would backport the upstream kernel patches for CET,
> > > > > port the glibc dynamic loader to the new kernel interface, and be ready
> > > > > to run with CET enabled in principle (except that porting userspace
> > > > > libraries such as OpenSSL has not really started upstream, so many
> > > > > processes where CET is particularly desirable will still run without
> > > > > it).
> > > > >
> > > > > I'm not sure if it is a good idea to port the legacy support if it's not
> > > > > part of the mainline kernel because it comes awfully close to creating
> > > > > our own private ABI.
> > > >
> > > > I guess we can aim to factor things so that PT_NOTE scanning is
> > > > available as a fallback on arches for which the absence of
> > > > PT_GNU_PROPERTY is not authoritative.
> > >
> > > We can probably check PT_GNU_PROPERTY first, and fallback (based on ld-linux
> > > version?) to PT_NOTE scanning?
> >
> > For arm64, we can check for PT_GNU_PROPERTY and then give up
> > unconditionally.
> >
> > For x86, we would fall back to PT_NOTE scanning, but this will add a bit
> > of cost to binaries that don't have NT_GNU_PROPERTY_TYPE_0. The ld.so
> > version doesn't tell you what ELF ABI a given executable conforms to.
> >
> > Since this sounds like it's largely a distro-specific issue, maybe there
> > could be a Kconfig option to turn the fallback PT_NOTE scanning on?
>
> Yes, I will make it a Kconfig option.

OK, that works for me. This would also help keep the PT_NOTE scanning
separate from the rest of the code.

For arm64 we could then unconditionally select/deselect that option,
where x86 could leave it configurable either way.

Cheers
---Dave

2019-06-17 11:09:31

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

* Dave Martin:

> On Tue, Jun 11, 2019 at 12:31:34PM -0700, Yu-cheng Yu wrote:
>> On Tue, 2019-06-11 at 12:41 +0100, Dave Martin wrote:
>> > On Mon, Jun 10, 2019 at 07:24:43PM +0200, Florian Weimer wrote:
>> > > * Yu-cheng Yu:
>> > >
>> > > > To me, looking at PT_GNU_PROPERTY and not trying to support anything is a
>> > > > logical choice. And it breaks only a limited set of toolchains.
>> > > >
>> > > > I will simplify the parser and leave this patch as-is for anyone who wants
>> > > > to
>> > > > back-port. Are there any objections or concerns?
>> > >
>> > > Red Hat Enterprise Linux 8 does not use PT_GNU_PROPERTY and is probably
>> > > the largest collection of CET-enabled binaries that exists today.
>> >
>> > For clarity, RHEL is actively parsing these properties today?
>> >
>> > > My hope was that we would backport the upstream kernel patches for CET,
>> > > port the glibc dynamic loader to the new kernel interface, and be ready
>> > > to run with CET enabled in principle (except that porting userspace
>> > > libraries such as OpenSSL has not really started upstream, so many
>> > > processes where CET is particularly desirable will still run without
>> > > it).
>> > >
>> > > I'm not sure if it is a good idea to port the legacy support if it's not
>> > > part of the mainline kernel because it comes awfully close to creating
>> > > our own private ABI.
>> >
>> > I guess we can aim to factor things so that PT_NOTE scanning is
>> > available as a fallback on arches for which the absence of
>> > PT_GNU_PROPERTY is not authoritative.
>>
>> We can probably check PT_GNU_PROPERTY first, and fallback (based on ld-linux
>> version?) to PT_NOTE scanning?
>
> For arm64, we can check for PT_GNU_PROPERTY and then give up
> unconditionally.
>
> For x86, we would fall back to PT_NOTE scanning, but this will add a bit
> of cost to binaries that don't have NT_GNU_PROPERTY_TYPE_0. The ld.so
> version doesn't tell you what ELF ABI a given executable conforms to.
>
> Since this sounds like it's largely a distro-specific issue, maybe there
> could be a Kconfig option to turn the fallback PT_NOTE scanning on?

I'm worried that this causes interop issues similarly to what we see
with VSYSCALL today. If we need both and a way to disable it, it should
be something like a personality flag which can be configured for each
process tree separately. Ideally, we'd settle on one correct approach
(i.e., either always process both, or only process PT_GNU_PROPERTY) and
enforce that.

Thanks,
Florian

2019-06-17 12:23:05

by Thomas Gleixner

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

On Mon, 17 Jun 2019, Florian Weimer wrote:
> * Dave Martin:
> > On Tue, Jun 11, 2019 at 12:31:34PM -0700, Yu-cheng Yu wrote:
> >> We can probably check PT_GNU_PROPERTY first, and fallback (based on ld-linux
> >> version?) to PT_NOTE scanning?
> >
> > For arm64, we can check for PT_GNU_PROPERTY and then give up
> > unconditionally.
> >
> > For x86, we would fall back to PT_NOTE scanning, but this will add a bit
> > of cost to binaries that don't have NT_GNU_PROPERTY_TYPE_0. The ld.so
> > version doesn't tell you what ELF ABI a given executable conforms to.
> >
> > Since this sounds like it's largely a distro-specific issue, maybe there
> > could be a Kconfig option to turn the fallback PT_NOTE scanning on?
>
> I'm worried that this causes interop issues similarly to what we see
> with VSYSCALL today. If we need both and a way to disable it, it should
> be something like a personality flag which can be configured for each
> process tree separately. Ideally, we'd settle on one correct approach
> (i.e., either always process both, or only process PT_GNU_PROPERTY) and
> enforce that.

Chose one and only the one which makes technically sense and is not some
horrible vehicle.

Everytime we did those 'oh we need to make x fly workarounds' we regretted
it sooner than later.

Thanks,

tglx

2019-06-18 09:13:20

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

On Mon, Jun 17, 2019 at 02:20:40PM +0200, Thomas Gleixner wrote:
> On Mon, 17 Jun 2019, Florian Weimer wrote:
> > * Dave Martin:
> > > On Tue, Jun 11, 2019 at 12:31:34PM -0700, Yu-cheng Yu wrote:
> > >> We can probably check PT_GNU_PROPERTY first, and fallback (based on ld-linux
> > >> version?) to PT_NOTE scanning?
> > >
> > > For arm64, we can check for PT_GNU_PROPERTY and then give up
> > > unconditionally.
> > >
> > > For x86, we would fall back to PT_NOTE scanning, but this will add a bit
> > > of cost to binaries that don't have NT_GNU_PROPERTY_TYPE_0. The ld.so
> > > version doesn't tell you what ELF ABI a given executable conforms to.
> > >
> > > Since this sounds like it's largely a distro-specific issue, maybe there
> > > could be a Kconfig option to turn the fallback PT_NOTE scanning on?
> >
> > I'm worried that this causes interop issues similarly to what we see
> > with VSYSCALL today. If we need both and a way to disable it, it should
> > be something like a personality flag which can be configured for each
> > process tree separately. Ideally, we'd settle on one correct approach
> > (i.e., either always process both, or only process PT_GNU_PROPERTY) and
> > enforce that.
>
> Chose one and only the one which makes technically sense and is not some
> horrible vehicle.
>
> Everytime we did those 'oh we need to make x fly workarounds' we regretted
> it sooner than later.

So I guess that points to keeping PT_NOTE scanning always available as a
fallback on x86. This sucks a bit, but if there are binaries already in
the wild that rely on this, I don't think we have much choice...

I'd still favour a Kconfig option to allow this support to be suppressed
by arches that don't have a similar legacy to be compatible with.

Cheers
---Dave

2019-06-18 12:42:29

by Peter Zijlstra

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

On Tue, Jun 18, 2019 at 10:12:50AM +0100, Dave Martin wrote:
> On Mon, Jun 17, 2019 at 02:20:40PM +0200, Thomas Gleixner wrote:
> > On Mon, 17 Jun 2019, Florian Weimer wrote:
> > > * Dave Martin:
> > > > On Tue, Jun 11, 2019 at 12:31:34PM -0700, Yu-cheng Yu wrote:
> > > >> We can probably check PT_GNU_PROPERTY first, and fallback (based on ld-linux
> > > >> version?) to PT_NOTE scanning?
> > > >
> > > > For arm64, we can check for PT_GNU_PROPERTY and then give up
> > > > unconditionally.
> > > >
> > > > For x86, we would fall back to PT_NOTE scanning, but this will add a bit
> > > > of cost to binaries that don't have NT_GNU_PROPERTY_TYPE_0. The ld.so
> > > > version doesn't tell you what ELF ABI a given executable conforms to.
> > > >
> > > > Since this sounds like it's largely a distro-specific issue, maybe there
> > > > could be a Kconfig option to turn the fallback PT_NOTE scanning on?
> > >
> > > I'm worried that this causes interop issues similarly to what we see
> > > with VSYSCALL today. If we need both and a way to disable it, it should
> > > be something like a personality flag which can be configured for each
> > > process tree separately. Ideally, we'd settle on one correct approach
> > > (i.e., either always process both, or only process PT_GNU_PROPERTY) and
> > > enforce that.
> >
> > Chose one and only the one which makes technically sense and is not some
> > horrible vehicle.
> >
> > Everytime we did those 'oh we need to make x fly workarounds' we regretted
> > it sooner than later.
>
> So I guess that points to keeping PT_NOTE scanning always available as a
> fallback on x86. This sucks a bit, but if there are binaries already in
> the wild that rely on this, I don't think we have much choice...

I'm not sure I read Thomas' comment like that. In my reading keeping the
PT_NOTE fallback is exactly one of those 'fly workarounds'. By not
supporting PT_NOTE only the 'fine' people already shit^Hpping this out
of tree are affected, and we don't have to care about them at all.

2019-06-18 12:47:44

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

* Peter Zijlstra:

> I'm not sure I read Thomas' comment like that. In my reading keeping the
> PT_NOTE fallback is exactly one of those 'fly workarounds'. By not
> supporting PT_NOTE only the 'fine' people already shit^Hpping this out
> of tree are affected, and we don't have to care about them at all.

Just to be clear here: There was an ABI document that required PT_NOTE
parsing. The Linux kernel does *not* define the x86-64 ABI, it only
implements it. The authoritative source should be the ABI document.

In this particularly case, so far anyone implementing this ABI extension
tried to provide value by changing it, sometimes successfully. Which
makes me wonder why we even bother to mainatain ABI documentation. The
kernel is just very late to the party.

Thanks,
Florian

2019-06-18 12:57:37

by Peter Zijlstra

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

On Tue, Jun 18, 2019 at 02:47:00PM +0200, Florian Weimer wrote:
> * Peter Zijlstra:
>
> > I'm not sure I read Thomas' comment like that. In my reading keeping the
> > PT_NOTE fallback is exactly one of those 'fly workarounds'. By not
> > supporting PT_NOTE only the 'fine' people already shit^Hpping this out
> > of tree are affected, and we don't have to care about them at all.
>
> Just to be clear here: There was an ABI document that required PT_NOTE
> parsing.

URGH.

> The Linux kernel does *not* define the x86-64 ABI, it only
> implements it. The authoritative source should be the ABI document.
>
> In this particularly case, so far anyone implementing this ABI extension
> tried to provide value by changing it, sometimes successfully. Which
> makes me wonder why we even bother to mainatain ABI documentation. The
> kernel is just very late to the party.

How can the kernel be late to the party if all of this is spinning
wheels without kernel support?

2019-06-18 13:34:42

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

On Tue, Jun 18, 2019 at 02:55:12PM +0200, Peter Zijlstra wrote:
> On Tue, Jun 18, 2019 at 02:47:00PM +0200, Florian Weimer wrote:
> > * Peter Zijlstra:
> >
> > > I'm not sure I read Thomas' comment like that. In my reading keeping the
> > > PT_NOTE fallback is exactly one of those 'fly workarounds'. By not
> > > supporting PT_NOTE only the 'fine' people already shit^Hpping this out
> > > of tree are affected, and we don't have to care about them at all.
> >
> > Just to be clear here: There was an ABI document that required PT_NOTE
> > parsing.
>
> URGH.
>
> > The Linux kernel does *not* define the x86-64 ABI, it only
> > implements it. The authoritative source should be the ABI document.
> >
> > In this particularly case, so far anyone implementing this ABI extension
> > tried to provide value by changing it, sometimes successfully. Which
> > makes me wonder why we even bother to mainatain ABI documentation. The
> > kernel is just very late to the party.
>
> How can the kernel be late to the party if all of this is spinning
> wheels without kernel support?

PT_GNU_PROPERTY is mentioned and allocated a p_type value in hjl's
spec [1], but otherwise seems underspecified.

In particular, it's not clear whether a PT_GNU_PROPERTY phdr _must_ be
emitted for NT_GNU_PROPERTY_TYPE_0. While it seems a no-brainer to emit
it, RHEL's linker already doesn't IIUC, and there are binaries in the
wild.

Maybe this phdr type is a late addition -- I haven't attempted to dig
through the history.

For arm64 we don't have this out-of-tree legacy to support, so we can
avoid exhausitvely searching for the note: no PT_GNU_PROPERTY ->
no note.

So, can we do the same for x86, forcing RHEL to carry some code out of
tree to support their legacy binaries? Or do we accept that there is
already a de facto ABI and try to be compatible with it?

From my side, I want to avoid duplication between x86 and arm64, and
keep unneeded complexity out of the ELF loader where possible.

Cheers
---Dave

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

2019-06-18 15:07:15

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

On Tue, 2019-06-18 at 14:32 +0100, Dave Martin wrote:
> On Tue, Jun 18, 2019 at 02:55:12PM +0200, Peter Zijlstra wrote:
> > On Tue, Jun 18, 2019 at 02:47:00PM +0200, Florian Weimer wrote:
> > > * Peter Zijlstra:
> > >
> > > > I'm not sure I read Thomas' comment like that. In my reading keeping the
> > > > PT_NOTE fallback is exactly one of those 'fly workarounds'. By not
> > > > supporting PT_NOTE only the 'fine' people already shit^Hpping this out
> > > > of tree are affected, and we don't have to care about them at all.
> > >
> > > Just to be clear here: There was an ABI document that required PT_NOTE
> > > parsing.
> >
> > URGH.
> >
> > > The Linux kernel does *not* define the x86-64 ABI, it only
> > > implements it. The authoritative source should be the ABI document.
> > >
> > > In this particularly case, so far anyone implementing this ABI extension
> > > tried to provide value by changing it, sometimes successfully. Which
> > > makes me wonder why we even bother to mainatain ABI documentation. The
> > > kernel is just very late to the party.
> >
> > How can the kernel be late to the party if all of this is spinning
> > wheels without kernel support?
>
> PT_GNU_PROPERTY is mentioned and allocated a p_type value in hjl's
> spec [1], but otherwise seems underspecified.
>
> In particular, it's not clear whether a PT_GNU_PROPERTY phdr _must_ be
> emitted for NT_GNU_PROPERTY_TYPE_0. While it seems a no-brainer to emit
> it, RHEL's linker already doesn't IIUC, and there are binaries in the
> wild.
>
> Maybe this phdr type is a late addition -- I haven't attempted to dig
> through the history.
>
>
> For arm64 we don't have this out-of-tree legacy to support, so we can
> avoid exhausitvely searching for the note: no PT_GNU_PROPERTY ->
> no note.
>
> So, can we do the same for x86, forcing RHEL to carry some code out of
> tree to support their legacy binaries? Or do we accept that there is
> already a de facto ABI and try to be compatible with it?
>
>
> From my side, I want to avoid duplication between x86 and arm64, and
> keep unneeded complexity out of the ELF loader where possible.

Hi Florian,

The kernel looks at only ld-linux. Other applications are loaded by ld-linux.
So the issues are limited to three versions of ld-linux's. Can we somehow
update those??

Thanks,
Yu-cheng

2019-06-18 15:51:55

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

* Yu-cheng Yu:

> The kernel looks at only ld-linux. Other applications are loaded by ld-linux.
> So the issues are limited to three versions of ld-linux's. Can we somehow
> update those??

I assumed that it would also parse the main executable and make
adjustments based on that.

ld.so can certainly provide whatever the kernel needs. We need to tweak
the existing loader anyway.

No valid statically-linked binaries exist today, so this is not a
consideration at this point.

Thanks,
Florian

2019-06-18 16:02:10

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

On Tue, 2019-06-18 at 17:49 +0200, Florian Weimer wrote:
> * Yu-cheng Yu:
>
> > The kernel looks at only ld-linux. Other applications are loaded by ld-
> > linux.
> > So the issues are limited to three versions of ld-linux's. Can we somehow
> > update those??
>
> I assumed that it would also parse the main executable and make
> adjustments based on that.

Yes, Linux also looks at the main executable's header, but not its
NT_GNU_PROPERTY_TYPE_0 if there is a loader.

>
> ld.so can certainly provide whatever the kernel needs. We need to tweak
> the existing loader anyway.
>
> No valid statically-linked binaries exist today, so this is not a
> consideration at this point.

So from kernel, we look at only PT_GNU_PROPERTY?

Yu-cheng

2019-06-18 16:07:12

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

* Yu-cheng Yu:

>> I assumed that it would also parse the main executable and make
>> adjustments based on that.
>
> Yes, Linux also looks at the main executable's header, but not its
> NT_GNU_PROPERTY_TYPE_0 if there is a loader.
>
>>
>> ld.so can certainly provide whatever the kernel needs. We need to tweak
>> the existing loader anyway.
>>
>> No valid statically-linked binaries exist today, so this is not a
>> consideration at this point.
>
> So from kernel, we look at only PT_GNU_PROPERTY?

If you don't parse notes/segments in the executable for CET, then yes.
We can put PT_GNU_PROPERTY into the loader.

Thanks,
Florian

2019-06-18 16:09:08

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

On Tue, 2019-06-18 at 18:05 +0200, Florian Weimer wrote:
> * Yu-cheng Yu:
>
> > > I assumed that it would also parse the main executable and make
> > > adjustments based on that.
> >
> > Yes, Linux also looks at the main executable's header, but not its
> > NT_GNU_PROPERTY_TYPE_0 if there is a loader.
> >
> > >
> > > ld.so can certainly provide whatever the kernel needs. We need to tweak
> > > the existing loader anyway.
> > >
> > > No valid statically-linked binaries exist today, so this is not a
> > > consideration at this point.
> >
> > So from kernel, we look at only PT_GNU_PROPERTY?
>
> If you don't parse notes/segments in the executable for CET, then yes.
> We can put PT_GNU_PROPERTY into the loader.

Thanks!

2019-06-18 16:20:39

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

On Tue, Jun 18, 2019 at 09:00:35AM -0700, Yu-cheng Yu wrote:
> On Tue, 2019-06-18 at 18:05 +0200, Florian Weimer wrote:
> > * Yu-cheng Yu:
> >
> > > > I assumed that it would also parse the main executable and make
> > > > adjustments based on that.
> > >
> > > Yes, Linux also looks at the main executable's header, but not its
> > > NT_GNU_PROPERTY_TYPE_0 if there is a loader.
> > >
> > > >
> > > > ld.so can certainly provide whatever the kernel needs. We need to tweak
> > > > the existing loader anyway.
> > > >
> > > > No valid statically-linked binaries exist today, so this is not a
> > > > consideration at this point.
> > >
> > > So from kernel, we look at only PT_GNU_PROPERTY?
> >
> > If you don't parse notes/segments in the executable for CET, then yes.
> > We can put PT_GNU_PROPERTY into the loader.
>
> Thanks!

Would this require the kernel and ld.so to be updated in a particular
order to avoid breakage? I don't know enough about RHEL to know how
controversial that might be.

Also:

What about static binaries distrubited as part of RHEL?

A user would also reasonably expect static binaries built using the
distro toolchain to work on top of the distro kernel... which might
be broken by this.

(When I say "broken" I mean that the binary would run, but CET
protections would be silently turned off.)

Cheers
---Dave

2019-06-18 16:26:41

[permalink] [raw]

Subject: Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file

* Dave Martin:

> On Tue, Jun 18, 2019 at 09:00:35AM -0700, Yu-cheng Yu wrote:
>> On Tue, 2019-06-18 at 18:05 +0200, Florian Weimer wrote:
>> > * Yu-cheng Yu:
>> >
>> > > > I assumed that it would also parse the main executable and make
>> > > > adjustments based on that.
>> > >
>> > > Yes, Linux also looks at the main executable's header, but not its
>> > > NT_GNU_PROPERTY_TYPE_0 if there is a loader.
>> > >
>> > > >
>> > > > ld.so can certainly provide whatever the kernel needs. We need to tweak
>> > > > the existing loader anyway.
>> > > >
>> > > > No valid statically-linked binaries exist today, so this is not a
>> > > > consideration at this point.
>> > >
>> > > So from kernel, we look at only PT_GNU_PROPERTY?
>> >
>> > If you don't parse notes/segments in the executable for CET, then yes.
>> > We can put PT_GNU_PROPERTY into the loader.
>>
>> Thanks!
>
> Would this require the kernel and ld.so to be updated in a particular
> order to avoid breakage? I don't know enough about RHEL to know how
> controversial that might be.

There is no official ld.so that will work with the current userspace
interface (in this patch submission). Upstream glibc needs to be
updated anyway, so yet another change isn't much of an issue. This is
not a problem; we knew that something like this might happen.

Sure, people need a new binutils with backports for PT_GNU_PROPERTY, but
given that only very few people will build CET binaries with older
binutils, I think that's not a real issue either.

Thanks,
Florian

2019-06-18 16:52:02