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

Marek Vasut
2014-08-07T17:12:59Z

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.