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

[Masahiro Yamada]

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.


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