Re: [U-Boot] Prevent null pointer dereference originating in cmd_pxe.c

[Steven Falco]

Pass a valid cmdtp into do_tftpb(), do_ext2load(), and do_get_fat(), to avoid
possible crashes due to null pointer dereferencing.

Signed-off-by: Steven A. Falco stevenfa...@gmail.com

---

 This doesn't apply cleanly, nor with --ignore-whitespace for me.  Can
 you please re-check and re-send the patch?  Thanks.

Sorry - I've been having trouble getting Thunderbird to leave my text
alone.  There was some insane flowed text setting that I just discovered
and disabled.

I think I've got it right now.  I'll download this email from the list
after I post it, and do a diff to be sure.

Commit d7884e047d08447dfd1374e9fa2fdf7ab36e56f5 does not go far enough.  There
is still at least one call chain that can result in a crash.

The do_tftpb(), do_ext2load(), and do_get_fat() functions expect a valid cmdtp.
Passing in NULL is particularly bad in the do_tftpb() case, because eventually
boot_get_kernel() will be called with a NULL cmdtp:

do_tftpb() - netboot_common() - bootm_maybe_autostart() - do_bootm() -
do_bootm_states() - bootm_find_os() - boot_get_kernel()

Around line 991 in cmd_bootm.c, boot_get_kernel() will dereference the null
pointer, and the board will crash.

diff --git a/common/cmd_pxe.c b/common/cmd_pxe.c
index c5f4a22..79d3a06 100644
--- a/common/cmd_pxe.c
+++ b/common/cmd_pxe.c
@@ -114,16 +114,16 @@ static int get_bootfile_path(const char *file_path, char 
*bootfile_path,
return 1;
 }
 
-static int (*do_getfile)(const char *file_path, char *file_addr);
+static int (*do_getfile)(cmd_tbl_t *cmdtp, const char *file_path, char 
*file_addr);
 
