[U-Boot] [PATCH v1] env: fix crash using default -f -a

[U-Boot] [PATCH v1] env: fix crash using default -f -a

[Stefano Babic]

newval pointer is not checked before its usage,
inside env_check_apply().

Signed-off-by: Stefano Babic <sbabic@denx.de>
CC: gerlando.falauto at keymile.com
---
 common/cmd_nvedit.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index 3474bc6..8144c85 100644
--- a/common/cmd_nvedit.c
+++ b/common/cmd_nvedit.c
@@ -272,7 +272,7 @@ int env_check_apply(const char *name, const char *oldval,
 		/*
 		 * Switch to new baudrate if new baudrate is supported
 		 */
-		if (strcmp(name, "baudrate") == 0) {
+		if ((strcmp(name, "baudrate") == 0) && (newval != NULL)) {
 			int baudrate = simple_strtoul(newval, NULL, 10);
 			int i;
 			for (i = 0; i < N_BAUDRATES; ++i) {
@@ -308,12 +308,12 @@ int env_check_apply(const char *name, const char *oldval,
 	 * Some variables should be updated when the corresponding
 	 * entry in the environment is changed
 	 */
-	if (strcmp(name, "loadaddr") == 0) {
+	if ((strcmp(name, "loadaddr") == 0) && (newval != NULL)) {
 		load_addr = simple_strtoul(newval, NULL, 16);
 		return 0;
 	}
 #if defined(CONFIG_CMD_NET)
-	else if (strcmp(name, "bootfile") == 0) {
+	else if ((strcmp(name, "bootfile") == 0) && (newval != NULL)) {
 		copy_filename(BootFile, newval, sizeof(BootFile));
 		return 0;
 	}
-- 
1.7.9.5

[U-Boot] [PATCH v1] env: fix crash using default -f -a

[Tom Rini]

On Thu, Oct 04, 2012 at 07:30:43PM +0200, Stefano Babic wrote:

> newval pointer is not checked before its usage,
> inside env_check_apply().
> 
> Signed-off-by: Stefano Babic <sbabic@denx.de>
> CC: gerlando.falauto at keymile.com

Some egg on my face for not catching this to start with, confirmed
problem and fix here.

Tested-by: Tom Rini <trini@ti.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121004/c35cedbf/attachment.pgp>

[U-Boot] [PATCH v1] env: fix crash using default -f -a

[Gerlando Falauto]

Hi Stefano,

thanks for reporting and providing a fix for this.
I'm very sorry for introducing this problem and for the late response.

Please see my comments below.

[As a side node, I couldn't really reproduce the issue neither on 
PowerPC nor on ARM (though simple_strtoul should legitimately crash) as 
apparently accessing NULL locations doesn't really bother our 
architectures...]

On 10/04/2012 07:30 PM, Stefano Babic wrote:
> newval pointer is not checked before its usage,
> inside env_check_apply().
>
> Signed-off-by: Stefano Babic<sbabic@denx.de>
> CC: gerlando.falauto at keymile.com
> ---
>   common/cmd_nvedit.c |    6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
> index 3474bc6..8144c85 100644
> --- a/common/cmd_nvedit.c
> +++ b/common/cmd_nvedit.c
> @@ -272,7 +272,7 @@ int env_check_apply(const char *name, const char *oldval,
>   		/*
>   		 * Switch to new baudrate if new baudrate is supported
>   		 */
> -		if (strcmp(name, "baudrate") == 0) {
> +		if ((strcmp(name, "baudrate") == 0)&&  (newval != NULL)) {

You're right in that the problem here is due to the function explicitly 
being called with a NULL value.
My idea was to have "env default" perform some sort of cleanup before 
assigning the actual values from the default environment (if any).
So by adding these extra checks you're subtly canceling those efforts.
So I would rather provide a default value to prevent the crashes, 
instead of skipping settings altogether. Posting a patch in a few minutes.

Whether it's legitimate to perform these "cleanups", well that's of 
course open for discussion.
But I believe it doesn't make much sense to call a function and pass an 
argument which is sort of guaranteed to have no effect whatsoever.

>   			int baudrate = simple_strtoul(newval, NULL, 10);
>   			int i;
>   			for (i = 0; i<  N_BAUDRATES; ++i) {
> @@ -308,12 +308,12 @@ int env_check_apply(const char *name, const char *oldval,
>   	 * Some variables should be updated when the corresponding
>   	 * entry in the environment is changed
>   	 */
> -	if (strcmp(name, "loadaddr") == 0) {
> +	if ((strcmp(name, "loadaddr") == 0)&&  (newval != NULL)) {
>   		load_addr = simple_strtoul(newval, NULL, 16);
>   		return 0;
>   	}
>   #if defined(CONFIG_CMD_NET)
> -	else if (strcmp(name, "bootfile") == 0) {
> +	else if ((strcmp(name, "bootfile") == 0)&&  (newval != NULL)) {
>   		copy_filename(BootFile, newval, sizeof(BootFile));
>   		return 0;
>   	}

Thank you and sorry again,
Gerlando

[U-Boot] [PATCH v1] env: fix crash using default -f -a

[Stefano Babic]

On 05/10/2012 12:29, Gerlando Falauto wrote:
> Hi Stefano,
> 

Hi Gerlando,

> thanks for reporting and providing a fix for this.
> I'm very sorry for introducing this problem and for the late response.
> 
> Please see my comments below.
> 
> [As a side node, I couldn't really reproduce the issue neither on
> PowerPC nor on ARM (though simple_strtoul should legitimately crash) as
> apparently accessing NULL locations doesn't really bother our
> architectures...]

Right. For PowerPC, Address 0 is a valid address in memory and the
system does not crash. However, I tested on a TI's ARM and address zero
is not valid.

> 
> On 10/04/2012 07:30 PM, Stefano Babic wrote:
>> newval pointer is not checked before its usage,
>> inside env_check_apply().
>>
>> Signed-off-by: Stefano Babic<sbabic@denx.de>
>> CC: gerlando.falauto at keymile.com
>> ---
>>   common/cmd_nvedit.c |    6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
>> index 3474bc6..8144c85 100644
>> --- a/common/cmd_nvedit.c
>> +++ b/common/cmd_nvedit.c
>> @@ -272,7 +272,7 @@ int env_check_apply(const char *name, const char
>> *oldval,
>>           /*
>>            * Switch to new baudrate if new baudrate is supported
>>            */
>> -        if (strcmp(name, "baudrate") == 0) {
>> +        if ((strcmp(name, "baudrate") == 0)&&  (newval != NULL)) {
> 
> You're right in that the problem here is due to the function explicitly
> being called with a NULL value.
> My idea was to have "env default" perform some sort of cleanup before
> assigning the actual values from the default environment (if any).

I have seen, I have not fully understood. My idea is that, if someone
want to restore the environment to the default values, no actions should
be taken, else the "default" values cannot be really restored.

> So by adding these extra checks you're subtly canceling those efforts.

That is correct, but in any case a check for a NULL pointer should be
done before accessing to the pointer, independently what we want to do
with it.

> So I would rather provide a default value to prevent the crashes,
> instead of skipping settings altogether. Posting a patch in a few minutes.

Ok, I will review it. In any case, we need a fix for the coming release.

> 
> Whether it's legitimate to perform these "cleanups", well that's of
> course open for discussion.

Indeed.

> But I believe it doesn't make much sense to call a function and pass an
> argument which is sort of guaranteed to have no effect whatsoever.

Right. It is the same as calling the function with do_apply=0.

> 
>>               int baudrate = simple_strtoul(newval, NULL, 10);
>>               int i;
>>               for (i = 0; i<  N_BAUDRATES; ++i) {
>> @@ -308,12 +308,12 @@ int env_check_apply(const char *name, const char
>> *oldval,
>>        * Some variables should be updated when the corresponding
>>        * entry in the environment is changed
>>        */
>> -    if (strcmp(name, "loadaddr") == 0) {
>> +    if ((strcmp(name, "loadaddr") == 0)&&  (newval != NULL)) {
>>           load_addr = simple_strtoul(newval, NULL, 16);
>>           return 0;
>>       }
>>   #if defined(CONFIG_CMD_NET)
>> -    else if (strcmp(name, "bootfile") == 0) {
>> +    else if ((strcmp(name, "bootfile") == 0)&&  (newval != NULL)) {
>>           copy_filename(BootFile, newval, sizeof(BootFile));
>>           return 0;
>>       }
>

Best regards,
Stefano


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH] env: fix crash using default -f -a

[Gerlando Falauto]

env default -a -f calls env_check_apply on all existing environment
variables with a NULL value for "newval" as a way of cleaning up.
This causes string manipulation functions to crash on most architectures.
So replace a NULL argument with an empty string.

Reported-By: Stefano Babic <sbabic@denx.de>
Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
---
 common/cmd_nvedit.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index 3474bc6..c38b475 100644
--- a/common/cmd_nvedit.c
+++ b/common/cmd_nvedit.c
@@ -213,6 +213,9 @@ int env_check_apply(const char *name, const char *oldval,
 {
 	int   console = -1;
 
+	/* Default value for NULL to protect string-manipulating functions */
+	newval = newval ? : "";
+
 	/* Check for console redirection */
 	if (strcmp(name, "stdin") == 0)
 		console = stdin;
-- 
1.7.10.1

[U-Boot] [PATCH] env: fix crash using default -f -a

[Stefano Babic]

On 05/10/2012 12:46, Gerlando Falauto wrote:
> env default -a -f calls env_check_apply on all existing environment
> variables with a NULL value for "newval" as a way of cleaning up.
> This causes string manipulation functions to crash on most architectures.
> So replace a NULL argument with an empty string.
> 
> Reported-By: Stefano Babic <sbabic@denx.de>
> Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
> ---
>  common/cmd_nvedit.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
> index 3474bc6..c38b475 100644
> --- a/common/cmd_nvedit.c
> +++ b/common/cmd_nvedit.c
> @@ -213,6 +213,9 @@ int env_check_apply(const char *name, const char *oldval,
>  {
>  	int   console = -1;
>  
> +	/* Default value for NULL to protect string-manipulating functions */
> +	newval = newval ? : "";
> +
>  	/* Check for console redirection */
>  	if (strcmp(name, "stdin") == 0)
>  		console = stdin;
> 

It fixes the issue.

Tested-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH] env: fix crash using default -f -a

[Tom Rini]

On Fri, Oct 05, 2012 at 12:46:10PM +0200, Gerlando Falauto wrote:

> env default -a -f calls env_check_apply on all existing environment
> variables with a NULL value for "newval" as a way of cleaning up.
> This causes string manipulation functions to crash on most architectures.
> So replace a NULL argument with an empty string.
> 
> Reported-By: Stefano Babic <sbabic@denx.de>
> Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121005/9919bbd3/attachment.pgp>

[U-Boot] [PATCH v1] env: fix crash using default -f -a

[Gerlando Falauto]

Hi Stefano,

On 10/05/2012 12:41 PM, Stefano Babic wrote:
> On 05/10/2012 12:29, Gerlando Falauto wrote:
>> Hi Stefano,
>>
>
> Hi Gerlando,
>
>> thanks for reporting and providing a fix for this.
>> I'm very sorry for introducing this problem and for the late response.
>>
>> Please see my comments below.
>>
>> [As a side node, I couldn't really reproduce the issue neither on
>> PowerPC nor on ARM (though simple_strtoul should legitimately crash) as
>> apparently accessing NULL locations doesn't really bother our
>> architectures...]
>
> Right. For PowerPC, Address 0 is a valid address in memory and the
> system does not crash.

I didn't know that, thanks.

> However, I tested on a TI's ARM and address zero
> is not valid.
>
>>
>> On 10/04/2012 07:30 PM, Stefano Babic wrote:
>>> newval pointer is not checked before its usage,
>>> inside env_check_apply().
>>>
>>> Signed-off-by: Stefano Babic<sbabic@denx.de>
>>> CC: gerlando.falauto at keymile.com
>>> ---
>>>    common/cmd_nvedit.c |    6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
>>> index 3474bc6..8144c85 100644
>>> --- a/common/cmd_nvedit.c
>>> +++ b/common/cmd_nvedit.c
>>> @@ -272,7 +272,7 @@ int env_check_apply(const char *name, const char
>>> *oldval,
>>>            /*
>>>             * Switch to new baudrate if new baudrate is supported
>>>             */
>>> -        if (strcmp(name, "baudrate") == 0) {
>>> +        if ((strcmp(name, "baudrate") == 0)&&   (newval != NULL)) {
>>
>> You're right in that the problem here is due to the function explicitly
>> being called with a NULL value.
>> My idea was to have "env default" perform some sort of cleanup before
>> assigning the actual values from the default environment (if any).
>
> I have seen, I have not fully understood. My idea is that, if someone
> want to restore the environment to the default values, no actions should
> be taken, else the "default" values cannot be really restored.

My idea here was to "apply" the changes as soon as the environment gets 
changed, like it would happen with a manual "setenv"/"env set".
So for instance, if you switch from a custom baudrate back to the 
default baudrate, you get a message prompting you to switch to the new 
baudrate (which actually *DOES* change on the console).

Whether that should also be performed as a "cleanup" action upon 
restoring defaults, well I am not sure...

>> So by adding these extra checks you're subtly canceling those efforts.
>
> That is correct, but in any case a check for a NULL pointer should be
> done before accessing to the pointer, independently what we want to do
> with it.

Absolutely. See my proposed patch.

>> So I would rather provide a default value to prevent the crashes,
>> instead of skipping settings altogether. Posting a patch in a few minutes.
>
> Ok, I will review it. In any case, we need a fix for the coming release.

Absolutely. Once again, I cannot apologize and thank you enough for 
reporting and digging into this.
I assumed providing an own patch would make more sense than asking you 
to review yours, not sure whether it is popular practice to do so.

>> Whether it's legitimate to perform these "cleanups", well that's of
>> course open for discussion.
>
> Indeed.
>
>> But I believe it doesn't make much sense to call a function and pass an
>> argument which is sort of guaranteed to have no effect whatsoever.
>
> Right. It is the same as calling the function with do_apply=0.

Right, but since the return value is also ignored, what's the point in 
calling it in the first place?

>>
>>>                int baudrate = simple_strtoul(newval, NULL, 10);
>>>                int i;
>>>                for (i = 0; i<   N_BAUDRATES; ++i) {
>>> @@ -308,12 +308,12 @@ int env_check_apply(const char *name, const char
>>> *oldval,
>>>         * Some variables should be updated when the corresponding
>>>         * entry in the environment is changed
>>>         */
>>> -    if (strcmp(name, "loadaddr") == 0) {
>>> +    if ((strcmp(name, "loadaddr") == 0)&&   (newval != NULL)) {
>>>            load_addr = simple_strtoul(newval, NULL, 16);
>>>            return 0;
>>>        }
>>>    #if defined(CONFIG_CMD_NET)
>>> -    else if (strcmp(name, "bootfile") == 0) {
>>> +    else if ((strcmp(name, "bootfile") == 0)&&   (newval != NULL)) {
>>>            copy_filename(BootFile, newval, sizeof(BootFile));
>>>            return 0;
>>>        }
>>
>
> Best regards,
> Stefano
>

Best regards,
Gerlando

[U-Boot] [PATCH v1] env: fix crash using default -f -a

[Stefano Babic]

On 05/10/2012 12:58, Gerlando Falauto wrote:

> My idea here was to "apply" the changes as soon as the environment gets
> changed, like it would happen with a manual "setenv"/"env set".
> So for instance, if you switch from a custom baudrate back to the
> default baudrate, you get a message prompting you to switch to the new
> baudrate (which actually *DOES* change on the console).

This is exactly the reason I am personally not so convinced to do it.
This forbids to reset the environment in a script. In fact, nobody wants
that a script stops waiting for input.

Think this use case. A board come back from field and in the factory the
original status should be restored. It can be easy done with a script
that contains "env default -f -a", plus parts to restore the original
software. The whole script should run, and not wait for input.

My personal opinion is that who issues such commands (as restoring the
environment, or destroying data in flash, or..) knows what is doing and
it should not be forbidden to do what he minds.

> 
> Whether that should also be performed as a "cleanup" action upon
> restoring defaults, well I am not sure...

I explained my opinion above ;-). But this has nothing to do with the
patch, that must fix a crash.

> 
>>> So by adding these extra checks you're subtly canceling those efforts.
>>
>> That is correct, but in any case a check for a NULL pointer should be
>> done before accessing to the pointer, independently what we want to do
>> with it.
> 
> Absolutely. See my proposed patch.
> 
>>> So I would rather provide a default value to prevent the crashes,
>>> instead of skipping settings altogether. Posting a patch in a few
>>> minutes.
>>
>> Ok, I will review it. In any case, we need a fix for the coming release.
> 
> Absolutely. Once again, I cannot apologize and thank you enough for
> reporting and digging into this.
> I assumed providing an own patch would make more sense than asking you
> to review yours, not sure whether it is popular practice to do so.

Never mind. It is only important to fix the bug ;-)

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================