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

[Jann Horn]

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