Re: [PATCH v3 6/6] libfdt: Add overlay application function

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3132 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]