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

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

[Masahiro Yamada]

Hi Simon,


I have a qustion about lists_driver_lookup_name() function.



	for (entry = drv; entry != drv + n_ents; entry++) {
		if (strncmp(name, entry->name, len))
			continue;

		/* Full match */
		if (len == strlen(entry->name))
			return entry;
	}




Why is this not like follows?




	for (entry = drv; entry != drv + n_ents; entry++) {
		if (!strcmp(name, entry->name))
			return entry;
	}



It seems equivalent to the former and simpler.

Am I missing something?



Best Regards
Masahiro Yamada

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

[Simon Glass]

Hi Masahiro,

On 12 September 2014 05:25, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> Hi Simon,
>
>
> I have a qustion about lists_driver_lookup_name() function.
>
>
>
>         for (entry = drv; entry != drv + n_ents; entry++) {
>                 if (strncmp(name, entry->name, len))
>                         continue;
>
>                 /* Full match */
>                 if (len == strlen(entry->name))
>                         return entry;
>         }
>
>
>
>
> Why is this not like follows?
>
>
>
>
>         for (entry = drv; entry != drv + n_ents; entry++) {
>                 if (!strcmp(name, entry->name))
>                         return entry;
>         }

I think that code was there from the beginning. Marek might have
written it...the original series that I did as an RFC was here.

http://patchwork.ozlabs.org/patch/239255/

Might worth a patch.

>
>
>
> It seems equivalent to the former and simpler.
>
> Am I missing something?
>
>
>
> Best Regards
> Masahiro Yamada
>

Regards,
Simon

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

[Igor Grinberg]

Hi,

On 09/14/14 21:28, Simon Glass wrote:
> Hi Masahiro,
> 
> On 12 September 2014 05:25, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>> Hi Simon,
>>
>>
>> I have a qustion about lists_driver_lookup_name() function.
>>
>>
>>
>>         for (entry = drv; entry != drv + n_ents; entry++) {
>>                 if (strncmp(name, entry->name, len))
>>                         continue;
>>
>>                 /* Full match */
>>                 if (len == strlen(entry->name))
>>                         return entry;
>>         }
>>
>>
>>
>>
>> Why is this not like follows?
>>
>>
>>
>>
>>         for (entry = drv; entry != drv + n_ents; entry++) {
>>                 if (!strcmp(name, entry->name))
>>                         return entry;
>>         }

I would suggest still using strncmp as it is safer,
but count also the '\0', so something like:

for (entry = drv; entry != drv + n_ents; entry++) {
	if (!strncmp(name, entry->name, len + 1))
		return entry;
}

[...]


-- 
Regards,
Igor.

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

[Masahiro Yamada]

Hi Igor,



On Mon, 15 Sep 2014 11:04:20 +0300
Igor Grinberg <grinberg@compulab.co.il> wrote:

> Hi,
> 
> On 09/14/14 21:28, Simon Glass wrote:
> > Hi Masahiro,
> > 
> > On 12 September 2014 05:25, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> >> Hi Simon,
> >>
> >>
> >> I have a qustion about lists_driver_lookup_name() function.
> >>
> >>
> >>
> >>         for (entry = drv; entry != drv + n_ents; entry++) {
> >>                 if (strncmp(name, entry->name, len))
> >>                         continue;
> >>
> >>                 /* Full match */
> >>                 if (len == strlen(entry->name))
> >>                         return entry;
> >>         }
> >>
> >>
> >>
> >>
> >> Why is this not like follows?
> >>
> >>
> >>
> >>
> >>         for (entry = drv; entry != drv + n_ents; entry++) {
> >>                 if (!strcmp(name, entry->name))
> >>                         return entry;
> >>         }
> 
> I would suggest still using strncmp as it is safer,
> but count also the '\0', so something like:

Why safer?

Could you give me more detailed explanation?



Best Regards
Masahiro Yamada

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

[Igor Grinberg]

