Re: Fwd: [U-Boot] [PATCH] cmd_pxe.c: Pass along 'cmdtp' to do_bootm()/do_bootz()

Steven Falco <stevenfalco@xxxxxxxxx> · Wed, 25 Sep 2013 08:07:16 -0400

On 09/25/2013 02:35 AM, rihoward1@xxxxxxxxx wrote:

On Sep 24, 2013, at 4:29 PM, Steven Falco wrote:

On 09/24/2013 11:37 AM, Jon wrote:
Thanks for reporting this Steven Falco.

We could incorporate this new patch into uboot-tools until we catchup
with upstream.

Does this qualify to put into the alpha as a last minute fix?
Sure would be nice to have for f20.
I am comfortable with that, as it is very low risk.

Tom Rini has submitted a patch http://lists.denx.de/pipermail/u-boot/2013-September/163363.html that goes to the root cause of the crash you saw in cmd_pxe.c

True, but as I said below, it is incomplete.  I have not yet gotten
feedback on my additional findings.

For now, Fedora should take Tom's fix, as it will allow the Wandboards
to boot in the majority of cases.  Longer term, I suspect my additional
patch will be accepted in some form.

	Steve


  I did find
another case where a crash might happen, but it is not an
immediate problem, unless you are pxe booting with tftp.  Then
you might hit it.  I've submitted a patch to the u-boot list with
a fix for that one.

Below is what I submitted there.

	Steve


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@xxxxxxxxx>

---

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");


_______________________________________________
arm mailing list
arm@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/arm




_______________________________________________
arm mailing list
arm@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/arm








