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

[Steve Rae <srae@broadcom.com>]

On 14-08-07 06:23 AM, Marek Vasut wrote:
> 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.
>

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?

>>>> 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)....
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!
Thanks, Steve

>
>> 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 !
>