On 09/17/14 11:18, Masahiro Yamada wrote:
> Hi Igor,
> 
> 
> 
> On Mon, 15 Sep 2014 11:04:20 +0300
> Igor Grinberg <grinberg@compulab.co.il> wrote:
> 
>> Hi,
>>
>> On 09/14/14 21:28, Simon Glass wrote:
>>> Hi Masahiro,
>>>
>>> On 12 September 2014 05:25, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>>>> Hi Simon,
>>>>
>>>>
>>>> I have a qustion about lists_driver_lookup_name() function.
>>>>
>>>>
>>>>
>>>>         for (entry = drv; entry != drv + n_ents; entry++) {
>>>>                 if (strncmp(name, entry->name, len))
>>>>                         continue;
>>>>
>>>>                 /* Full match */
>>>>                 if (len == strlen(entry->name))
>>>>                         return entry;
>>>>         }
>>>>
>>>>
>>>>
>>>>
>>>> Why is this not like follows?
>>>>
>>>>
>>>>
>>>>
>>>>         for (entry = drv; entry != drv + n_ents; entry++) {
>>>>                 if (!strcmp(name, entry->name))
>>>>                         return entry;
>>>>         }
>>
>> I would suggest still using strncmp as it is safer,
>> but count also the '\0', so something like:
> 
> Why safer?
> 
> Could you give me more detailed explanation?

Well, I'm not an expert in s/w security, but I'll try to explain...

strcmp() walks the strings and never stops until it reaches '\0'
in either of strings.
In theory (or by mistake), you can supply strings that are not '\0'
terminated and strcmp() will continue running on addresses where
it is not supposed to.
This can lead to exceptions, crashes, etc..

Since this is a library code, I would expect it to be immune to
that kind of problem.

But, again, I'm not an expert in this area, so its only a suggestion.

-- 
Regards,
Igor.

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

[Bill Pringlemeir]

> On 12 September 2014 05:25, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:

>>>>> I have a qustion about lists_driver_lookup_name() function.

>>>>> for (entry = drv; entry != drv + n_ents; entry++) {
>>>>> if (strncmp(name, entry->name, len))
>>>>> continue;

>>>>> /* Full match */
>>>>> if (len == strlen(entry->name))
>>>>> return entry;
>>>>> }

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

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.

Fwiw,
Bill Pringlemeir.

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

[Igor Grinberg]

Hi Bill,

On 09/17/14 18:25, Bill Pringlemeir wrote:
> 
>> On 12 September 2014 05:25, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> 
>>>>>> I have a qustion about lists_driver_lookup_name() function.
> 
>>>>>> for (entry = drv; entry != drv + n_ents; entry++) {
>>>>>> if (strncmp(name, entry->name, len))
>>>>>> continue;
> 
>>>>>> /* Full match */
>>>>>> if (len == strlen(entry->name))
>>>>>> return entry;
>>>>>> }
> 
>>>> 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.
> 
> 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.

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.

-- 
Regards,
Igor.

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

[Bill Pringlemeir]

>>> On 12 September 2014 05:25, Masahiro Yamada
>>> <yamada.m@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.

Sorry, I didn't look and understand your query now.

Fwiw,
Bill Pringlemeir.

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

[Igor Grinberg]

On 09/18/14 18:46, Bill Pringlemeir wrote:
> 
>>>> On 12 September 2014 05:25, Masahiro Yamada
>>>> <yamada.m@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.


-- 
Regards,
Igor.

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

[Masahiro Yamada]

On Fri, 19 Sep 2014 09:34:53 +0300
Igor Grinberg <grinberg@compulab.co.il> wrote:

> On 09/18/14 18:46, Bill Pringlemeir wrote:
> > 
> >>>> On 12 September 2014 05:25, Masahiro Yamada
> >>>> <yamada.m@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.


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,  strcmp(name, entry->name) is always safe.




(In the current code,


len = strlen(name);  is *NOT* safe,


but,


len = strlen(entry->name);  is safe





Best Regards
Masahiro Yamada

[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@compulab.co.il> wrote:
> 
>> On 09/18/14 18:46, Bill Pringlemeir wrote:
>>>
>>>>>> On 12 September 2014 05:25, Masahiro Yamada
>>>>>> <yamada.m@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.