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

From: Yu-cheng Yu
Date: Mon Jul 01 2019 - 15:58:12 EST


On Mon, 2019-07-01 at 21:49 +0200, Jann Horn wrote:
> On Fri, Jun 28, 2019 at 7:30 PM Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx> wrote:
> [...]
> > In the discussion, we decided to look at only an ELF header's
> > PT_GNU_PROPERTY, which is a shortcut pointing to the file's
> > .note.gnu.property.
> >
> > The Linux gABI extension draft is here:
> >
> > https://github.com/hjl-tools/linux-abi/wiki/linux-abi-draft.pdf.
> >
> > A few existing CET-enabled binary files were built without
> > PT_GNU_PROPERTY; but those files' .note.gnu.property are checked by
> > ld-linux, not Linux. The compatibility impact from this change is
> > therefore managable.
> >
> > 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 specific
> > arch_parse_property() and arch_setup_property().
>
> [...]
> > +typedef bool (test_item_fn)(void *buf, u32 *arg, u32 type);
> > +typedef void *(next_item_fn)(void *buf, u32 *arg, u32 type);
> > +
> > +static bool test_property(void *buf, u32 *max_type, u32 pr_type)
> > +{
> > + struct gnu_property *pr = buf;
> > +
> > + /*
> > + * Property types must be in ascending order.
> > + * Keep track of the max when testing each.
> > + */
> > + if (pr->pr_type > *max_type)
> > + *max_type = pr->pr_type;
> > +
> > + return (pr->pr_type == pr_type);
> > +}
> > +
> > +static void *next_property(void *buf, u32 *max_type, u32 pr_type)
> > +{
> > + struct gnu_property *pr = buf;
> > +
> > + if ((buf + sizeof(*pr) + pr->pr_datasz < buf) ||
>
> This looks like UB to me, see below.
>
> > + (pr->pr_type > pr_type) ||
> > + (pr->pr_type > *max_type))
> > + return NULL;
> > + else
> > + return (buf + sizeof(*pr) + pr->pr_datasz);
> > +}
> > +
> > +/*
> > + * Scan 'buf' for a pattern; return true if found.
> > + * *pos is the distance from the beginning of buf to where
> > + * the searched item or the next item is located.
> > + */
> > +static int scan(u8 *buf, u32 buf_size, int item_size, test_item_fn
> > test_item,
> > + next_item_fn next_item, u32 *arg, u32 type, u32 *pos)
> > +{
> > + int found = 0;
> > + u8 *p, *max;
> > +
> > + max = buf + buf_size;
> > + if (max < buf)
> > + return 0;
>
> How can this ever legitimately happen? If it can't, perhaps you meant
> to put a WARN_ON_ONCE() or something like that here?
> Also, computing out-of-bounds pointers is UB (section 6.5.6 of C99:
> "If both the pointer operand and the result point to elements of the
> same array object, or one past the last element of the array object,
> the evaluation shall not produce an overflow; otherwise, the behavior
> is undefined."), and if the addition makes the pointer wrap, that's
> certainly out of bounds; so I don't think this condition can trigger
> without UB.
>
> > +
> > + p = buf;
> > +
> > + while ((p + item_size < max) && (p + item_size > buf)) {
>
> Again, as far as I know, this is technically UB. Please rewrite this.
> For example, you could do something like:
>
> while (max - p >= item_size) {
>
> and then make sure that next_item() never computes OOB pointers.
>
> > + if (test_item(p, arg, type)) {
> > + found = 1;
> > + break;
> > + }
> > +
> > + p = next_item(p, arg, type);
> > + }
> > +
> > + *pos = (p + item_size <= buf) ? 0 : (u32)(p - buf);
> > + return found;
> > +}
> > +
> > +/*
> > + * Search an NT_GNU_PROPERTY_TYPE_0 for the property of 'pr_type'.
> > + */
> > +static int find_property(u32 pr_type, u32 *property, struct file *file,
> > + loff_t file_offset, unsigned long desc_size)
> > +{
> > + u8 *buf;
> > + int buf_size;
> > +
> > + 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;
> > +
> > + buf_size = (desc_size > PAGE_SIZE) ? PAGE_SIZE : desc_size;
>
> open-coded min(desc_size, PAGE_SIZE)
>
> > + buf = kmalloc(buf_size, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + 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;
>
> This leaks the memory allocated for `buf`.
>
> > +
> > + 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;
> > + }
>
> [...]
> > + kfree(buf);
> > + return ret;
> > +}

I will fix these.

Thanks,
Yu-cheng