[U-Boot] [PATCH 1/7] dfu:usb: Support for g_dnl composite download gadget.

[Mike Frysinger]

On Monday 23 July 2012 11:25:25 Lukasz Majewski wrote:
> Dear Mike Frysinger,
> > On Tuesday 03 July 2012 05:38:05 Lukasz Majewski wrote:
> > > +{
> > > +	int ret;
> > > +	static char str[16];
> > > +
> > > +	memset(str, '\0', sizeof(str));
> > > +
> > > +	strncpy(str, shortname, sizeof(shortname));
> > 
> > no need for the memset.
> 
> The gadget can be called from many separate commands (e.g. command
> "dfu" and command "ums") and those commands can be executed without
> power cycle. Thereof I need to be sure, that str is not polluted by
> previous name.

that makes no sense.  please read the documentation of the str*cpy functions 
-- they do no analysis of the target string and merely copy the source to the 
destination.  thus this code is basically:

	str[0] = '\0';
	str[1] = '\0';
	str[...] = '\0';
	str[0] = shortname[0];
	str[1] = shortname[1];
	str[...] = shortname[...];

it should be fairly obvious now why that memset is pointless.

> > this strncpy looks broken -- the 3rd arg is
> > for how many bytes are available in the *dest* buffer, not how long
> > the source is.
> 
> After looking deeply into the source I admit that providing the
> upper bound on the dest is more safe.

it isn't a matter of being safe, it's a matter of correctness

> > > +		strncat(str, s, sizeof(str));
> > 
> > this is also incorrect.  the length given to strncat is how many
> > bytes are left, not the total length.
> 
> I cannot agree. sizeof(str) return 16, which is the destination buffer
> size.

which is wrong.  please read the strncat specification.

> > since this string parsing logic is all just completely broken, i'd
> > suggest replacing it all with:
> > 
> > {
> > 	int ret;
> > 	/* We only allow "dfu" atm, so 3 should be enough */
> > 	static char name[sizeof(shortname) + 3];
> > 	
> > 	if (strcmp(s, "dfu")) {
> > 		printf("%s: unknown command: %s\n", __func__, s);
> > 		return -EINVAL;
> > 	}
> > 	
> > 	strcpy(name, shortname);
> > 	strcat(name, s);
> 
> This is a very neat design, but it assumes that there will be only one
> function ("dfu" in this case). For this particular function +3
> applies, but what if another function (like "usb_storage") will be
> defined?

why does that matter ?  the snippet i posted above is trivial to extend to 
support any number of functions.  increase the "3" to the max you care about, 
and then add more strcmp() to the if statement.

> I'm now working on another function - the USB Mass Storage (named
> "ums" ;-) ).
> 
> Another issue is omitting the strncmp/strncpy functions and depending on
> the: static char name[sizeof(shortname) + 3]; definition to prevent
> buffer overflow.

your existing code is already full of bugs that don't prevent overflow, and 
having the "3" right next to the "dfu" with a comment makes it pretty clear 
what is going on.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120724/a538543b/attachment.pgp>