[U-Boot] A minor question on a Driver Model function

[Igor Grinberg]

On 09/19/14 09:54, Masahiro Yamada wrote:
> 
> On Fri, 19 Sep 2014 09:34:53 +0300
> Igor Grinberg <grinberg at compulab.co.il> wrote:
> 
>> On 09/18/14 18:46, Bill Pringlemeir wrote:
>>>
>>>>>> On 12 September 2014 05:25, Masahiro Yamada
>>>>>> <yamada.m at jp.panasonic.com> wrote:
>>>>>
>>>>>>>>>> I have a qustion about lists_driver_lookup_name() function.
>>>
>>>>>>>> On 09/14/14 21:28, Simon Glass wrote:
>>>>>
>>>>>>>> I would suggest still using strncmp as it is safer,
>>>>>>>> but count also the '\0', so something like:
>>>>>
>>>>> On 17 Sep 2014, grinberg at compulab.co.il wrote:
>>>>>
>>>>>>> Why safer?
>>>>>
>>>>>>> Could you give me more detailed explanation?
>>>>>
>>>>>> On 09/17/14 11:18, Masahiro Yamada wrote:
>>>>>
>>>>>> Well, I'm not an expert in s/w security, but I'll try to explain...
>>>>>
>>>>> [snip]
>>>>>
>>>>>> But, again, I'm not an expert in this area, so its only a
>>>>>> suggestion.
>>>>>
>>>
>>>> On 09/17/14 18:25, Bill Pringlemeir wrote:
>>>
>>>>> I thought it was fairly apparent that the current code supports
>>>>> passing a string that is *NOT* null terminated.  This can be
>>>>> convenient if you extract a sub-string from a command line and do not
>>>>> need to make a copy that is NULL terminate or perform 'strtok()' type
>>>>> magic.
>>>
>>> On 18 Sep 2014, grinberg at compulab.co.il wrote:
>>>
>>>> Here is the whole function:
>>>>
>>>> ------------------------------cut--------------------------
>>>> struct driver *lists_driver_lookup_name(const char *name)
>>>> {
>>>> struct driver *drv =
>>>> ll_entry_start(struct driver, driver);
>>>> const int n_ents = ll_entry_count(struct driver, driver);
>>>> struct driver *entry;
>>>> int len;
>>>>
>>>> if (!drv || !n_ents)
>>>> return NULL;
>>>>
>>>> len = strlen(name);
>>>>
>>>> for (entry = drv; entry != drv + n_ents; entry++) {
>>>> if (strncmp(name, entry->name, len))
>>>> continue;
>>>>
>>>> /* Full match */
>>>> if (len == strlen(entry->name))
>>>> return entry;
>>>>>
>>>>
>>>> /* Not found */
>>>> return NULL;
>>>>>
>>>> ------------------------------cut--------------------------
>>>>
>>>> and... no, the code does not support passing a string that is
>>>> not null terminated.
>>>
>>> Then using the strncmp() seems useless for security reasons?  The 'len'
>>> is not passed in by the caller and 'strlen()' will have the same
>>> problems that 'strcmp()' would for read buffer overflows?  I would guess
>>> the code was cribbed from where 'len' was passed?  In that case, it
>>> would support strings that are not null terminated.
>>
>> Yes, that is correct.
>>
>> Since we are dealing with device/driver names here.
>> I think the best would be to define a sane name length limit
>> (say 20 or more characters) and use it as the maximal length
>> if no '\0' found before the limit.
>>
>>
> 
> 
> I disagre.
> 
> The argument "name" of this function may be derived from a device tree,
> that is, it is possibly not NULL-terminated
> if U-Boot is accidentally given a corrupted device tree.

If this can happen, then the name length limit is even more sensible...

> 
> 
> On the other hand, "entry->name" originates in
> a U_BOOT_DRIVER() instance.
> 
> For example, something like this
> 
>     U_BOOT_DRIVER(root_driver) = {
>     	.name	= "root_driver",
>     	.id	= UCLASS_ROOT,
>     };
> 
> 
> The .name member of U_BOOT_DRIVER is guaranteed
> to be NULL-terminated.

I'd say the chances for _not_ having .name null terminated are
quite low, but I fail to find something that guarantees this.
May be I'm missing something, but you still can mess with
the .name field, right?

> 
> 
> I'd say,  strcmp(name, entry->name) is always safe.
> 
> 
> 
> 
> (In the current code,
> 
> 
> len = strlen(name);  is *NOT* safe,
> 
> 
> but,
> 
> 
> len = strlen(entry->name);  is safe

Yes, this was actually the first though that came into my mind, but
I wanted to take it a step further.

I agree that running strlen(entry->name) is better than
running it with name argument.

Yet, having a strong limit will prevent various corner cases.

Or, I'm just being too much paranoid/pedantic, and using
entry->name as an argument would be enough...

-- 
Regards,
Igor.