[U-Boot] [PATCH v3 2/4] usb/gadget: fastboot: add eMMC support for flash command

[Marek Vasut]

On Thursday, August 07, 2014 at 02:28:13 AM, Steve Rae wrote:
> On 14-08-06 05:13 PM, Marek Vasut wrote:
> > On Thursday, August 07, 2014 at 01:48:06 AM, Steve Rae wrote:
> >> On 14-07-30 06:37 PM, Marek Vasut wrote:
> >>> On Thursday, June 26, 2014 at 10:13:22 PM, Steve Rae wrote:
> >>> [...]
> >>> 
> >>>> +
> >>>> +#include <common.h>
> >>>> +#include <fb_mmc.h>
> >>>> +#include <part.h>
> >>>> +#include <sparse_format.h>
> >>>> +
> >>>> +/* The 64 defined bytes plus \0 */
> >>>> +#define RESPONSE_LEN	(64 + 1)
> >>>> +
> >>>> +static char *response_str;
> >>> 
> >>> I'd suggest to pass this "response_str" around instead of making it
> >>> global.
> >> 
> >> That would involve adding it to fastboot_resp(), which is called 11
> >> times in this code, from 3 different functions (so would need to add
> >> this to two of the functions...). And as these evolve, there will likely
> >> be more nested functions, which would all require "passing it
> >> around".... I think that this "static global pointer" is a cleaner
> >> implementation.
> > 
> > Eventually, the amount of these static variables in the code will grow
> > and it will become increasingly difficult to weed them out. I believe it
> > would be even better to pass around some kind of a structure with
> > "private data" of the fastboot, which would cater for all possible
> > variables which might come in the future. What do you think ?
> 
> Yes -- if there is private data that the fastboot implementation
> requires, then a data structure is the way to go. However, I still think
> that this "fastboot response string" would even be an exception to that
> private data....

OK, let's leave it this way for now.

> >>>> +static void fastboot_resp(const char *s)
> >>>> +{
> >>>> +	strncpy(response_str, s, RESPONSE_LEN);
> >>>> +	response_str[RESPONSE_LEN - 1] = '\0';
> >>> 
> >>> This could be shrunk to a single snprintf(response_str,
> >>> RESPONSE_LENGTH, s); I think, but I'm not sure if the overhead won't
> >>> grow.
> >> 
> >> snprintf() is used very sparingling in U-Boot
> > 
> > This is not a reason to avoid it.
> 
> true....
> 
> >> , and with the cautionary statements in README (line 852)
> > 
> > Which statements? Can you please point them out? I fail to see them,
> > sorry.
> 
> I was referring to what you mention below...
>   852 - Safe printf() functions
>   853      Define CONFIG_SYS_VSNPRINTF to compile in safe versions of
>   854      the printf() functions. These are defined in
>   855      include/vsprintf.h and include snprintf(), vsnprintf() and
>   856      so on. Code size increase is approximately 300-500 bytes.
>   857      If this option is not given then these functions will
>   858      silently discard their buffer size argument - this means
>   859      you are not getting any overflow checking in this case.

I really don't see the "cautionary statements" here , no . I see that it 
discards the size checking if this CONFIG_SYS_VSNPRINTF is not enabled, but that 
does not obstruct the operation of those functions.

> >> and the fact that CONFIG_SYS_VSNPRINTF is not defined for armv7 builds,
> >> I am
> > 
> > not going to use it....
> > 
> > Is it a problem to define it? Also, even without CONFIG_SYS_VSNPRINTF ,
> > the
> > 
> > functions are still available, see the README:
> >   857                 If this option is not given then these functions
> >   will 858                 silently discard their buffer size argument -
> >   this means 859                 you are not getting any overflow
> >   checking in this case.
> > 
> > I have yet to see some hard-evidence against using safe printing
> > functions here.
> 
> I don't want to be the first to defined it for all of armv7....

Honestly, we should just enable this CONFIG_SYS_VSNPRINTF by default for the 
good of humanity and all the things, since this unbounded string handling is 
just evil (see how OpenSSL ended up, partly because of that ... and I am just 
starting to see the pattern in all the security code). I don't want to go down 
that road with U-Boot.

So, would you please cook a separate patch to enable this by default, so it 
would spur the right kind of discussion on this matter ?

> And I really don't want to define it only only my boards running so that
> they can run 'fastboot'
> What do you suggest?

See above, thanks !