On 09/19/14 09:54, Masahiro Yamada wrote: > > 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.
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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot