[U-Boot] [PATCH 1/2] mtdparts: fix write through NULL pointer

[U-Boot] [PATCH 1/2] mtdparts: fix write through NULL pointer

[Wolfgang Denk]

The "mtdparts add" command wrote through a NULL pointer - on many
systems this went unnoticed (PowerPC has writable RAM there, some ARM
systems have ROM where a write has no effect), but on arm1136
(i.MX31) it crashed the system.

Add appropriate checks.

Signed-off-by: Wolfgang Denk <wd@denx.de>
---
 common/cmd_mtdparts.c |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/common/cmd_mtdparts.c b/common/cmd_mtdparts.c
index 0b5f747..cec154c 100644
--- a/common/cmd_mtdparts.c
+++ b/common/cmd_mtdparts.c
@@ -837,14 +837,16 @@ static int device_parse(const char *const mtd_dev, const char **ret, struct mtd_
 	u32 offset;
 	int err = 1;
 
-	p = mtd_dev;
+	DEBUGF("===device_parse===\n");
+
+	assert(retdev);
 	*retdev = NULL;
-	*ret = NULL;
 
-	DEBUGF("===device_parse===\n");
+	if (ret)
+		*ret = NULL;
 
 	/* fetch <mtd-id> */
-	mtd_id = p;
+	mtd_id = p = mtd_dev;
 	if (!(p = strchr(mtd_id, ':'))) {
 		printf("no <mtd-id> identifier\n");
 		return 1;
@@ -913,12 +915,15 @@ static int device_parse(const char *const mtd_dev, const char **ret, struct mtd_
 	/* check for next device presence */
 	if (p) {
 		if (*p == ';') {
-			*ret = ++p;
+			if (ret)
+				*ret = ++p;
 		} else if (*p == '\0') {
-			*ret = p;
+			if (ret)
+				*ret = p;
 		} else {
 			printf("unexpected character '%c' at the end of device\n", *p);
-			*ret = NULL;
+			if (ret)
+				*ret = NULL;
 			return 1;
 		}
 	}
-- 
1.6.2.5

[U-Boot] [PATCH 2/2] mtdparts: get rid of custom DEBUG macro, use debug()

[Wolfgang Denk]

Signed-off-by: Wolfgang Denk <wd@denx.de>
---
 common/cmd_mtdparts.c |   90 ++++++++++++++++++++++---------------------------
 1 files changed, 40 insertions(+), 50 deletions(-)

diff --git a/common/cmd_mtdparts.c b/common/cmd_mtdparts.c
index cec154c..116e637 100644
--- a/common/cmd_mtdparts.c
+++ b/common/cmd_mtdparts.c
@@ -103,16 +103,6 @@
 #include <onenand_uboot.h>
 #endif
 
-/* enable/disable debugging messages */
-#define	DEBUG_MTDPARTS
-#undef	DEBUG_MTDPARTS
-
-#ifdef  DEBUG_MTDPARTS
-# define DEBUGF(fmt, args...)	printf(fmt ,##args)
-#else
-# define DEBUGF(fmt, args...)
-#endif
-
 /* special size referring to all the remaining space in a partition */
 #define SIZE_REMAINING		0xFFFFFFFF
 
@@ -243,7 +233,7 @@ static void index_partitions(void)
 	struct list_head *dentry;
 	struct mtd_device *dev;
 
-	DEBUGF("--- index partitions ---\n");
+	debug("--- index partitions ---\n");
 
 	if (current_mtd_dev) {
 		mtddevnum = 0;
@@ -261,12 +251,12 @@ static void index_partitions(void)
 		part = mtd_part_info(current_mtd_dev, current_mtd_partnum);
 		setenv("mtddevname", part->name);
 
-		DEBUGF("=> mtddevnum %d,\n=> mtddevname %s\n", mtddevnum, part->name);
+		debug("=> mtddevnum %d,\n=> mtddevname %s\n", mtddevnum, part->name);
 	} else {
 		setenv("mtddevnum", NULL);
 		setenv("mtddevname", NULL);
 
-		DEBUGF("=> mtddevnum NULL\n=> mtddevname NULL\n");
+		debug("=> mtddevnum NULL\n=> mtddevname NULL\n");
 	}
 }
 
@@ -277,7 +267,7 @@ static void current_save(void)
 {
 	char buf[16];
 
-	DEBUGF("--- current_save ---\n");
+	debug("--- current_save ---\n");
 
 	if (current_mtd_dev) {
 		sprintf(buf, "%s%d,%d", MTD_DEV_TYPE(current_mtd_dev->id->type),
@@ -286,12 +276,12 @@ static void current_save(void)
 		setenv("partition", buf);
 		strncpy(last_partition, buf, 16);
 
-		DEBUGF("=> partition %s\n", buf);
+		debug("=> partition %s\n", buf);
 	} else {
 		setenv("partition", NULL);
 		last_partition[0] = '\0';
 
-		DEBUGF("=> partition NULL\n");
+		debug("=> partition NULL\n");
 	}
 	index_partitions();
 }
@@ -505,7 +495,7 @@ static int part_sort_add(struct mtd_device *dev, struct part_info *part)
 	part->dev = dev;
 
 	if (list_empty(&dev->parts)) {
-		DEBUGF("part_sort_add: list empty\n");
+		debug("part_sort_add: list empty\n");
 		list_add(&part->link, &dev->parts);
 		dev->num_parts++;
 		index_partitions();
@@ -598,7 +588,7 @@ static int part_parse(const char *const partdef, const char **ret, struct part_i
 	/* fetch the partition size */
 	if (*p == '-') {
 		/* assign all remaining space to this partition */
-		DEBUGF("'-': remaining size assigned\n");
+		debug("'-': remaining size assigned\n");
 		size = SIZE_REMAINING;
 		p++;
 	} else {
@@ -683,7 +673,7 @@ static int part_parse(const char *const partdef, const char **ret, struct part_i
 	part->name[name_len - 1] = '\0';
 	INIT_LIST_HEAD(&part->link);
 
-	DEBUGF("+ partition: name %-22s size 0x%08x offset 0x%08x mask flags %d\n",
+	debug("+ partition: name %-22s size 0x%08x offset 0x%08x mask flags %d\n",
 			part->name, part->size,
 			part->offset, part->mask_flags);
 
@@ -837,7 +827,7 @@ static int device_parse(const char *const mtd_dev, const char **ret, struct mtd_
 	u32 offset;
 	int err = 1;
 
-	DEBUGF("===device_parse===\n");
+	debug("===device_parse===\n");
 
 	assert(retdev);
 	*retdev = NULL;
@@ -860,11 +850,11 @@ static int device_parse(const char *const mtd_dev, const char **ret, struct mtd_
 		return 1;
 	}
 
-	DEBUGF("dev type = %d (%s), dev num = %d, mtd-id = %s\n",
+	debug("dev type = %d (%s), dev num = %d, mtd-id = %s\n",
 			id->type, MTD_DEV_TYPE(id->type),
 			id->num, id->mtd_id);
 	pend = strchr(p, ';');
-	DEBUGF("parsing partitions %.*s\n", (pend ? pend - p : strlen(p)), p);
+	debug("parsing partitions %.*s\n", (pend ? pend - p : strlen(p)), p);
 
 
 	/* parse partitions */
@@ -910,7 +900,7 @@ static int device_parse(const char *const mtd_dev, const char **ret, struct mtd_
 		return 1;
 	}
 
-	DEBUGF("\ntotal partitions: %d\n", num_parts);
+	debug("\ntotal partitions: %d\n", num_parts);
 
 	/* check for next device presence */
 	if (p) {
@@ -951,7 +941,7 @@ static int device_parse(const char *const mtd_dev, const char **ret, struct mtd_
 
 	*retdev = dev;
 
-	DEBUGF("===\n\n");
+	debug("===\n\n");
 	return 0;
 }
 
@@ -1003,13 +993,13 @@ static struct mtdids* id_find_by_mtd_id(const char *mtd_id, unsigned int mtd_id_
 	struct list_head *entry;
 	struct mtdids *id;
 
-	DEBUGF("--- id_find_by_mtd_id: '%.*s' (len = %d)\n",
+	debug("--- id_find_by_mtd_id: '%.*s' (len = %d)\n",
 			mtd_id_len, mtd_id, mtd_id_len);
 
 	list_for_each(entry, &mtdids) {
 		id = list_entry(entry, struct mtdids, link);
 
-		DEBUGF("entry: '%s' (len = %d)\n",
+		debug("entry: '%s' (len = %d)\n",
 				id->mtd_id, strlen(id->mtd_id));
 
 		if (mtd_id_len != strlen(id->mtd_id))
@@ -1079,7 +1069,7 @@ static int generate_mtdparts(char *buf, u32 buflen)
 	u32 size, offset, len, part_cnt;
 	u32 maxlen = buflen - 1;
 
-	DEBUGF("--- generate_mtdparts ---\n");
+	debug("--- generate_mtdparts ---\n");
 
 	if (list_empty(&devices)) {
 		buf[0] = '\0';
@@ -1221,7 +1211,7 @@ static void list_partitions(void)
 	struct mtd_device *dev;
 	int part_num;
 
-	DEBUGF("\n---list_partitions---\n");
+	debug("\n---list_partitions---\n");
 	list_for_each(dentry, &devices) {
 		dev = list_entry(dentry, struct mtd_device, link);
 		printf("\ndevice %s%d <%s>, # parts = %d\n",
@@ -1286,7 +1276,7 @@ int find_dev_and_part(const char *id, struct mtd_device **dev,
 	u8 type, dnum, pnum;
 	const char *p;
 
-	DEBUGF("--- find_dev_and_part ---\nid = %s\n", id);
+	debug("--- find_dev_and_part ---\nid = %s\n", id);
 
 	list_for_each(dentry, &devices) {
 		*part_num = 0;
@@ -1347,7 +1337,7 @@ static int delete_partition(const char *id)
 
 	if (find_dev_and_part(id, &dev, &pnum, &part) == 0) {
 
-		DEBUGF("delete_partition: device = %s%d, partition %d = (%s) 0x%08x at 0x%08x\n",
+		debug("delete_partition: device = %s%d, partition %d = (%s) 0x%08x at 0x%08x\n",
 				MTD_DEV_TYPE(dev->id->type), dev->id->num, pnum,
 				part->name, part->size, part->offset);
 
@@ -1378,7 +1368,7 @@ static int parse_mtdparts(const char *const mtdparts)
 	struct mtd_device *dev;
 	int err = 1;
 
-	DEBUGF("\n---parse_mtdparts---\nmtdparts = %s\n\n", p);
+	debug("\n---parse_mtdparts---\nmtdparts = %s\n\n", p);
 
 	/* delete all devices and partitions */
 	if (mtd_devices_init() != 0) {
@@ -1400,7 +1390,7 @@ static int parse_mtdparts(const char *const mtdparts)
 		if ((device_parse(p, &p, &dev) != 0) || (!dev))
 			break;
 
-		DEBUGF("+ device: %s\t%d\t%s\n", MTD_DEV_TYPE(dev->id->type),
+		debug("+ device: %s\t%d\t%s\n", MTD_DEV_TYPE(dev->id->type),
 				dev->id->num, dev->id->mtd_id);
 
 		/* check if parsed device is already on the list */
@@ -1441,12 +1431,12 @@ static int parse_mtdids(const char *const ids)
 	u32 size;
 	int ret = 1;
 
-	DEBUGF("\n---parse_mtdids---\nmtdids = %s\n\n", ids);
+	debug("\n---parse_mtdids---\nmtdids = %s\n\n", ids);
 
 	/* clean global mtdids list */
 	list_for_each_safe(entry, n, &mtdids) {
 		id_tmp = list_entry(entry, struct mtdids, link);
-		DEBUGF("mtdids del: %d %d\n", id_tmp->type, id_tmp->num);
+		debug("mtdids del: %d %d\n", id_tmp->type, id_tmp->num);
 		list_del(entry);
 		free(id_tmp);
 	}
@@ -1512,7 +1502,7 @@ static int parse_mtdids(const char *const ids)
 		id->mtd_id[mtd_id_len - 1] = '\0';
 		INIT_LIST_HEAD(&id->link);
 
-		DEBUGF("+ id %s%d\t%16d bytes\t%s\n",
+		debug("+ id %s%d\t%16d bytes\t%s\n",
 				MTD_DEV_TYPE(id->type), id->num,
 				id->size, id->mtd_id);
 
@@ -1546,7 +1536,7 @@ int mtdparts_init(void)
 	int ids_changed;
 	char tmp_ep[PARTITION_MAXLEN];
 
-	DEBUGF("\n---mtdparts_init---\n");
+	debug("\n---mtdparts_init---\n");
 	if (!initialized) {
 		INIT_LIST_HEAD(&mtdids);
 		INIT_LIST_HEAD(&devices);
@@ -1567,18 +1557,18 @@ int mtdparts_init(void)
 	if (current_partition)
 		strncpy(tmp_ep, current_partition, PARTITION_MAXLEN);
 
-	DEBUGF("last_ids  : %s\n", last_ids);
-	DEBUGF("env_ids   : %s\n", ids);
-	DEBUGF("last_parts: %s\n", last_parts);
-	DEBUGF("env_parts : %s\n\n", parts);
+	debug("last_ids  : %s\n", last_ids);
+	debug("env_ids   : %s\n", ids);
+	debug("last_parts: %s\n", last_parts);
+	debug("env_parts : %s\n\n", parts);
 
-	DEBUGF("last_partition : %s\n", last_partition);
-	DEBUGF("env_partition  : %s\n", current_partition);
+	debug("last_partition : %s\n", last_partition);
+	debug("env_partition  : %s\n", current_partition);
 
 	/* if mtdids varible is empty try to use defaults */
 	if (!ids) {
 		if (mtdids_default) {
-			DEBUGF("mtdids variable not defined, using default\n");
+			debug("mtdids variable not defined, using default\n");
 			ids = mtdids_default;
 			setenv("mtdids", (char *)ids);
 		} else {
@@ -1634,7 +1624,7 @@ int mtdparts_init(void)
 		current_mtd_partnum = 0;
 		current_save();
 
-		DEBUGF("mtdparts_init: current_mtd_dev  = %s%d, current_mtd_partnum = %d\n",
+		debug("mtdparts_init: current_mtd_dev  = %s%d, current_mtd_partnum = %d\n",
 				MTD_DEV_TYPE(current_mtd_dev->id->type),
 				current_mtd_dev->id->num, current_mtd_partnum);
 	}
@@ -1653,7 +1643,7 @@ int mtdparts_init(void)
 		struct mtd_device *cdev;
 		u8 pnum;
 
-		DEBUGF("--- getting current partition: %s\n", tmp_ep);
+		debug("--- getting current partition: %s\n", tmp_ep);
 
 		if (find_dev_and_part(tmp_ep, &cdev, &pnum, &p) == 0) {
 			current_mtd_dev = cdev;
@@ -1661,7 +1651,7 @@ int mtdparts_init(void)
 			current_save();
 		}
 	} else if (getenv("partition") == NULL) {
-		DEBUGF("no partition variable set, setting...\n");
+		debug("no partition variable set, setting...\n");
 		current_save();
 	}
 
@@ -1685,7 +1675,7 @@ static struct part_info* mtd_part_info(struct mtd_device *dev, unsigned int part
 	if (!dev)
 		return NULL;
 
-	DEBUGF("\n--- mtd_part_info: partition number %d for device %s%d (%s)\n",
+	debug("\n--- mtd_part_info: partition number %d for device %s%d (%s)\n",
 			part_num, MTD_DEV_TYPE(dev->id->type),
 			dev->id->num, dev->id->mtd_id);
 
@@ -1821,12 +1811,12 @@ int do_mtdparts(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 		}
 		sprintf(tmpbuf, "%s:%s(%s)%s",
 				id->mtd_id, argv[3], argv[4], argv[5] ? argv[5] : "");
-		DEBUGF("add tmpbuf: %s\n", tmpbuf);
+		debug("add tmpbuf: %s\n", tmpbuf);
 
 		if ((device_parse(tmpbuf, NULL, &dev) != 0) || (!dev))
 			return 1;
 
-		DEBUGF("+ %s\t%d\t%s\n", MTD_DEV_TYPE(dev->id->type),
+		debug("+ %s\t%d\t%s\n", MTD_DEV_TYPE(dev->id->type),
 				dev->id->num, dev->id->mtd_id);
 
 		if ((dev_tmp = device_find(dev->id->type, dev->id->num)) == NULL) {
@@ -1850,7 +1840,7 @@ int do_mtdparts(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 
 	/* mtdparts del part-id */
 	if ((argc == 3) && (strcmp(argv[1], "del") == 0)) {
-		DEBUGF("del: part-id = %s\n", argv[2]);
+		debug("del: part-id = %s\n", argv[2]);
 
 		return delete_partition(argv[2]);
 	}
-- 
1.6.2.5

[U-Boot] [PATCH 1/2] mtdparts: fix write through NULL pointer

[Stefano Babic]

Wolfgang Denk wrote:
> The "mtdparts add" command wrote through a NULL pointer - on many
> systems this went unnoticed (PowerPC has writable RAM there, some ARM
> systems have ROM where a write has no effect), but on arm1136
> (i.MX31) it crashed the system.
> 
> Add appropriate checks.
> 

Hi Wolfgang,

I have already sent a patch fixing this issue,

http://lists.denx.de/pipermail/u-boot/2010-April/070347.html

However, I preferred (easier way) to fix the calling function as the
device_parse(). I do not know if Stefan has already merged it.

Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH 1/2] mtdparts: fix write through NULL pointer

[Stefan Roese]

Hi Stefano,

On Wednesday 28 April 2010 13:12:22 Stefano Babic wrote:
> > The "mtdparts add" command wrote through a NULL pointer - on many
> > systems this went unnoticed (PowerPC has writable RAM there, some ARM
> > systems have ROM where a write has no effect), but on arm1136
> > (i.MX31) it crashed the system.
> > 
> > Add appropriate checks.
> 
> Hi Wolfgang,
> 
> I have already sent a patch fixing this issue,
> 
> http://lists.denx.de/pipermail/u-boot/2010-April/070347.html
> 
> However, I preferred (easier way) to fix the calling function as the
> device_parse(). I do not know if Stefan has already merged it.

No, I haven't merged it. Since I was not sure if this has to go through one of 
my git repositories. Both cfi-flash and ubi don't really include this mtdparts 
stuff.

Wolfgang, I suggest you apply your preferred version directly.

Thanks.

Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de

[U-Boot] [PATCH 1/2] mtdparts: fix write through NULL pointer

[Wolfgang Denk]

Dear Stefano Babic,

In message <4BD81816.2050601@denx.de> you wrote:
>
> I have already sent a patch fixing this issue,

I've seen it (unfortunately only after running into this myself).

> http://lists.denx.de/pipermail/u-boot/2010-April/070347.html
> 
> However, I preferred (easier way) to fix the calling function as the
> device_parse(). I do not know if Stefan has already merged it.

I tend to prefer my version, as it seems to be more defensive, and it
also adds an assert() for the 3rd parameter.

If you agree, I'll apply my version?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Don't you know anything? I should have thought anyone knows that  who
knows anything about anything...      - Terry Pratchett, _Soul Music_

[U-Boot] [PATCH 1/2] mtdparts: fix write through NULL pointer

[Stefano Babic]

Wolfgang Denk wrote:

> I tend to prefer my version, as it seems to be more defensive, and it
> also adds an assert() for the 3rd parameter.
> 
> If you agree, I'll apply my version?

Yes, of course.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================