All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH] x86: zImage: avoid potential NULL dereference
@ 2017-04-15 13:58 Heinrich Schuchardt
  2017-04-15 16:12 ` Tom Rini
  0 siblings, 1 reply; 4+ messages in thread
From: Heinrich Schuchardt @ 2017-04-15 13:58 UTC (permalink / raw)
  To: u-boot

If bootargs is not assigned getenv("bootargs") will
return NULL.
Some part of the code is checking for this condition.
Other parts dereference a possible NULL pointer.

The problem was indicated by cppcheck.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 arch/x86/lib/zimage.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/lib/zimage.c b/arch/x86/lib/zimage.c
index aafbeb01f9..9b564340a6 100644
--- a/arch/x86/lib/zimage.c
+++ b/arch/x86/lib/zimage.c
@@ -48,12 +48,14 @@ static void build_command_line(char *command_line, int auto_boot)
 
 	command_line[0] = '\0';
 
-	env_command_line =  getenv("bootargs");
+	env_command_line = getenv("bootargs");
+
+	if (!env_command_line)
+		env_command_line = "";
 
 	/* set console= argument if we use a serial console */
 	if (!strstr(env_command_line, "console=")) {
 		if (!strcmp(getenv("stdout"), "serial")) {
-
 			/* We seem to use serial console */
 			sprintf(command_line, "console=ttyS0,%s ",
 				getenv("baudrate"));
@@ -63,8 +65,7 @@ static void build_command_line(char *command_line, int auto_boot)
 	if (auto_boot)
 		strcat(command_line, "auto ");
 
-	if (env_command_line)
-		strcat(command_line, env_command_line);
+	strcat(command_line, env_command_line);
 
 	printf("Kernel command line: \"%s\"\n", command_line);
 }
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [U-Boot] [PATCH] x86: zImage: avoid potential NULL dereference
  2017-04-15 13:58 [U-Boot] [PATCH] x86: zImage: avoid potential NULL dereference Heinrich Schuchardt
@ 2017-04-15 16:12 ` Tom Rini
  2017-04-15 16:30   ` Heinrich Schuchardt
  0 siblings, 1 reply; 4+ messages in thread
From: Tom Rini @ 2017-04-15 16:12 UTC (permalink / raw)
  To: u-boot

On Sat, Apr 15, 2017 at 03:58:55PM +0200, Heinrich Schuchardt wrote:

> If bootargs is not assigned getenv("bootargs") will
> return NULL.
> Some part of the code is checking for this condition.
> Other parts dereference a possible NULL pointer.
> 
> The problem was indicated by cppcheck.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  arch/x86/lib/zimage.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/lib/zimage.c b/arch/x86/lib/zimage.c
> index aafbeb01f9..9b564340a6 100644
> --- a/arch/x86/lib/zimage.c
> +++ b/arch/x86/lib/zimage.c
> @@ -48,12 +48,14 @@ static void build_command_line(char *command_line, int auto_boot)
>  
>  	command_line[0] = '\0';
>  
> -	env_command_line =  getenv("bootargs");
> +	env_command_line = getenv("bootargs");
> +
> +	if (!env_command_line)
> +		env_command_line = "";
>  
>  	/* set console= argument if we use a serial console */
>  	if (!strstr(env_command_line, "console=")) {
>  		if (!strcmp(getenv("stdout"), "serial")) {
> -
>  			/* We seem to use serial console */
>  			sprintf(command_line, "console=ttyS0,%s ",
>  				getenv("baudrate"));
> @@ -63,8 +65,7 @@ static void build_command_line(char *command_line, int auto_boot)
>  	if (auto_boot)
>  		strcat(command_line, "auto ");
>  
> -	if (env_command_line)
> -		strcat(command_line, env_command_line);
> +	strcat(command_line, env_command_line);
>  
>  	printf("Kernel command line: \"%s\"\n", command_line);
>  }

I think this is a false positive from cppcheck.  With env_command_line
set to NULL, strstr will return NULL.  The only other place we use
env_command_line is further on where we alrady have a check.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170415/bfa78e10/attachment.sig>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [U-Boot] [PATCH] x86: zImage: avoid potential NULL dereference
  2017-04-15 16:12 ` Tom Rini
@ 2017-04-15 16:30   ` Heinrich Schuchardt
  2017-04-15 18:46     ` Tom Rini
  0 siblings, 1 reply; 4+ messages in thread
From: Heinrich Schuchardt @ 2017-04-15 16:30 UTC (permalink / raw)
  To: u-boot

