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

[Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

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

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?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]