On Fri, Aug 23, 2019 at 06:23:40PM +0100, Dave Martin wrote:
> ELF program properties will needed for detecting whether to enable
> optional architecture or ABI features for a new ELF process.
>
> For now, there are no generic properties that we care about, so do
> nothing unless CONFIG_ARCH_USE_GNU_PROPERTY=y.
>
> Otherwise, the presence of properties using the PT_PROGRAM_PROPERTY
> phdrs entry (if any), and notify each property to the arch code.
>
> For now, the added code is not used.
>
> Signed-off-by: Dave Martin <Dave.Martin@xxxxxxx>
Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
Note below...
> [...]
> +static int parse_elf_property(const char *data, size_t *off, size_t datasz,
> + struct arch_elf_state *arch,
> + bool have_prev_type, u32 *prev_type)
> +{
> + size_t size, step;
> + const struct gnu_property *pr;
> + int ret;
> +
> + if (*off == datasz)
> + return -ENOENT;
> +
> + if (WARN_ON(*off > datasz || *off % elf_gnu_property_align))
> + return -EIO;
> +
> + size = datasz - *off;
> + if (size < sizeof(*pr))
> + return -EIO;
> +
> + pr = (const struct gnu_property *)(data + *off);
> + if (pr->pr_datasz > size - sizeof(*pr))
> + return -EIO;
> +
> + step = round_up(sizeof(*pr) + pr->pr_datasz, elf_gnu_property_align);
> + if (step > size)
> + return -EIO;
> +
> + /* Properties are supposed to be unique and sorted on pr_type: */
> + if (have_prev_type && pr->pr_type <= *prev_type)
> + return -EIO;
> + *prev_type = pr->pr_type;
> +
> + ret = arch_parse_elf_property(pr->pr_type,
> + data + *off + sizeof(*pr),
> + pr->pr_datasz, ELF_COMPAT, arch);
I find it slightly hard to read the "cursor" motion in this parse. It
feels strange, for example, to refer twice to "data + *off" with the
second including consumed *pr size. Everything is fine AFAICT in the math,
though, and I haven't been able to construct a convincingly "cleaner"
version. Maybe:
data += *off;
pr = (const struct gnu_property *)data;
data += sizeof(*pr);
...
ret = arch_parse_elf_property(pr->pr_type, data,
pr->pr_datasz, ELF_COMPAT, arch);
But that feels disjoint from the "step" calculation, so... I think what
you have is fine. :)
--
Kees Cook