On 04/15/2017 06:12 PM, Tom Rini wrote:
> On Sat, Apr 15, 2017 at 03:58:55PM +0200, Heinrich Schuchardt wrote:
> 
>> If bootargs is not assigned getenv("bootargs") will
>> return NULL.
>> Some part of the code is checking for this condition.
>> Other parts dereference a possible NULL pointer.
>>
>> The problem was indicated by cppcheck.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  arch/x86/lib/zimage.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/lib/zimage.c b/arch/x86/lib/zimage.c
>> index aafbeb01f9..9b564340a6 100644
>> --- a/arch/x86/lib/zimage.c
>> +++ b/arch/x86/lib/zimage.c
>> @@ -48,12 +48,14 @@ static void build_command_line(char *command_line, int auto_boot)
>>  
>>  	command_line[0] = '\0';
>>  
>> -	env_command_line =  getenv("bootargs");
>> +	env_command_line = getenv("bootargs");
>> +
>> +	if (!env_command_line)
>> +		env_command_line = "";
>>  
>>  	/* set console= argument if we use a serial console */
>>  	if (!strstr(env_command_line, "console=")) {
>>  		if (!strcmp(getenv("stdout"), "serial")) {
>> -
>>  			/* We seem to use serial console */
>>  			sprintf(command_line, "console=ttyS0,%s ",
>>  				getenv("baudrate"));
>> @@ -63,8 +65,7 @@ static void build_command_line(char *command_line, int auto_boot)
>>  	if (auto_boot)
>>  		strcat(command_line, "auto ");
>>  
>> -	if (env_command_line)
>> -		strcat(command_line, env_command_line);
>> +	strcat(command_line, env_command_line);
>>  
>>  	printf("Kernel command line: \"%s\"\n", command_line);
>>  }
> 
> I think this is a false positive from cppcheck.  With env_command_line
> set to NULL, strstr will return NULL.  The only other place we use
> env_command_line is further on where we alrady have a check.  Thanks!
> 

Please, have a look at lib/string.c:
strstr(NULL, b) will happily start searching at 0x0.
So the result will depend on the memory content there.
Should the first bytes be "foo, console=bar\0" the address of "console="
will be returned.
Or maybe a security controller will stop the process due to illegal
memory access.

Best regards

Heinrich Schuchardt

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170415/d30595b2/attachment.sig>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [U-Boot] [PATCH] x86: zImage: avoid potential NULL dereference
  2017-04-15 16:30   ` Heinrich Schuchardt
@ 2017-04-15 18:46     ` Tom Rini
  0 siblings, 0 replies; 4+ messages in thread
From: Tom Rini @ 2017-04-15 18:46 UTC (permalink / raw)
  To: u-boot

On Sat, Apr 15, 2017 at 06:30:21PM +0200, Heinrich Schuchardt wrote:
> On 04/15/2017 06:12 PM, Tom Rini wrote:
> > On Sat, Apr 15, 2017 at 03:58:55PM +0200, Heinrich Schuchardt wrote:
> > 
> >> If bootargs is not assigned getenv("bootargs") will
> >> return NULL.
> >> Some part of the code is checking for this condition.
> >> Other parts dereference a possible NULL pointer.
> >>
> >> The problem was indicated by cppcheck.
> >>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> ---
> >>  arch/x86/lib/zimage.c | 9 +++++----
> >>  1 file changed, 5 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/x86/lib/zimage.c b/arch/x86/lib/zimage.c
> >> index aafbeb01f9..9b564340a6 100644
> >> --- a/arch/x86/lib/zimage.c
> >> +++ b/arch/x86/lib/zimage.c
> >> @@ -48,12 +48,14 @@ static void build_command_line(char *command_line, int auto_boot)
> >>  
> >>  	command_line[0] = '\0';
> >>  
> >> -	env_command_line =  getenv("bootargs");
> >> +	env_command_line = getenv("bootargs");
> >> +
> >> +	if (!env_command_line)
> >> +		env_command_line = "";
> >>  
> >>  	/* set console= argument if we use a serial console */
> >>  	if (!strstr(env_command_line, "console=")) {
> >>  		if (!strcmp(getenv("stdout"), "serial")) {
> >> -
> >>  			/* We seem to use serial console */
> >>  			sprintf(command_line, "console=ttyS0,%s ",
> >>  				getenv("baudrate"));
> >> @@ -63,8 +65,7 @@ static void build_command_line(char *command_line, int auto_boot)
> >>  	if (auto_boot)
> >>  		strcat(command_line, "auto ");
> >>  
> >> -	if (env_command_line)
> >> -		strcat(command_line, env_command_line);
> >> +	strcat(command_line, env_command_line);
> >>  
> >>  	printf("Kernel command line: \"%s\"\n", command_line);
> >>  }
> > 
> > I think this is a false positive from cppcheck.  With env_command_line
> > set to NULL, strstr will return NULL.  The only other place we use
> > env_command_line is further on where we alrady have a check.  Thanks!
> > 
> 
> Please, have a look at lib/string.c:
> strstr(NULL, b) will happily start searching at 0x0.
> So the result will depend on the memory content there.
> Should the first bytes be "foo, console=bar\0" the address of "console="
> will be returned.
> Or maybe a security controller will stop the process due to illegal
> memory access.

OK, thanks.  But now, reading over the whole function again, I think
build_command_line() needs some thinking and re-work.  If baudrate isn't
set and auto_boot is we'll loose the space between our generated
console= and auto.  Can you try and re-work this so all of the corner
cases are right?  It shouldn't be too bad since you can boot up Linux
under qemu here.  Thanks for all of the patches btw!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170415/771dfd31/attachment.sig>

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2017-04-15 18:46 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-15 13:58 [U-Boot] [PATCH] x86: zImage: avoid potential NULL dereference Heinrich Schuchardt
2017-04-15 16:12 ` Tom Rini
2017-04-15 16:30   ` Heinrich Schuchardt
2017-04-15 18:46     ` Tom Rini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.