[PATCH 0/5] DFU: Update dfu_alt_info parser etc.

[PATCH 0/5] DFU: Update dfu_alt_info parser etc.

[Masami Hiramatsu]

Hi,

This series is improving DFU subsystem. This improves dfu_alt_info
parser and fixing documents etc.

When I was debuging my patch for updating dfu_alt_info on the
DeveloperBox platform, I found that dfu_alt_info parser doesn't
accept redundant spaces and tabs. Also the dfu.rst description
seems wrong. Moreover, there is no way to check whether the
parser parses the dfu_alt_info correctly.

These patches fixes such issues. [1/5] is just for avoiding
buffer overrun, [2/5] and [3/5] improves dfu_alt_info parser
to accept redundant spaces and tabs, and check the number of
arguments strictly so that the parser (and user) can notice
any unexpected parameters. [4/5] fixes the documents (there
seems some wrong description maybe coming from copy&paste).
[5/5] allows user to run 'dfu list' even if the platform
doesn't support DFU_OVER_USB.

Thank you,

---

Masami Hiramatsu (5):
      DFU: Do not copy the entity name over the buffer size
      DFU: Accept redundant spaces and tabs in dfu_alt_info
      DFU: Check the number of arguments and argument string strictly
      doc: usage: DFU: Fix dfu_alt_info document
      cmd/dfu: Enable 'dfu list' command without DFU_OVER_USB


 cmd/dfu.c              |    6 +++--
 doc/usage/dfu.rst      |   57 ++++++++++++++++++++++++++++++++++--------------
 drivers/dfu/dfu.c      |   37 ++++++++++++++++++++++++-------
 drivers/dfu/dfu_mmc.c  |   55 +++++++++++++++++++++++++++-------------------
 drivers/dfu/dfu_mtd.c  |   34 +++++++++++++++++++----------
 drivers/dfu/dfu_nand.c |   34 ++++++++++++++++++-----------
 drivers/dfu/dfu_ram.c  |   24 ++++++++++----------
 drivers/dfu/dfu_sf.c   |   34 ++++++++++++++++++-----------
 drivers/dfu/dfu_virt.c |    5 +++-
 include/dfu.h          |   33 ++++++++++++++++++----------
 10 files changed, 205 insertions(+), 114 deletions(-)

--
Masami Hiramatsu <masami.hiramatsu@linaro.org>

[PATCH 1/5] DFU: Do not copy the entity name over the buffer size

[Masami Hiramatsu]

Use strlcpy() instead of strcpy() to prevent copying the
entity name over the name buffer size.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
 drivers/dfu/dfu.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
index af3975925a..66c41b5e76 100644
--- a/drivers/dfu/dfu.c
+++ b/drivers/dfu/dfu.c
@@ -503,7 +503,7 @@ static int dfu_fill_entity(struct dfu_entity *dfu, char *s, int alt,
 
 	debug("%s: %s interface: %s dev: %s\n", __func__, s, interface, devstr);
 	st = strsep(&s, " ");
-	strcpy(dfu->name, st);
+	strlcpy(dfu->name, st, DFU_NAME_SIZE);
 
 	dfu->alt = alt;
 	dfu->max_buf_size = 0;

[PATCH 2/5] DFU: Accept redundant spaces and tabs in dfu_alt_info

[Masami Hiramatsu]

If dfu_alt_info has repeated spaces or tab (for indentation or
readability), the dfu fails to parse it. For example, if
dfu_alt_info="mtd nor1=image raw  100000 200000" (double spaces
after "raw"), the image entity start address is '0' and the size
'0x100000'. This is because the repeated space is not skipped.

Use space and tab as a separater and apply skip_spaces() to
skip redundant spaces and tabs.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
 drivers/dfu/dfu.c      |    6 ++++--
 drivers/dfu/dfu_mmc.c  |   11 ++++++++---
 drivers/dfu/dfu_mtd.c  |    8 ++++++--
 drivers/dfu/dfu_nand.c |   11 ++++++++---
 drivers/dfu/dfu_ram.c  |    3 ++-
 drivers/dfu/dfu_sf.c   |   12 +++++++++---
 6 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
index 66c41b5e76..18154774f9 100644
--- a/drivers/dfu/dfu.c
+++ b/drivers/dfu/dfu.c
@@ -123,9 +123,10 @@ int dfu_config_interfaces(char *env)
 	s = env;
 	while (s) {
 		ret = -EINVAL;
-		i = strsep(&s, " ");
+		i = strsep(&s, " \t");
 		if (!i)
 			break;
+		s = skip_spaces(s);
 		d = strsep(&s, "=");
 		if (!d)
 			break;
@@ -502,8 +503,9 @@ static int dfu_fill_entity(struct dfu_entity *dfu, char *s, int alt,
 	char *st;
 
 	debug("%s: %s interface: %s dev: %s\n", __func__, s, interface, devstr);
-	st = strsep(&s, " ");
+	st = strsep(&s, " \t");
 	strlcpy(dfu->name, st, DFU_NAME_SIZE);
+	s = skip_spaces(s);
 
 	dfu->alt = alt;
 	dfu->max_buf_size = 0;
diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
index 3dab5a5f63..d747ede66c 100644
--- a/drivers/dfu/dfu_mmc.c
+++ b/drivers/dfu/dfu_mmc.c
@@ -351,11 +351,12 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s)
 	dfu->data.mmc.dev_num = dectoul(devstr, NULL);
 
 	for (; parg < argv + sizeof(argv) / sizeof(*argv); ++parg) {
-		*parg = strsep(&s, " ");
+		*parg = strsep(&s, " \t");
 		if (*parg == NULL) {
 			pr_err("Invalid number of arguments.\n");
 			return -ENODEV;
 		}
+		s = skip_spaces(s);
 	}
 
 	entity_type = argv[0];
@@ -390,9 +391,11 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s)
 		 * specifying the mmc HW defined partition number
 		 */
 		if (s)
-			if (!strcmp(strsep(&s, " "), "mmcpart"))
+			if (!strcmp(strsep(&s, " \t"), "mmcpart")) {
+				s = skip_spaces(s);
 				dfu->data.mmc.hw_partition =
 					simple_strtoul(s, NULL, 0);
+			}
 
 	} else if (!strcmp(entity_type, "part")) {
 		struct disk_partition partinfo;
@@ -412,8 +415,10 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s)
 		 * specifying the mmc HW defined partition number
 		 */
 		if (s)
-			if (!strcmp(strsep(&s, " "), "offset"))
+			if (!strcmp(strsep(&s, " \t"), "offset")) {
+				s = skip_spaces(s);
 				offset = simple_strtoul(s, NULL, 0);
+			}
 
 		dfu->layout			= DFU_RAW_ADDR;
 		dfu->data.mmc.lba_start		= partinfo.start + offset;
diff --git a/drivers/dfu/dfu_mtd.c b/drivers/dfu/dfu_mtd.c
index 0b7f17761f..ee534a9d10 100644
--- a/drivers/dfu/dfu_mtd.c
+++ b/drivers/dfu/dfu_mtd.c
@@ -12,6 +12,7 @@
 #include <mtd.h>
 #include <jffs2/load_kernel.h>
 #include <linux/err.h>
+#include <linux/ctype.h>
 
 static bool mtd_is_aligned_with_block_size(struct mtd_info *mtd, u64 size)
 {
@@ -265,11 +266,14 @@ int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, char *s)
 	dfu->data.mtd.info = mtd;
 	dfu->max_buf_size = mtd->erasesize;
 
-	st = strsep(&s, " ");
+	st = strsep(&s, " \t");
+	s = skip_spaces(s);
 	if (!strcmp(st, "raw")) {
 		dfu->layout = DFU_RAW_ADDR;
 		dfu->data.mtd.start = hextoul(s, &s);
-		s++;
+		if (!isspace(*s))
+			return -1;
+		s = skip_spaces(s);
 		dfu->data.mtd.size = hextoul(s, &s);
 	} else if ((!strcmp(st, "part")) || (!strcmp(st, "partubi"))) {
 		char mtd_id[32];
diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c
index e53b35e42b..76761939ab 100644
--- a/drivers/dfu/dfu_nand.c
+++ b/drivers/dfu/dfu_nand.c
@@ -201,11 +201,14 @@ int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char *s)
 
 	dfu->data.nand.ubi = 0;
 	dfu->dev_type = DFU_DEV_NAND;
-	st = strsep(&s, " ");
+	st = strsep(&s, " \t");
+	s = skip_spaces(s);
 	if (!strcmp(st, "raw")) {
 		dfu->layout = DFU_RAW_ADDR;
 		dfu->data.nand.start = hextoul(s, &s);
-		s++;
+		if (!isspace(*s))
+			return -1;
+		s = skip_spaces(s);
 		dfu->data.nand.size = hextoul(s, &s);
 	} else if ((!strcmp(st, "part")) || (!strcmp(st, "partubi"))) {
 		char mtd_id[32];
@@ -216,7 +219,9 @@ int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char *s)
 		dfu->layout = DFU_RAW_ADDR;
 
 		dev = dectoul(s, &s);
-		s++;
+		if (!isspace(*s))
+			return -1;
+		s = skip_spaces(s);
 		part = dectoul(s, &s);
 
 		sprintf(mtd_id, "%s%d,%d", "nand", dev, part - 1);
diff --git a/drivers/dfu/dfu_ram.c b/drivers/dfu/dfu_ram.c
index cc7e45ba33..361a3ff8af 100644
--- a/drivers/dfu/dfu_ram.c
+++ b/drivers/dfu/dfu_ram.c
@@ -60,11 +60,12 @@ int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr, char *s)
 	const char **parg = argv;
 
 	for (; parg < argv + sizeof(argv) / sizeof(*argv); ++parg) {
-		*parg = strsep(&s, " ");
+		*parg = strsep(&s, " \t");
 		if (*parg == NULL) {
 			pr_err("Invalid number of arguments.\n");
 			return -ENODEV;
 		}
+		s = skip_spaces(s);
 	}
 
 	dfu->dev_type = DFU_DEV_RAM;
diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c
index b72493ced8..993e951bc3 100644
--- a/drivers/dfu/dfu_sf.c
+++ b/drivers/dfu/dfu_sf.c
@@ -13,6 +13,7 @@
 #include <spi_flash.h>
 #include <jffs2/load_kernel.h>
 #include <linux/mtd/mtd.h>
+#include <linux/ctype.h>
 
 static int dfu_get_medium_size_sf(struct dfu_entity *dfu, u64 *size)
 {
@@ -178,11 +179,14 @@ int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char *s)
 	dfu->dev_type = DFU_DEV_SF;
 	dfu->max_buf_size = dfu->data.sf.dev->sector_size;
 
-	st = strsep(&s, " ");
+	st = strsep(&s, " \t");
+	s = skip_spaces(s);
 	if (!strcmp(st, "raw")) {
 		dfu->layout = DFU_RAW_ADDR;
 		dfu->data.sf.start = hextoul(s, &s);
-		s++;
+		if (!isspace(*s))
+			return -1;
+		s = skip_spaces(s);
 		dfu->data.sf.size = hextoul(s, &s);
 	} else if (CONFIG_IS_ENABLED(DFU_SF_PART) &&
 		   (!strcmp(st, "part") || !strcmp(st, "partubi"))) {
@@ -195,7 +199,9 @@ int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char *s)
 		dfu->layout = DFU_RAW_ADDR;
 
 		dev = dectoul(s, &s);
-		s++;
+		if (!isspace(*s))
+			return -1;
+		s = skip_spaces(s);
 		part = dectoul(s, &s);
 
 		sprintf(mtd_id, "%s%d,%d", "nor", dev, part - 1);

[PATCH 3/5] DFU: Check the number of arguments and argument string strictly

[Masami Hiramatsu]

When parsing the dfu_alt_info, check the number of arguments
and argument string strictly. If there is any garbage data
(which is not able to be parsed correctly) in dfu_alt_info,
that means something wrong and user may make a typo or mis-
understanding about the syntax. Since the dfu_alt_info is
used for updating the firmware, this mistake may lead to
brick the hardware.
Thus it should be checked strictly for making sure there
is no mistake.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
 drivers/dfu/dfu.c      |   31 +++++++++++++++++++++------
 drivers/dfu/dfu_mmc.c  |   56 ++++++++++++++++++++++++++----------------------
 drivers/dfu/dfu_mtd.c  |   36 +++++++++++++++++++------------
 drivers/dfu/dfu_nand.c |   39 ++++++++++++++++++---------------
 drivers/dfu/dfu_ram.c  |   25 ++++++++++-----------
 drivers/dfu/dfu_sf.c   |   38 +++++++++++++++++----------------
 drivers/dfu/dfu_virt.c |    5 +++-
 include/dfu.h          |   33 ++++++++++++++++++----------
 8 files changed, 154 insertions(+), 109 deletions(-)

diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
index 18154774f9..516dda6179 100644
--- a/drivers/dfu/dfu.c
+++ b/drivers/dfu/dfu.c
@@ -500,12 +500,29 @@ int dfu_read(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
 static int dfu_fill_entity(struct dfu_entity *dfu, char *s, int alt,
 			   char *interface, char *devstr)
 {
+	char *argv[DFU_MAX_ENTITY_ARGS];
+	int argc;
 	char *st;
 
 	debug("%s: %s interface: %s dev: %s\n", __func__, s, interface, devstr);
 	st = strsep(&s, " \t");
 	strlcpy(dfu->name, st, DFU_NAME_SIZE);
-	s = skip_spaces(s);
+
+	/* Parse arguments */
+	for (argc = 0; s && argc < DFU_MAX_ENTITY_ARGS; argc++) {
+		s = skip_spaces(s);
+		if (!*s)
+			break;
+		argv[argc] = strsep(&s, " \t");
+	}
+
+	if (argc == DFU_MAX_ENTITY_ARGS && s) {
+		s = skip_spaces(s);
+		if (*s) {
+			log_err("Too many arguments for %s\n", dfu->name);
+			return -EINVAL;
+		}
+	}
 
 	dfu->alt = alt;
 	dfu->max_buf_size = 0;
@@ -513,22 +530,22 @@ static int dfu_fill_entity(struct dfu_entity *dfu, char *s, int alt,
 
 	/* Specific for mmc device */
 	if (strcmp(interface, "mmc") == 0) {
-		if (dfu_fill_entity_mmc(dfu, devstr, s))
+		if (dfu_fill_entity_mmc(dfu, devstr, argv, argc))
 			return -1;
 	} else if (strcmp(interface, "mtd") == 0) {
-		if (dfu_fill_entity_mtd(dfu, devstr, s))
+		if (dfu_fill_entity_mtd(dfu, devstr, argv, argc))
 			return -1;
 	} else if (strcmp(interface, "nand") == 0) {
-		if (dfu_fill_entity_nand(dfu, devstr, s))
+		if (dfu_fill_entity_nand(dfu, devstr, argv, argc))
 			return -1;
 	} else if (strcmp(interface, "ram") == 0) {
-		if (dfu_fill_entity_ram(dfu, devstr, s))
+		if (dfu_fill_entity_ram(dfu, devstr, argv, argc))
 			return -1;
 	} else if (strcmp(interface, "sf") == 0) {
-		if (dfu_fill_entity_sf(dfu, devstr, s))
+		if (dfu_fill_entity_sf(dfu, devstr, argv, argc))
 			return -1;
 	} else if (strcmp(interface, "virt") == 0) {
-		if (dfu_fill_entity_virt(dfu, devstr, s))
+		if (dfu_fill_entity_virt(dfu, devstr, argv, argc))
 			return -1;
 	} else {
 		printf("%s: Device %s not (yet) supported!\n",
diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
index d747ede66c..a91da972d4 100644
--- a/drivers/dfu/dfu_mmc.c
+++ b/drivers/dfu/dfu_mmc.c
@@ -337,35 +337,34 @@ void dfu_free_entity_mmc(struct dfu_entity *dfu)
  *	4th (optional):
  *		mmcpart <num> (access to HW eMMC partitions)
  */
-int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s)
+int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char **argv, int argc)
 {
 	const char *entity_type;
 	ssize_t second_arg;
 	size_t third_arg;
-
 	struct mmc *mmc;
+	char *s;
 
-	const char *argv[3];
-	const char **parg = argv;
-
-	dfu->data.mmc.dev_num = dectoul(devstr, NULL);
-
-	for (; parg < argv + sizeof(argv) / sizeof(*argv); ++parg) {
-		*parg = strsep(&s, " \t");
-		if (*parg == NULL) {
-			pr_err("Invalid number of arguments.\n");
-			return -ENODEV;
-		}
-		s = skip_spaces(s);
+	if (argc < 3) {
+		pr_err("The number of parameters are not enough.\n");
+		return -EINVAL;
 	}
 
+	dfu->data.mmc.dev_num = dectoul(devstr, &s);
+	if (*s)
+		return -EINVAL;
+
 	entity_type = argv[0];
 	/*
 	 * Base 0 means we'll accept (prefixed with 0x or 0) base 16, 8,
 	 * with default 10.
 	 */
-	second_arg = simple_strtol(argv[1], NULL, 0);
-	third_arg = simple_strtoul(argv[2], NULL, 0);
+	second_arg = simple_strtol(argv[1], &s, 0);
+	if (*s)
+		return -EINVAL;
+	third_arg = simple_strtoul(argv[2], &s, 0);
+	if (*s)
+		return -EINVAL;
 
 	mmc = find_mmc_device(dfu->data.mmc.dev_num);
 	if (mmc == NULL) {
@@ -390,12 +389,14 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s)
 		 * Check for an extra entry at dfu_alt_info env variable
 		 * specifying the mmc HW defined partition number
 		 */
-		if (s)
-			if (!strcmp(strsep(&s, " \t"), "mmcpart")) {
-				s = skip_spaces(s);
-				dfu->data.mmc.hw_partition =
-					simple_strtoul(s, NULL, 0);
+		if (argc > 3) {
+			if (argc != 5 || strcmp(argv[3], "mmcpart")) {
+				pr_err("DFU mmc raw accept 'mmcpart <partnum>' option.\n");
+				return -EINVAL;
 			}
+			dfu->data.mmc.hw_partition =
+				simple_strtoul(argv[4], NULL, 0);
+		}
 
 	} else if (!strcmp(entity_type, "part")) {
 		struct disk_partition partinfo;
@@ -414,15 +415,18 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s)
 		 * Check for an extra entry at dfu_alt_info env variable
 		 * specifying the mmc HW defined partition number
 		 */
-		if (s)
-			if (!strcmp(strsep(&s, " \t"), "offset")) {
-				s = skip_spaces(s);
-				offset = simple_strtoul(s, NULL, 0);
+		if (argc > 3) {
+			if (argc != 5 || strcmp(argv[3], "offset")) {
+				pr_err("DFU mmc raw accept 'mmcpart <partnum>' option.\n");
+				return -EINVAL;
 			}
+			dfu->data.mmc.hw_partition =
+				simple_strtoul(argv[4], NULL, 0);
+		}
 
 		dfu->layout			= DFU_RAW_ADDR;
 		dfu->data.mmc.lba_start		= partinfo.start + offset;
-		dfu->data.mmc.lba_size		= partinfo.size-offset;
+		dfu->data.mmc.lba_size		= partinfo.size - offset;
 		dfu->data.mmc.lba_blk_size	= partinfo.blksz;
 	} else if (!strcmp(entity_type, "fat")) {
 		dfu->layout = DFU_FS_FAT;
diff --git a/drivers/dfu/dfu_mtd.c b/drivers/dfu/dfu_mtd.c
index ee534a9d10..81357769ff 100644
--- a/drivers/dfu/dfu_mtd.c
+++ b/drivers/dfu/dfu_mtd.c
@@ -251,9 +251,9 @@ static unsigned int dfu_polltimeout_mtd(struct dfu_entity *dfu)
 	return DFU_DEFAULT_POLL_TIMEOUT;
 }
 
-int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, char *s)
+int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, char **argv, int argc)
 {
-	char *st;
+	char *s;
 	struct mtd_info *mtd;
 	int ret, part;
 
@@ -265,25 +265,33 @@ int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, char *s)
 	dfu->dev_type = DFU_DEV_MTD;
 	dfu->data.mtd.info = mtd;
 	dfu->max_buf_size = mtd->erasesize;
+	if (argc < 1)
+		return -EINVAL;
 
-	st = strsep(&s, " \t");
-	s = skip_spaces(s);
-	if (!strcmp(st, "raw")) {
+	if (!strcmp(argv[0], "raw")) {
+		if (argc != 3)
+			return -EINVAL;
 		dfu->layout = DFU_RAW_ADDR;
-		dfu->data.mtd.start = hextoul(s, &s);
-		if (!isspace(*s))
-			return -1;
-		s = skip_spaces(s);
-		dfu->data.mtd.size = hextoul(s, &s);
-	} else if ((!strcmp(st, "part")) || (!strcmp(st, "partubi"))) {
+		dfu->data.mtd.start = hextoul(argv[1], &s);
+		if (*s)
+			return -EINVAL;
+		dfu->data.mtd.size = hextoul(argv[2], &s);
+		if (*s)
+			return -EINVAL;
+	} else if ((!strcmp(argv[0], "part")) || (!strcmp(argv[0], "partubi"))) {
 		char mtd_id[32];
 		struct mtd_device *mtd_dev;
 		u8 part_num;
 		struct part_info *pi;
 
+		if (argc != 2)
+			return -EINVAL;
+
 		dfu->layout = DFU_RAW_ADDR;
 
-		part = dectoul(s, &s);
+		part = dectoul(argv[1], &s);
+		if (*s)
+			return -EINVAL;
 
 		sprintf(mtd_id, "%s,%d", devstr, part - 1);
 		printf("using id '%s'\n", mtd_id);
@@ -298,10 +306,10 @@ int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, char *s)
 
 		dfu->data.mtd.start = pi->offset;
 		dfu->data.mtd.size = pi->size;
-		if (!strcmp(st, "partubi"))
+		if (!strcmp(argv[0], "partubi"))
 			dfu->data.mtd.ubi = 1;
 	} else {
-		printf("%s: Memory layout (%s) not supported!\n", __func__, st);
+		printf("%s: Memory layout (%s) not supported!\n", __func__, argv[0]);
 		return -1;
 	}
 
diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c
index 76761939ab..08e8cf5cdb 100644
--- a/drivers/dfu/dfu_nand.c
+++ b/drivers/dfu/dfu_nand.c
@@ -194,23 +194,25 @@ unsigned int dfu_polltimeout_nand(struct dfu_entity *dfu)
 	return DFU_DEFAULT_POLL_TIMEOUT;
 }
 
-int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char *s)
+int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char **argv, int argc)
 {
-	char *st;
+	char *s;
 	int ret, dev, part;
 
 	dfu->data.nand.ubi = 0;
 	dfu->dev_type = DFU_DEV_NAND;
-	st = strsep(&s, " \t");
-	s = skip_spaces(s);
-	if (!strcmp(st, "raw")) {
+	if (argc != 3)
+		return -EINVAL;
+
+	if (!strcmp(argv[0], "raw")) {
 		dfu->layout = DFU_RAW_ADDR;
-		dfu->data.nand.start = hextoul(s, &s);
-		if (!isspace(*s))
-			return -1;
-		s = skip_spaces(s);
-		dfu->data.nand.size = hextoul(s, &s);
-	} else if ((!strcmp(st, "part")) || (!strcmp(st, "partubi"))) {
+		dfu->data.nand.start = hextoul(argv[1], &s);
+		if (*s)
+			return -EINVAL;
+		dfu->data.nand.size = hextoul(argv[2], &s);
+		if (*s)
+			return -EINVAL;
+	} else if ((!strcmp(argv[0], "part")) || (!strcmp(argv[0], "partubi"))) {
 		char mtd_id[32];
 		struct mtd_device *mtd_dev;
 		u8 part_num;
@@ -218,11 +220,12 @@ int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char *s)
 
 		dfu->layout = DFU_RAW_ADDR;
 
-		dev = dectoul(s, &s);
-		if (!isspace(*s))
-			return -1;
-		s = skip_spaces(s);
-		part = dectoul(s, &s);
+		dev = dectoul(argv[1], &s);
+		if (*s)
+			return -EINVAL;
+		part = dectoul(argv[2], &s);
+		if (*s)
+			return -EINVAL;
 
 		sprintf(mtd_id, "%s%d,%d", "nand", dev, part - 1);
 		debug("using id '%s'\n", mtd_id);
@@ -237,10 +240,10 @@ int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char *s)
 
 		dfu->data.nand.start = pi->offset;
 		dfu->data.nand.size = pi->size;
-		if (!strcmp(st, "partubi"))
+		if (!strcmp(argv[0], "partubi"))
 			dfu->data.nand.ubi = 1;
 	} else {
-		printf("%s: Memory layout (%s) not supported!\n", __func__, st);
+		printf("%s: Memory layout (%s) not supported!\n", __func__, argv[0]);
 		return -1;
 	}
 
diff --git a/drivers/dfu/dfu_ram.c b/drivers/dfu/dfu_ram.c
index 361a3ff8af..9d10303164 100644
--- a/drivers/dfu/dfu_ram.c
+++ b/drivers/dfu/dfu_ram.c
@@ -54,18 +54,13 @@ static int dfu_read_medium_ram(struct dfu_entity *dfu, u64 offset,
 	return dfu_transfer_medium_ram(DFU_OP_READ, dfu, offset, buf, len);
 }
 
-int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr, char *s)
+int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr, char **argv, int argc)
 {
-	const char *argv[3];
-	const char **parg = argv;
-
-	for (; parg < argv + sizeof(argv) / sizeof(*argv); ++parg) {
-		*parg = strsep(&s, " \t");
-		if (*parg == NULL) {
-			pr_err("Invalid number of arguments.\n");
-			return -ENODEV;
-		}
-		s = skip_spaces(s);
+	char *s;
+
+	if (argc != 3) {
+		pr_err("Invalid number of arguments.\n");
+		return -EINVAL;
 	}
 
 	dfu->dev_type = DFU_DEV_RAM;
@@ -75,8 +70,12 @@ int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr, char *s)
 	}
 
 	dfu->layout = DFU_RAM_ADDR;
-	dfu->data.ram.start = hextoul(argv[1], NULL);
-	dfu->data.ram.size = hextoul(argv[2], NULL);
+	dfu->data.ram.start = hextoul(argv[1], &s);
+	if (*s)
+		return -EINVAL;
+	dfu->data.ram.size = hextoul(argv[2], &s);
+	if (*s)
+		return -EINVAL;
 
 	dfu->write_medium = dfu_write_medium_ram;
 	dfu->get_medium_size = dfu_get_medium_size_ram;
diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c
index 993e951bc3..25a9c81850 100644
--- a/drivers/dfu/dfu_sf.c
+++ b/drivers/dfu/dfu_sf.c
@@ -166,9 +166,9 @@ static struct spi_flash *parse_dev(char *devstr)
 	return dev;
 }
 
-int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char *s)
+int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char **argv, int argc)
 {
-	char *st;
+	char *s;
 	char *devstr_bkup = strdup(devstr);
 
 	dfu->data.sf.dev = parse_dev(devstr_bkup);
@@ -179,17 +179,18 @@ int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char *s)
 	dfu->dev_type = DFU_DEV_SF;
 	dfu->max_buf_size = dfu->data.sf.dev->sector_size;
 
-	st = strsep(&s, " \t");
-	s = skip_spaces(s);
-	if (!strcmp(st, "raw")) {
+	if (argc != 3)
+		return -EINVAL;
+	if (!strcmp(argv[0], "raw")) {
 		dfu->layout = DFU_RAW_ADDR;
-		dfu->data.sf.start = hextoul(s, &s);
-		if (!isspace(*s))
-			return -1;
-		s = skip_spaces(s);
-		dfu->data.sf.size = hextoul(s, &s);
+		dfu->data.sf.start = hextoul(argv[1], &s);
+		if (*s)
+			return -EINVAL;
+		dfu->data.sf.size = hextoul(argv[2], &s);
+		if (*s)
+			return -EINVAL;
 	} else if (CONFIG_IS_ENABLED(DFU_SF_PART) &&
-		   (!strcmp(st, "part") || !strcmp(st, "partubi"))) {
+		   (!strcmp(argv[0], "part") || !strcmp(argv[0], "partubi"))) {
 		char mtd_id[32];
 		struct mtd_device *mtd_dev;
 		u8 part_num;
@@ -198,11 +199,12 @@ int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char *s)
 
 		dfu->layout = DFU_RAW_ADDR;
 
-		dev = dectoul(s, &s);
-		if (!isspace(*s))
-			return -1;
-		s = skip_spaces(s);
-		part = dectoul(s, &s);
+		dev = dectoul(argv[1], &s);
+		if (*s)
+			return -EINVAL;
+		part = dectoul(argv[2], &s);
+		if (*s)
+			return -EINVAL;
 
 		sprintf(mtd_id, "%s%d,%d", "nor", dev, part - 1);
 		printf("using id '%s'\n", mtd_id);
@@ -216,10 +218,10 @@ int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char *s)
 		}
 		dfu->data.sf.start = pi->offset;
 		dfu->data.sf.size = pi->size;
-		if (!strcmp(st, "partubi"))
+		if (!strcmp(argv[0], "partubi"))
 			dfu->data.sf.ubi = 1;
 	} else {
-		printf("%s: Memory layout (%s) not supported!\n", __func__, st);
+		printf("%s: Memory layout (%s) not supported!\n", __func__, argv[0]);
 		spi_flash_free(dfu->data.sf.dev);
 		return -1;
 	}
diff --git a/drivers/dfu/dfu_virt.c b/drivers/dfu/dfu_virt.c
index 80c99cb06e..29f7a08f67 100644
--- a/drivers/dfu/dfu_virt.c
+++ b/drivers/dfu/dfu_virt.c
@@ -32,10 +32,13 @@ int __weak dfu_read_medium_virt(struct dfu_entity *dfu, u64 offset,
 	return 0;
 }
 
-int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr, char *s)
+int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr, char **argv, int argc)
 {
 	debug("%s: devstr = %s\n", __func__, devstr);
 
+	if (argc != 0)
+		return -EINVAL;
+
 	dfu->dev_type = DFU_DEV_VIRT;
 	dfu->layout = DFU_RAW_ADDR;
 	dfu->data.virt.dev_num = dectoul(devstr, NULL);
diff --git a/include/dfu.h b/include/dfu.h
index f6868982df..dcb9cd9d79 100644
--- a/include/dfu.h
+++ b/include/dfu.h
@@ -432,11 +432,15 @@ static inline void dfu_set_defer_flush(struct dfu_entity *dfu)
 int dfu_write_from_mem_addr(struct dfu_entity *dfu, void *buf, int size);
 
 /* Device specific */
+/* Each entity has 5 arguments in maximum. */
+#define DFU_MAX_ENTITY_ARGS	5
+
 #if CONFIG_IS_ENABLED(DFU_MMC)
-extern int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s);
+extern int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr,
+			       char **argv, int argc);
 #else
 static inline int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr,
-				      char *s)
+				      char **argv, int argc)
 {
 	puts("MMC support not available!\n");
 	return -1;
@@ -444,10 +448,11 @@ static inline int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr,
 #endif
 
 #if CONFIG_IS_ENABLED(DFU_NAND)
-extern int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char *s);
+extern int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr,
+				char **argv, int argc);
 #else
 static inline int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr,
-				       char *s)
+				       char **argv, int argc)
 {
 	puts("NAND support not available!\n");
 	return -1;
@@ -455,10 +460,11 @@ static inline int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr,
 #endif
 
 #if CONFIG_IS_ENABLED(DFU_RAM)
-extern int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr, char *s);
+extern int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr,
+			       char **argv, int argc);
 #else
 static inline int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr,
-				      char *s)
+				      char **argv, int argc)
 {
 	puts("RAM support not available!\n");
 	return -1;
@@ -466,10 +472,11 @@ static inline int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr,
 #endif
 
 #if CONFIG_IS_ENABLED(DFU_SF)
-extern int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char *s);
+extern int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr,
+			      char **argv, int argc);
 #else
 static inline int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr,
-				     char *s)
+				     char **argv, int argc)
 {
 	puts("SF support not available!\n");
 	return -1;
@@ -477,10 +484,11 @@ static inline int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr,
 #endif
 
 #if CONFIG_IS_ENABLED(DFU_MTD)
-int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, char *s);
+extern int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr,
+			       char **argv, int argc);
 #else
 static inline int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr,
-				      char *s)
+				      char **argv, int argc)
 {
 	puts("MTD support not available!\n");
 	return -1;
@@ -488,7 +496,8 @@ static inline int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr,
 #endif
 
 #ifdef CONFIG_DFU_VIRT
-int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr, char *s);
+int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr,
+			 char **argv, int argc);
 int dfu_write_medium_virt(struct dfu_entity *dfu, u64 offset,
 			  void *buf, long *len);
 int dfu_get_medium_size_virt(struct dfu_entity *dfu, u64 *size);
@@ -496,7 +505,7 @@ int dfu_read_medium_virt(struct dfu_entity *dfu, u64 offset,
 			 void *buf, long *len);
 #else
 static inline int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr,
-				       char *s)
+				       char **argv, int argc)
 {
 	puts("VIRT support not available!\n");
 	return -1;

[PATCH 4/5] doc: usage: DFU: Fix dfu_alt_info document

[Masami Hiramatsu]

Fix some typo and wrong information about dfu_alt_info.
- Add the parameter format, decimal only or hexadecimal.
- Use same parameter name for the same kind of parameters.
  (e.g. dev -> dev_id)

Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
 doc/usage/dfu.rst |   57 +++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 17 deletions(-)

diff --git a/doc/usage/dfu.rst b/doc/usage/dfu.rst
index 11c88072b8..ed47ff561e 100644
--- a/doc/usage/dfu.rst
+++ b/doc/usage/dfu.rst
@@ -113,9 +113,9 @@ mmc
     each element in *dfu_alt_info* being
 
     * <name> raw <offset> <size> [mmcpart <num>]   raw access to mmc device
-    * <name> part <dev> <part_id> [mmcpart <num>]  raw access to partition
-    * <name> fat <dev> <part_id> [mmcpart <num>]   file in FAT partition
-    * <name> ext4 <dev> <part_id> [mmcpart <num>]  file in EXT4 partition
+    * <name> part <dev> <part_id> [offset <byte>]  raw access to partition
+    * <name> fat <dev> <part_id>                   file in FAT partition
+    * <name> ext4 <dev> <part_id>                  file in EXT4 partition
     * <name> skip 0 0                              ignore flashed data
     * <name> script 0 0                            execute commands in shell
 
@@ -169,14 +169,20 @@ nand
 
     each element in *dfu_alt_info* being either of
 
-    * <name> raw <offset> <size>   raw access to mmc device
-    * <name> part <dev> <part_id>  raw acces to partition
-    * <name> partubi <dev> <part_id>  raw acces to ubi partition
+    * <name> raw <offset> <size>        raw access to nand device
+    * <name> part <dev_id> <part_id>     raw access to partition
+    * <name> partubi <dev_id> <part_id>  raw access to ubi partition
 
     with
 
-    partid
-        is the MTD partition index
+    offset
+        is the offset in the nand device (hexadecimal without "0x")
+    size
+        is the size of the access area (hexadecimal without "0x")
+    dev_id
+        is the NAND device index (decimal only)
+    part_id
+        is the NAND partition index (decimal only)
 
 ram
     raw access to ram::
@@ -190,6 +196,13 @@ ram
 
       <name> ram <offset> <size>  raw access to ram
 
+    with
+
+    offset
+        is the offset in the ram device (hexadecimal without "0x")
+    size
+        is the size of the access area (hexadecimal without "0x")
+
 sf
     serial flash : NOR::
 
@@ -198,13 +211,19 @@ sf
     each element in *dfu_alt_info* being either of:
 
     * <name> raw <offset> <size>  raw access to sf device
-    * <name> part <dev> <part_id>  raw acces to partition
-    * <name> partubi <dev> <part_id>  raw acces to ubi partition
+    * <name> part <dev_id> <part_id>  raw acces to partition
+    * <name> partubi <dev_id> <part_id>  raw acces to ubi partition
 
     with
 
-    partid
-        is the MTD partition index
+    offset
+        is the offset in the sf device (hexadecimal without "0x")
+    size
+        is the size of the access area (hexadecimal without "0x")
+    dev_id
+        is the sf device index (the device is "nor<dev_id>") (deximal only)
+    part_id
+        is the MTD partition index (decimal only)
 
 mtd
     all MTD device: NAND, SPI-NOR, SPI-NAND,...::
@@ -219,14 +238,18 @@ mtd
 
     each element in *dfu_alt_info* being either of:
 
-    * <name> raw <offset> <size> forraw access to mtd device
-    * <name> part <dev> <part_id> for raw acces to partition
-    * <name> partubi <dev> <part_id> for raw acces to ubi partition
+    * <name> raw <offset> <size>  for raw access to mtd device
+    * <name> part <part_id>       for raw access to partition
+    * <name> partubi <part_id>    for raw access to ubi partition
 
     with
 
-    partid
-        is the MTD partition index
+    offset
+        is the offset in the mtd device (hexadecimal without "0x")
+    size
+        is the size of the access area (hexadecimal without "0x")
+    part_id
+        is the MTD partition index (decimal only)
 
 virt
     virtual flash back end for DFU

[PATCH 5/5] cmd/dfu: Enable 'dfu list' command without DFU_OVER_USB

[Masami Hiramatsu]

Since dfu is not only used for USB, and some platform only
supports DFU_OVER_TFTP or EFI capsule update, dfu_alt_info
is defined on such platforms too.

For such platform, 'dfu list' command is useful to check
how the current dfu_alt_info setting is parsed.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
 cmd/dfu.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/cmd/dfu.c b/cmd/dfu.c
index 4a288f74c2..208555c888 100644
--- a/cmd/dfu.c
+++ b/cmd/dfu.c
@@ -50,7 +50,6 @@ static int do_dfu(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	if (!strcmp(argv[1], "tftp"))
 		return update_tftp(value, interface, devstring);
 #endif
-#ifdef CONFIG_DFU_OVER_USB
 	ret = dfu_init_env_entities(interface, devstring);
 	if (ret)
 		goto done;
@@ -65,6 +64,7 @@ static int do_dfu(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 		goto done;
 	}
 
+#ifdef CONFIG_DFU_OVER_USB
 	int controller_index = simple_strtoul(usb_controller, NULL, 0);
 	bool retry = false;
 	do {
@@ -79,9 +79,9 @@ static int do_dfu(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 		}
 	} while (retry);
 
+#endif
 done:
 	dfu_free_entities();
-#endif
 	return ret;
 }
 
@@ -100,8 +100,8 @@ U_BOOT_CMD(dfu, CONFIG_SYS_MAXARGS, 1, do_dfu,
 #ifdef CONFIG_DFU_TIMEOUT
 	"    [<timeout>] - specify inactivity timeout in seconds\n"
 #endif
-	"    [list] - list available alt settings\n"
 #endif
+	"    [list] - list available alt settings\n"
 #ifdef CONFIG_DFU_OVER_TFTP
 #ifdef CONFIG_DFU_OVER_USB
 	"dfu "

Re: [PATCH 1/5] DFU: Do not copy the entity name over the buffer size

[Ilias Apalodimas]

On Mon, 6 Dec 2021 at 07:44, Masami Hiramatsu
<masami.hiramatsu@linaro.org> wrote:
>
> Use strlcpy() instead of strcpy() to prevent copying the
> entity name over the name buffer size.
>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> ---
>  drivers/dfu/dfu.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> index af3975925a..66c41b5e76 100644
> --- a/drivers/dfu/dfu.c
> +++ b/drivers/dfu/dfu.c
> @@ -503,7 +503,7 @@ static int dfu_fill_entity(struct dfu_entity *dfu, char *s, int alt,
>
>         debug("%s: %s interface: %s dev: %s\n", __func__, s, interface, devstr);
>         st = strsep(&s, " ");
> -       strcpy(dfu->name, st);
> +       strlcpy(dfu->name, st, DFU_NAME_SIZE);
>
>         dfu->alt = alt;
>         dfu->max_buf_size = 0;
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH 5/5] cmd/dfu: Enable 'dfu list' command without DFU_OVER_USB

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 547 bytes --]

On Mon, Dec 06, 2021 at 02:45:11PM +0900, Masami Hiramatsu wrote:

> Since dfu is not only used for USB, and some platform only
> supports DFU_OVER_TFTP or EFI capsule update, dfu_alt_info
> is defined on such platforms too.
> 
> For such platform, 'dfu list' command is useful to check
> how the current dfu_alt_info setting is parsed.
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>

This patch breaks at least smegw01 and likely warp and iot2050 as well,
please update the series and repost, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 5/5] cmd/dfu: Enable 'dfu list' command without DFU_OVER_USB

[Masami Hiramatsu]

Hi Tom,

Thanks for testing, I confirmed the error.

  CC      cmd/dfu.o
cmd/dfu.c: In function ‘do_dfu’:
cmd/dfu.c:53:30: error: ‘interface’ undeclared (first use in this
function); did you mean ‘usb_interface’?
   53 |  ret = dfu_init_env_entities(interface, devstring);
      |                              ^~~~~~~~~
      |                              usb_interface
cmd/dfu.c:53:30: note: each undeclared identifier is reported only
once for each function it appears in
cmd/dfu.c:53:41: error: ‘devstring’ undeclared (first use in this
function); did you mean ‘fdt_string’?
   53 |  ret = dfu_init_env_entities(interface, devstring);
      |                                         ^~~~~~~~~
      |                                         fdt_string

Let me fix it.

Thank you!

2022年1月29日(土) 1:30 Tom Rini <trini@konsulko.com>:
>
> On Mon, Dec 06, 2021 at 02:45:11PM +0900, Masami Hiramatsu wrote:
>
> > Since dfu is not only used for USB, and some platform only
> > supports DFU_OVER_TFTP or EFI capsule update, dfu_alt_info
> > is defined on such platforms too.
> >
> > For such platform, 'dfu list' command is useful to check
> > how the current dfu_alt_info setting is parsed.
> >
> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>
> This patch breaks at least smegw01 and likely warp and iot2050 as well,
> please update the series and repost, thanks!
>
> --
> Tom



-- 
Masami Hiramatsu