On Mon, Jul 25, 2016 at 08:20:41PM +0200, Maxime Ripard wrote:
> Hi David,
>
> On Mon, Jul 25, 2016 at 12:29:08AM +1000, David Gibson wrote:
> > > +static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
> > > + int property)
> > > +{
> > > + const char *value;
> > > + const char *label;
> > > + int len;
> > > +
> > > + value = fdt_getprop_by_offset(fdto, property,
> > > + &label, &len);
> > > + if (!value) {
> > > + if (len == -FDT_ERR_NOTFOUND)
> > > + return -FDT_ERR_INTERNAL;
> > > +
> > > + return len;
> > > + }
> > > +
> > > + do {
> > > + const char *prop_string = value;
> > > + const char *path, *name;
> > > + uint32_t prop_len = strnlen(value, len);
> >
> > prop_len is a bad name, since it could well be less than the length of
> > the whole property.
> >
> > > + uint32_t path_len, name_len;
> > > + char *sep, *endptr;
> > > + int index;
> > > + int ret;
> > > +
> > > + path = prop_string;
> > > + sep = memchr(prop_string, ':', prop_len);
> > > + if (!sep || (*sep != ':'))
> > > + return -FDT_ERR_BADSTRUCTURE;
> >
> > As mentioned on some of the other patches in the series, I think we
> > want a new error code for bad fixup / overlay information.
> >
> > > +
> > > + path_len = sep - path;
> > > + if (path_len == prop_len)
> > > + return -FDT_ERR_BADSTRUCTURE;
> >
> > I'm pretty sure this is impossible if sep != NULL.
> >
> > > + name = sep + 1;
> >
> > But I think the case you actually need to test for is path_len ==
> > (prop_len - 1), that will occur when : is the last character.
> >
> > > + sep = memchr(name, ':', prop_len);
> > > + if (!sep || *sep != ':')
> > > + return -FDT_ERR_BADSTRUCTURE;
> >
> > This still isn't quite safe. If the property has no \0, and : is the
> > last character in it, you'll access beyond the end of the property
> > here. It's probably easier if you just fail early if there is no \0 -
> > that's probably easier if you use memchr(\0) instead of strnlen().
> >
> > > +
> > > + name_len = sep - name;
> > > + if ((path_len + 1 + name_len) == prop_len)
> > > + return -FDT_ERR_BADSTRUCTURE;
> >
> > Again, off-by-one in this test, I think. Since there are so many
> > tricky edge cases here, it might be worth making testcases for them.
>
> What do you want here? That we move the parsing code out of that loop,
> make it public and put the prototype in libfdt_internal, or that we
> craft some DT that would outline all the possible issues with that
> function, and just test the return code of fdt_overlay_apply?
I was thinking crafted DTs. But I've realized that doing usefully is
pretty tricky, since overruns probably won't cause an immediate
problem most of the time. I guess they'd trip errors under valgrind
at least (as long you make sure there's at least one byte's alignment
gap until the next tag).
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
