Re: [U-Boot] A minor question on a Driver Model function
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.
--
Regards,
Igor.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] A minor question on a Driver Model function
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
Re: [U-Boot] A minor question on a Driver Model function
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
Re: [U-Boot] A minor question on a Driver Model function
Hi Bill,
On 09/17/14 18:25, 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.
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, 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.
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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] A minor question on a Driver Model function
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.
Sorry, I didn't look and understand your query now.
Fwiw,
Bill Pringlemeir.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] A minor question on a Driver Model function
Hi Igor,
On Mon, 15 Sep 2014 11:04:20 +0300
Igor Grinberg grinb...@compulab.co.il wrote:
Hi,
On 09/14/14 21:28, Simon Glass wrote:
Hi Masahiro,
On 12 September 2014 05:25, Masahiro Yamada yamad...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] A minor question on a Driver Model function
On 09/17/14 11:18, Masahiro Yamada wrote:
Hi Igor,
On Mon, 15 Sep 2014 11:04:20 +0300
Igor Grinberg grinb...@compulab.co.il wrote:
Hi,
On 09/14/14 21:28, Simon Glass wrote:
Hi Masahiro,
On 12 September 2014 05:25, Masahiro Yamada yamad...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] A minor question on a Driver Model function
On 12 September 2014 05:25, Masahiro Yamada yamad...@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, 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.
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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] A minor question on a Driver Model function
Hi,
On 09/14/14 21:28, Simon Glass wrote:
Hi Masahiro,
On 12 September 2014 05:25, Masahiro Yamada yamad...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] A minor question on a Driver Model function
Hi Masahiro,
On 12 September 2014 05:25, Masahiro Yamada yamad...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] A minor question on a Driver Model function
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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
