[U-Boot] [PATCH] x86: zImage: avoid potential NULL dereference

[Tom Rini]

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 at 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>