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

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 <stevenfalco@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.

It's good to write commit message head for easy to track the fix from where.

cmd_pxe: Prevent null pointer deference

For more info, please look at here.
http://www.denx.de/wiki/U-Boot/Patches


On Mon, Oct 7, 2013 at 7:21 PM, Steven Falco <stevenfalco@gmail.com> wrote:
> 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 <stevenfalco@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 *pxefile_addr_r)
>  {
>         char *uuid_str;
>
> @@ -271,7 +271,7 @@ static int pxe_uuid_path(void *pxefile_addr_r)
>         if (!uuid_str)
>                 return -ENOENT;
>
> -       return get_pxelinux_path(uuid_str, pxefile_addr_r);
> +       return get_pxelinux_path(cmdtp, uuid_str, pxefile_addr_r);
>  }
>
>  /*
> @@ -280,7 +280,7 @@ static int pxe_uuid_path(void *pxefile_addr_r)
>   *
>   * Returns 1 on success or < 0 on error.
>   */
> -static int pxe_mac_path(void *pxefile_addr_r)
> +static int pxe_mac_path(cmd_tbl_t *cmdtp, void *pxefile_addr_r)
>  {
>         char mac_str[21];
>         int err;
> @@ -290,7 +290,7 @@ static int pxe_mac_path(void *pxefile_addr_r)
>         if (err < 0)
>                 return err;
>
> -       return get_pxelinux_path(mac_str, pxefile_addr_r);
> +       return get_pxelinux_path(cmdtp, mac_str, pxefile_addr_r);
>  }
>
>  /*
> @@ -300,7 +300,7 @@ static int pxe_mac_path(void *pxefile_addr_r)
>   *
>   * Returns 1 on success or < 0 on error.
>   */
> -static int pxe_ipaddr_paths(void *pxefile_addr_r)
> +static int pxe_ipaddr_paths(cmd_tbl_t *cmdtp, void *pxefile_addr_r)
>  {
>         char ip_addr[9];
>         int mask_pos, err;
> @@ -308,7 +308,7 @@ static int pxe_ipaddr_paths(void *pxefile_addr_r)
>         sprintf(ip_addr, "%08X", ntohl(NetOurIP));
>
>         for (mask_pos = 7; mask_pos >= 0;  mask_pos--) {
> -               err = get_pxelinux_path(ip_addr, pxefile_addr_r);
> +               err = get_pxelinux_path(cmdtp, ip_addr, pxefile_addr_r);
>
>                 if (err > 0)
>                         return err;
> @@ -359,16 +359,16 @@ do_pxe_get(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>          * Keep trying paths until we successfully get a file we're looking
>          * for.
>          */
> -       if (pxe_uuid_path((void *)pxefile_addr_r) > 0 ||
> -           pxe_mac_path((void *)pxefile_addr_r) > 0 ||
> -           pxe_ipaddr_paths((void *)pxefile_addr_r) > 0) {
> +       if (pxe_uuid_path(cmdtp, (void *)pxefile_addr_r) > 0 ||
> +           pxe_mac_path(cmdtp, (void *)pxefile_addr_r) > 0 ||
> +           pxe_ipaddr_paths(cmdtp, (void *)pxefile_addr_r) > 0) {
>                 printf("Config file found\n");
>
>                 return 0;
>         }
>
>         while (pxe_default_paths[i]) {
> -               if (get_pxelinux_path(pxe_default_paths[i],
> +               if (get_pxelinux_path(cmdtp, pxe_default_paths[i],
>                                       (void *)pxefile_addr_r) > 0) {
>                         printf("Config file found\n");
>                         return 0;
> @@ -388,7 +388,7 @@ do_pxe_get(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   *
>   * Returns 1 on success or < 0 on error.
>   */
> -static int get_relfile_envaddr(const char *file_path, const char *envaddr_name)
> +static int get_relfile_envaddr(cmd_tbl_t *cmdtp, const char *file_path, const char *envaddr_name)
>  {
>         unsigned long file_addr;
>         char *envaddr;
> @@ -401,7 +401,7 @@ static int get_relfile_envaddr(const char *file_path, const char *envaddr_name)
>         if (strict_strtoul(envaddr, 16, &file_addr) < 0)
>                 return -EINVAL;
>
> -       return get_relfile(file_path, (void *)file_addr);
> +       return get_relfile(cmdtp, file_path, (void *)file_addr);
>  }
>
>  /*
> @@ -599,7 +599,7 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label)
>         }
>
>         if (label->initrd) {
> -               if (get_relfile_envaddr(label->initrd, "ramdisk_addr_r") < 0) {
> +               if (get_relfile_envaddr(cmdtp, label->initrd, "ramdisk_addr_r") < 0) {
>                         printf("Skipping %s for failure retrieving initrd\n",
>                                         label->name);
>                         return 1;
> @@ -613,7 +613,7 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label)
>                 bootm_argv[2] = "-";
>         }
>
> -       if (get_relfile_envaddr(label->kernel, "kernel_addr_r") < 0) {
> +       if (get_relfile_envaddr(cmdtp, label->kernel, "kernel_addr_r") < 0) {
>                 printf("Skipping %s for failure retrieving kernel\n",
>                                 label->name);
>                 return 1;
> @@ -673,7 +673,7 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label)
>
>         /* if fdt label is defined then get fdt from server */
>         if (bootm_argv[3] && label->fdt) {
> -               if (get_relfile_envaddr(label->fdt, "fdt_addr_r") < 0) {
> +               if (get_relfile_envaddr(cmdtp, label->fdt, "fdt_addr_r") < 0) {
>                         printf("Skipping %s for failure retrieving fdt\n",
>                                         label->name);
>                         return 1;
> @@ -950,7 +950,7 @@ static int parse_integer(char **c, int *dst)
>         return 1;
>  }
>
> -static int parse_pxefile_top(char *p, struct pxe_menu *cfg, int nest_level);
> +static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, int nest_level);
>
>  /*
>   * Parse an include statement, and retrieve and parse the file it mentions.
> @@ -960,7 +960,7 @@ static int parse_pxefile_top(char *p, struct pxe_menu *cfg, int nest_level);
>   * include, nest_level has already been incremented and doesn't need to be
>   * incremented here.
>   */
> -static int handle_include(char **c, char *base,
> +static int handle_include(cmd_tbl_t *cmdtp, char **c, char *base,
>                                 struct pxe_menu *cfg, int nest_level)
>  {
>         char *include_path;
> @@ -975,14 +975,14 @@ static int handle_include(char **c, char *base,
>                 return err;
>         }
>
> -       err = get_pxe_file(include_path, base);
> +       err = get_pxe_file(cmdtp, include_path, base);
>
>         if (err < 0) {
>                 printf("Couldn't retrieve %s\n", include_path);
>                 return err;
>         }
>
> -       return parse_pxefile_top(base, cfg, nest_level);
> +       return parse_pxefile_top(cmdtp, base, cfg, nest_level);
>  }
>
>  /*
> @@ -995,7 +995,7 @@ static int handle_include(char **c, char *base,
>   * nest_level should be 1 when parsing the top level pxe file, 2 when parsing
>   * a file it includes, 3 when parsing a file included by that file, and so on.
>   */
> -static int parse_menu(char **c, struct pxe_menu *cfg, char *b, int nest_level)
> +static int parse_menu(cmd_tbl_t *cmdtp, char **c, struct pxe_menu *cfg, char *b, int nest_level)
>  {
>         struct token t;
>         char *s = *c;
> @@ -1010,7 +1010,7 @@ static int parse_menu(char **c, struct pxe_menu *cfg, char *b, int nest_level)
>                 break;
>
>         case T_INCLUDE:
> -               err = handle_include(c, b + strlen(b) + 1, cfg,
> +               err = handle_include(cmdtp, c, b + strlen(b) + 1, cfg,
>                                                 nest_level + 1);
>                 break;
>
> @@ -1172,7 +1172,7 @@ static int parse_label(char **c, struct pxe_menu *cfg)
>   *
>   * Returns 1 on success, < 0 on error.
>   */
> -static int parse_pxefile_top(char *p, struct pxe_menu *cfg, int nest_level)
> +static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, int nest_level)
>  {
>         struct token t;
>         char *s, *b, *label_name;
> @@ -1194,7 +1194,7 @@ static int parse_pxefile_top(char *p, struct pxe_menu *cfg, int nest_level)
>                 switch (t.type) {
>                 case T_MENU:
>                         cfg->prompt = 1;
> -                       err = parse_menu(&p, cfg, b, nest_level);
> +                       err = parse_menu(cmdtp, &p, cfg, b, nest_level);
>                         break;
>
>                 case T_TIMEOUT:
> @@ -1219,7 +1219,7 @@ static int parse_pxefile_top(char *p, struct pxe_menu *cfg, int nest_level)
>                         break;
>
>                 case T_INCLUDE:
> -                       err = handle_include(&p, b + ALIGN(strlen(b), 4), cfg,
> +                       err = handle_include(cmdtp, &p, b + ALIGN(strlen(b), 4), cfg,
>                                                         nest_level + 1);
>                         break;
>
> @@ -1276,7 +1276,7 @@ static void destroy_pxe_menu(struct pxe_menu *cfg)
>   * files it includes). The resulting pxe_menu struct can be free()'d by using
>   * the destroy_pxe_menu() function.
>   */
> -static struct pxe_menu *parse_pxefile(char *menucfg)
> +static struct pxe_menu *parse_pxefile(cmd_tbl_t *cmdtp, char *menucfg)
>  {
>         struct pxe_menu *cfg;
>
> @@ -1289,7 +1289,7 @@ static struct pxe_menu *parse_pxefile(char *menucfg)
>
>         INIT_LIST_HEAD(&cfg->labels);
>
> -       if (parse_pxefile_top(menucfg, cfg, 1) < 0) {
> +       if (parse_pxefile_top(cmdtp, menucfg, cfg, 1) < 0) {
>                 destroy_pxe_menu(cfg);
>                 return NULL;
>         }
> @@ -1446,7 +1446,7 @@ do_pxe_boot(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>                 return 1;
>         }
>
> -       cfg = parse_pxefile((char *)(pxefile_addr_r));
> +       cfg = parse_pxefile(cmdtp, (char *)(pxefile_addr_r));
>
>         if (cfg == NULL) {
>                 printf("Error parsing config file\n");
> @@ -1544,12 +1544,12 @@ int do_sysboot(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>                 return 1;
>         }
>
> -       if (get_pxe_file(filename, (void *)pxefile_addr_r) < 0) {
> +       if (get_pxe_file(cmdtp, filename, (void *)pxefile_addr_r) < 0) {
>                 printf("Error reading config file\n");
>                 return 1;
>         }
>
> -       cfg = parse_pxefile((char *)(pxefile_addr_r));
> +       cfg = parse_pxefile(cmdtp, (char *)(pxefile_addr_r));
>
>         if (cfg == NULL) {
>                 printf("Error parsing config file\n");
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

On Mon, Oct 7, 2013 at 10:51 AM, Steven Falco <stevenfalco@gmail.com> wrote:

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

Better send patches via 'git send-email' as this can avoid these kind
of email client issues.

On Mon, Oct 07, 2013 at 09:51:48AM -0400, Steven Falco wrote:

> 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 <stevenfalco@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.

With a reworded commit message to include more details, applied to
u-boot/master, thanks!