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

[Marek Vasut]

On Thursday, August 07, 2014 at 06:52:44 PM, Steve Rae wrote:

[...]

> >> 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.
> 
> I'm really confused: my code ensures that the buffer is not overflowed
> and that it is terminated properly. If snprintf() (without
> CONFIG_SYS_VSNPRINTF defined) doesn't provide "any overflow checking",
> then why would I use it?

That's why I suggested to enable CONFIG_SYS_VSNPRINTF unconditionally. Then your 
code would not need to duplicate all the overflow checks, would it ?

> >>>> 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 ?
> 
> I will apologize in advance, but I just don't know anything about SPL or
> TPL or any other boards (outside of my very limited armv7 and armv8
> scope)....

That's OK.

> I would be happy to review and test this suggested patch (on our
> boards), but would be uncomfortable with proposing this patch.
> Please go ahead and submit a patch, and I'll check it!

The patch would go something like:

#if !defined(CONFIG_SPL_BUILD) && !defined(CONFIG_TPL_BUILD)
#define CONFIG_SYS_VSNPRINTF
#endif

and this would go into include/config_cmd_default.h . Unless I'm wrong.