-static int do_get_tftp(const char *file_path, char *file_addr)
+static int do_get_tftp(cmd_tbl_t *cmdtp, const char *file_path, char 
*file_addr)
 {
char *tftp_argv[] = {tftp, NULL, NULL, NULL};
 
tftp_argv[1] = file_addr;
tftp_argv[2] = (void *)file_path;
 
-   if (do_tftpb(NULL, 0, 3, tftp_argv))
+   if (do_tftpb(cmdtp, 0, 3, tftp_argv))
return -ENOENT;
 
return 1;
@@ -131,27 +131,27 @@ static int do_get_tftp(const char *file_path, char 
*file_addr)
 
 static char *fs_argv[5];
 
-static int do_get_ext2(const char *file_path, char *file_addr)
+static int do_get_ext2(cmd_tbl_t *cmdtp, const char *file_path, char 
*file_addr)
 {
 #ifdef CONFIG_CMD_EXT2
fs_argv[0] = ext2load;
fs_argv[3] = file_addr;
fs_argv[4] = (void *)file_path;
 
-   if (!do_ext2load(NULL, 0, 5, fs_argv))
+   if (!do_ext2load(cmdtp, 0, 5, fs_argv))
return 1;
 #endif
return -ENOENT;
 }
 
-static int do_get_fat(const char *file_path, char *file_addr)
+static int do_get_fat(cmd_tbl_t *cmdtp, const char *file_path, char *file_addr)
 {
 #ifdef CONFIG_CMD_FAT
fs_argv[0] = fatload;
fs_argv[3] = file_addr;
fs_argv[4] = (void *)file_path;
 
-   if (!do_fat_fsload(NULL, 0, 5, fs_argv))
+   if (!do_fat_fsload(cmdtp, 0, 5, fs_argv))
return 1;
 #endif
return -ENOENT;
@@ -165,7 +165,7 @@ static int do_get_fat(const char *file_path, char 
*file_addr)
  *
  * Returns 1 for success, or  0 on error.
  */
-static int get_relfile(const char *file_path, void *file_addr)
+static int get_relfile(cmd_tbl_t *cmdtp, const char *file_path, void 
*file_addr)
 {
size_t path_len;
char relfile[MAX_TFTP_PATH_LEN+1];
@@ -194,7 +194,7 @@ static int get_relfile(const char *file_path, void 
*file_addr)
 
sprintf(addr_buf, %p, file_addr);
 
-   return do_getfile(relfile, addr_buf);
+   return do_getfile(cmdtp, relfile, addr_buf);
 }
 
 /*
@@ -204,13 +204,13 @@ static int get_relfile(const char *file_path, void 
*file_addr)
  *
  * Returns 1 on success, or  0 for error.
  */
-static int get_pxe_file(const char *file_path, void *file_addr)
+static int get_pxe_file(cmd_tbl_t *cmdtp, const char *file_path, void 
*file_addr)
 {
unsigned long config_file_size;
char *tftp_filesize;
int err;
 
-   err = get_relfile(file_path, file_addr);
+   err = get_relfile(cmdtp, file_path, file_addr);
 
if (err  0)
return err;
@@ -241,7 +241,7 @@ static int get_pxe_file(const char *file_path, void 
*file_addr)
  *
  * Returns 1 on success or  0 on error.
  */
-static int get_pxelinux_path(const char *file, void *pxefile_addr_r)
+static int get_pxelinux_path(cmd_tbl_t *cmdtp, const char *file, void 
*pxefile_addr_r)
 {
size_t base_len = strlen(PXELINUX_DIR);
char path[MAX_TFTP_PATH_LEN+1];
@@ -254,7 +254,7 @@ static int get_pxelinux_path(const char *file, void 
*pxefile_addr_r)
 
sprintf(path, PXELINUX_DIR %s, file);
 
-   return get_pxe_file(path, pxefile_addr_r);
+   return get_pxe_file(cmdtp, path, pxefile_addr_r);
 }
 
 /*
@@ -262,7 +262,7 @@ static int get_pxelinux_path(const char *file, void 
*pxefile_addr_r)
  *
  * Returns 1 on success or  0 on error.
  */
-static int pxe_uuid_path(void *pxefile_addr_r)
+static int pxe_uuid_path(cmd_tbl_t *cmdtp, void 

Re: [U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard

[Steven Falco]

On 09/23/2013 04:44 PM, Wolfgang Denk wrote:

Dear Tom,

In message 20130923194554.GA5273@bill-the-cat you wrote:


So, I went off an modified cmd_pxe.c to pass around cmdtp so that we get
an error message out, and it's not too bad looking, but it highlights
another problem, which is that we could really use a way to get at least
the is this a ... ? code, and just get the error code, rather than
printouts.  The pxe (and really it's the syslinux.conf/extlinux.conf
parsing) code shouldn't be doing bootm();bootz() but checking the image
type and calling the right boot.


Ah, full ACK  (I should have read the thread to end before posting).

Thanks!

Best regards,

Wolfgang Denk



I understand your point that it is better to fix the problem at the
source.

I also like Tom's suggestion that the type be checked earlier, and
then just directly choose the right bootX() variant.

So naturally, I withdraw my patch, in favor of your fix - at least I
was able to isolate the source of the crash for you. :-)

Steve
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard

[Steven Falco]

Prevent a crash when PXE boot calls do_bootm with a vmlinuz formatted image.
In this case, there will be a null cmdtp pointer, and we must not dereference
it.

Signed-off-by: Steven A. Falco stevenfa...@gmail.com

---

In file cmd_pxe.c around line 687 is a call:

do_bootm(NULL, 0, bootm_argc, bootm_argv);

Notice that the first argument is NULL.  Therefore, the cmdtp
pointer will always be NULL when using the pxe boot mechanism.

do_bootm() eventually calls boot_get_kernel(), still with cmdtp == NULL.
In the Wandboard case, the vmlinuz binary is not legacy format, nor is
it fit format, so U-Boot attempts to print:

printf(Wrong Image Format for %s command\n, cmdtp-name);

That is doomed to fail, because cmdtp is NULL.  The following patch corrects
the problem; the command name will be printed only if the pointer is valid.

diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
index 349f165..2249682 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -985,7 +985,10 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int 
flag, int argc,
break;
 #endif
default:
-   printf(Wrong Image Format for %s command\n, cmdtp-name);
+   if (cmdtp)
+   printf(Wrong Image Format for %s command\n, 
cmdtp-name);
+   else
+   printf(Wrong Image Format for command\n);
bootstage_error(BOOTSTAGE_ID_FIT_KERNEL_INFO);
return NULL;
}

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot