On Fri, 19 Sep 2014 09:34:53 +0300 Igor Grinberg <grinb...@compulab.co.il> wrote:
> On 09/18/14 18:46, Bill Pringlemeir wrote: > > > >>>> On 12 September 2014 05:25, Masahiro Yamada > >>>> <yamad...@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, grinb...@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, grinb...@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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot