* [PATCH v2 0/5] DFU: Update dfu_alt_info parser etc.
@ 2022-01-31 2:52 Masami Hiramatsu
2022-01-31 2:52 ` [PATCH v2 1/5] DFU: Do not copy the entity name over the buffer size Masami Hiramatsu
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Masami Hiramatsu @ 2022-01-31 2:52 UTC (permalink / raw)
To: Tom Rini, Lukasz Majewski
Cc: u-boot, ilias.apalodimas, sughosh.ganu, jaswinder.singh
Hi,
Here is the 2nd version of improving DFU subsystem series.
This improves dfu_alt_info parser and fixing documents etc.
In this version I fixed a build error of cmd/dfu.c so that
it can be build without DFU_OVER_USB and DFU_OVER_TFTP
(in this case, the platform will use the DFU only for EFI
capsule update.)
When I was debuging my patch for updating dfu_alt_info on the
DeveloperBox platform, I found that dfu_alt_info parser didn'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 nor DFU_OVER_TFTP.
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 | 8 +++----
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(+), 116 deletions(-)
--
Masami Hiramatsu <masami.hiramatsu@linaro.org>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/5] DFU: Do not copy the entity name over the buffer size
2022-01-31 2:52 [PATCH v2 0/5] DFU: Update dfu_alt_info parser etc Masami Hiramatsu
@ 2022-01-31 2:52 ` Masami Hiramatsu
2022-02-11 17:06 ` Tom Rini
2022-01-31 2:52 ` [PATCH v2 2/5] DFU: Accept redundant spaces and tabs in dfu_alt_info Masami Hiramatsu
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Masami Hiramatsu @ 2022-01-31 2:52 UTC (permalink / raw)
To: Tom Rini, Lukasz Majewski
Cc: u-boot, ilias.apalodimas, sughosh.ganu, jaswinder.singh
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;
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/5] DFU: Accept redundant spaces and tabs in dfu_alt_info
2022-01-31 2:52 [PATCH v2 0/5] DFU: Update dfu_alt_info parser etc Masami Hiramatsu
2022-01-31 2:52 ` [PATCH v2 1/5] DFU: Do not copy the entity name over the buffer size Masami Hiramatsu
@ 2022-01-31 2:52 ` Masami Hiramatsu
2022-02-11 17:06 ` Tom Rini
2022-01-31 2:52 ` [PATCH v2 3/5] DFU: Check the number of arguments and argument string strictly Masami Hiramatsu
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Masami Hiramatsu @ 2022-01-31 2:52 UTC (permalink / raw)
To: Tom Rini, Lukasz Majewski
Cc: u-boot, ilias.apalodimas, sughosh.ganu, jaswinder.singh
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 cce9ce0845..27c011f537 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)
{
@@ -285,11 +286,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);
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 3/5] DFU: Check the number of arguments and argument string strictly
2022-01-31 2:52 [PATCH v2 0/5] DFU: Update dfu_alt_info parser etc Masami Hiramatsu
2022-01-31 2:52 ` [PATCH v2 1/5] DFU: Do not copy the entity name over the buffer size Masami Hiramatsu
2022-01-31 2:52 ` [PATCH v2 2/5] DFU: Accept redundant spaces and tabs in dfu_alt_info Masami Hiramatsu
@ 2022-01-31 2:52 ` Masami Hiramatsu
2022-02-11 17:06 ` Tom Rini
2022-08-09 14:11 ` Michal Simek
2022-01-31 2:52 ` [PATCH v2 4/5] doc: usage: DFU: Fix dfu_alt_info document Masami Hiramatsu
2022-01-31 2:52 ` [PATCH v2 5/5] cmd/dfu: Enable 'dfu list' command without DFU_OVER_USB Masami Hiramatsu
4 siblings, 2 replies; 13+ messages in thread
From: Masami Hiramatsu @ 2022-01-31 2:52 UTC (permalink / raw)
To: Tom Rini, Lukasz Majewski
Cc: u-boot, ilias.apalodimas, sughosh.ganu, jaswinder.singh
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 27c011f537..c7075f12ec 100644
--- a/drivers/dfu/dfu_mtd.c
+++ b/drivers/dfu/dfu_mtd.c
@@ -271,9 +271,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;
@@ -285,25 +285,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);
@@ -318,10 +326,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;
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 4/5] doc: usage: DFU: Fix dfu_alt_info document
2022-01-31 2:52 [PATCH v2 0/5] DFU: Update dfu_alt_info parser etc Masami Hiramatsu
` (2 preceding siblings ...)
2022-01-31 2:52 ` [PATCH v2 3/5] DFU: Check the number of arguments and argument string strictly Masami Hiramatsu
@ 2022-01-31 2:52 ` Masami Hiramatsu
2022-02-11 17:06 ` Tom Rini
2022-01-31 2:52 ` [PATCH v2 5/5] cmd/dfu: Enable 'dfu list' command without DFU_OVER_USB Masami Hiramatsu
4 siblings, 1 reply; 13+ messages in thread
From: Masami Hiramatsu @ 2022-01-31 2:52 UTC (permalink / raw)
To: Tom Rini, Lukasz Majewski
Cc: u-boot, ilias.apalodimas, sughosh.ganu, jaswinder.singh
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 5/5] cmd/dfu: Enable 'dfu list' command without DFU_OVER_USB
2022-01-31 2:52 [PATCH v2 0/5] DFU: Update dfu_alt_info parser etc Masami Hiramatsu
` (3 preceding siblings ...)
2022-01-31 2:52 ` [PATCH v2 4/5] doc: usage: DFU: Fix dfu_alt_info document Masami Hiramatsu
@ 2022-01-31 2:52 ` Masami Hiramatsu
2022-02-11 17:06 ` Tom Rini
4 siblings, 1 reply; 13+ messages in thread
From: Masami Hiramatsu @ 2022-01-31 2:52 UTC (permalink / raw)
To: Tom Rini, Lukasz Majewski
Cc: u-boot, ilias.apalodimas, sughosh.ganu, jaswinder.singh
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>
---
Changes in v2:
Fix a compile error if CONFIG_DFU_OVER_USB=n && CONFIG_DFU_OVER_TFTP=n.
---
cmd/dfu.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/cmd/dfu.c b/cmd/dfu.c
index 4a288f74c2..d7bfb535dc 100644
--- a/cmd/dfu.c
+++ b/cmd/dfu.c
@@ -28,7 +28,6 @@ static int do_dfu(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
#ifdef CONFIG_DFU_OVER_USB
char *usb_controller = argv[1];
#endif
-#if defined(CONFIG_DFU_OVER_USB) || defined(CONFIG_DFU_OVER_TFTP)
char *interface = NULL;
char *devstring = NULL;
#if defined(CONFIG_DFU_TIMEOUT) || defined(CONFIG_DFU_OVER_TFTP)
@@ -42,7 +41,6 @@ static int do_dfu(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
#if defined(CONFIG_DFU_TIMEOUT) || defined(CONFIG_DFU_OVER_TFTP)
if (argc == 5 || argc == 3)
value = simple_strtoul(argv[argc - 1], NULL, 0);
-#endif
#endif
int ret = 0;
@@ -50,7 +48,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 +62,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 +77,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 +98,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 "
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/5] DFU: Do not copy the entity name over the buffer size
2022-01-31 2:52 ` [PATCH v2 1/5] DFU: Do not copy the entity name over the buffer size Masami Hiramatsu
@ 2022-02-11 17:06 ` Tom Rini
0 siblings, 0 replies; 13+ messages in thread
From: Tom Rini @ 2022-02-11 17:06 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Lukasz Majewski, u-boot, ilias.apalodimas, sughosh.ganu, jaswinder.singh
[-- Attachment #1: Type: text/plain, Size: 289 bytes --]
On Mon, Jan 31, 2022 at 11:52:20AM +0900, Masami Hiramatsu 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>
Applied to u-boot/master, thanks!
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/5] DFU: Accept redundant spaces and tabs in dfu_alt_info
2022-01-31 2:52 ` [PATCH v2 2/5] DFU: Accept redundant spaces and tabs in dfu_alt_info Masami Hiramatsu
@ 2022-02-11 17:06 ` Tom Rini
0 siblings, 0 replies; 13+ messages in thread
From: Tom Rini @ 2022-02-11 17:06 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Lukasz Majewski, u-boot, ilias.apalodimas, sughosh.ganu, jaswinder.singh
[-- Attachment #1: Type: text/plain, Size: 616 bytes --]
On Mon, Jan 31, 2022 at 11:52:29AM +0900, Masami Hiramatsu wrote:
> 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>
Applied to u-boot/master, thanks!
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/5] DFU: Check the number of arguments and argument string strictly
2022-01-31 2:52 ` [PATCH v2 3/5] DFU: Check the number of arguments and argument string strictly Masami Hiramatsu
@ 2022-02-11 17:06 ` Tom Rini
2022-08-09 14:11 ` Michal Simek
1 sibling, 0 replies; 13+ messages in thread
From: Tom Rini @ 2022-02-11 17:06 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Lukasz Majewski, u-boot, ilias.apalodimas, sughosh.ganu, jaswinder.singh
[-- Attachment #1: Type: text/plain, Size: 661 bytes --]
On Mon, Jan 31, 2022 at 11:52:37AM +0900, Masami Hiramatsu wrote:
> 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>
Applied to u-boot/master, thanks!
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 4/5] doc: usage: DFU: Fix dfu_alt_info document
2022-01-31 2:52 ` [PATCH v2 4/5] doc: usage: DFU: Fix dfu_alt_info document Masami Hiramatsu
@ 2022-02-11 17:06 ` Tom Rini
0 siblings, 0 replies; 13+ messages in thread
From: Tom Rini @ 2022-02-11 17:06 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Lukasz Majewski, u-boot, ilias.apalodimas, sughosh.ganu, jaswinder.singh
[-- Attachment #1: Type: text/plain, Size: 394 bytes --]
On Mon, Jan 31, 2022 at 11:52:46AM +0900, Masami Hiramatsu wrote:
> 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>
Applied to u-boot/master, thanks!
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 5/5] cmd/dfu: Enable 'dfu list' command without DFU_OVER_USB
2022-01-31 2:52 ` [PATCH v2 5/5] cmd/dfu: Enable 'dfu list' command without DFU_OVER_USB Masami Hiramatsu
@ 2022-02-11 17:06 ` Tom Rini
0 siblings, 0 replies; 13+ messages in thread
From: Tom Rini @ 2022-02-11 17:06 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Lukasz Majewski, u-boot, ilias.apalodimas, sughosh.ganu, jaswinder.singh
[-- Attachment #1: Type: text/plain, Size: 463 bytes --]
On Mon, Jan 31, 2022 at 11:52:54AM +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>
Applied to u-boot/master, thanks!
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/5] DFU: Check the number of arguments and argument string strictly
2022-01-31 2:52 ` [PATCH v2 3/5] DFU: Check the number of arguments and argument string strictly Masami Hiramatsu
2022-02-11 17:06 ` Tom Rini
@ 2022-08-09 14:11 ` Michal Simek
2022-08-10 0:19 ` AKASHI Takahiro
1 sibling, 1 reply; 13+ messages in thread
From: Michal Simek @ 2022-08-09 14:11 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Tom Rini, Lukasz Majewski, u-boot, ilias.apalodimas,
sughosh.ganu, jaswinder.singh
Hi Masami,
po 31. 1. 2022 v 3:53 odesílatel Masami Hiramatsu
<masami.hiramatsu@linaro.org> napsal:
>
> 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 27c011f537..c7075f12ec 100644
> --- a/drivers/dfu/dfu_mtd.c
> +++ b/drivers/dfu/dfu_mtd.c
> @@ -271,9 +271,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;
>
> @@ -285,25 +285,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);
> @@ -318,10 +326,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;
>
I tried a capsule update on zynq and I expect zynqmp is going to be the same.
dfu_alt_info is generated like this by u-boot
"mmc 0:1=boot.bin fat 0 1;u-boot.img fat 0 1"
when this patch is applied it expects
"mmc 0=boot.bin fat 0 1;u-boot.img fat 0 1"
I just want to double check it with you because then the following
patch needs to be applied
to fix current behavior.
Thanks,
Michal
diff --git a/board/xilinx/zynq/board.c b/board/xilinx/zynq/board.c
index a7d9476326ec..8fa5e023ed0b 100644
--- a/board/xilinx/zynq/board.c
+++ b/board/xilinx/zynq/board.c
@@ -179,7 +179,7 @@ void set_dfu_alt_info(char *interface, char *devstr)
switch ((zynq_slcr_get_boot_mode()) & ZYNQ_BM_MASK) {
case ZYNQ_BM_SD:
snprintf(buf, DFU_ALT_BUF_LEN,
- "mmc 0:1=boot.bin fat 0 1;"
+ "mmc 0=boot.bin fat 0 1;"
"%s fat 0 1", CONFIG_SPL_FS_LOAD_PAYLOAD_NAME);
break;
case ZYNQ_BM_QSPI:
diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
index 802dfbc0ae7c..daaa581ed4e5 100644
--- a/board/xilinx/zynqmp/zynqmp.c
+++ b/board/xilinx/zynqmp/zynqmp.c
@@ -662,13 +662,13 @@ void set_dfu_alt_info(char *interface, char *devstr)
bootseq = mmc_get_env_dev();
if (!multiboot)
snprintf(buf, DFU_ALT_BUF_LEN,
- "mmc %d:1=boot.bin fat %d 1;"
+ "mmc %d=boot.bin fat %d 1;"
"%s fat %d 1",
bootseq, bootseq,
CONFIG_SPL_FS_LOAD_PAYLOAD_NAME, bootseq);
else
snprintf(buf, DFU_ALT_BUF_LEN,
- "mmc %d:1=boot%04d.bin fat %d 1;"
+ "mmc %d=boot%04d.bin fat %d 1;"
"%s fat %d 1",
bootseq, multiboot, bootseq,
CONFIG_SPL_FS_LOAD_PAYLOAD_NAME, bootseq);
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/5] DFU: Check the number of arguments and argument string strictly
2022-08-09 14:11 ` Michal Simek
@ 2022-08-10 0:19 ` AKASHI Takahiro
0 siblings, 0 replies; 13+ messages in thread
From: AKASHI Takahiro @ 2022-08-10 0:19 UTC (permalink / raw)
To: Michal Simek
Cc: Masami Hiramatsu, Tom Rini, Lukasz Majewski, u-boot,
ilias.apalodimas, sughosh.ganu, jaswinder.singh
On Tue, Aug 09, 2022 at 04:11:40PM +0200, Michal Simek wrote:
> Hi Masami,
>
> po 31. 1. 2022 v 3:53 odesílatel Masami Hiramatsu
> <masami.hiramatsu@linaro.org> napsal:
> >
> > 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 27c011f537..c7075f12ec 100644
> > --- a/drivers/dfu/dfu_mtd.c
> > +++ b/drivers/dfu/dfu_mtd.c
> > @@ -271,9 +271,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;
> >
> > @@ -285,25 +285,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);
> > @@ -318,10 +326,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;
> >
>
> I tried a capsule update on zynq and I expect zynqmp is going to be the same.
> dfu_alt_info is generated like this by u-boot
> "mmc 0:1=boot.bin fat 0 1;u-boot.img fat 0 1"
>
> when this patch is applied it expects
> "mmc 0=boot.bin fat 0 1;u-boot.img fat 0 1"
>
> I just want to double check it with you because then the following
> patch needs to be applied
> to fix current behavior.
As you might have noticed, he has left Linaro.
Regarding the issue you mentioned above, if you continue to use a partition
on your mmc device as firmware storage, there may be a bug in his patch.
-Takahiro Akashi
> Thanks,
> Michal
>
> diff --git a/board/xilinx/zynq/board.c b/board/xilinx/zynq/board.c
> index a7d9476326ec..8fa5e023ed0b 100644
> --- a/board/xilinx/zynq/board.c
> +++ b/board/xilinx/zynq/board.c
> @@ -179,7 +179,7 @@ void set_dfu_alt_info(char *interface, char *devstr)
> switch ((zynq_slcr_get_boot_mode()) & ZYNQ_BM_MASK) {
> case ZYNQ_BM_SD:
> snprintf(buf, DFU_ALT_BUF_LEN,
> - "mmc 0:1=boot.bin fat 0 1;"
> + "mmc 0=boot.bin fat 0 1;"
> "%s fat 0 1", CONFIG_SPL_FS_LOAD_PAYLOAD_NAME);
> break;
> case ZYNQ_BM_QSPI:
> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
> index 802dfbc0ae7c..daaa581ed4e5 100644
> --- a/board/xilinx/zynqmp/zynqmp.c
> +++ b/board/xilinx/zynqmp/zynqmp.c
> @@ -662,13 +662,13 @@ void set_dfu_alt_info(char *interface, char *devstr)
> bootseq = mmc_get_env_dev();
> if (!multiboot)
> snprintf(buf, DFU_ALT_BUF_LEN,
> - "mmc %d:1=boot.bin fat %d 1;"
> + "mmc %d=boot.bin fat %d 1;"
> "%s fat %d 1",
> bootseq, bootseq,
> CONFIG_SPL_FS_LOAD_PAYLOAD_NAME, bootseq);
> else
> snprintf(buf, DFU_ALT_BUF_LEN,
> - "mmc %d:1=boot%04d.bin fat %d 1;"
> + "mmc %d=boot%04d.bin fat %d 1;"
> "%s fat %d 1",
> bootseq, multiboot, bootseq,
> CONFIG_SPL_FS_LOAD_PAYLOAD_NAME, bootseq);
>
>
>
>
> --
> Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
> w: www.monstr.eu p: +42-0-721842854
> Maintainer of Linux kernel - Xilinx Microblaze
> Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
> U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-08-10 0:19 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-31 2:52 [PATCH v2 0/5] DFU: Update dfu_alt_info parser etc Masami Hiramatsu
2022-01-31 2:52 ` [PATCH v2 1/5] DFU: Do not copy the entity name over the buffer size Masami Hiramatsu
2022-02-11 17:06 ` Tom Rini
2022-01-31 2:52 ` [PATCH v2 2/5] DFU: Accept redundant spaces and tabs in dfu_alt_info Masami Hiramatsu
2022-02-11 17:06 ` Tom Rini
2022-01-31 2:52 ` [PATCH v2 3/5] DFU: Check the number of arguments and argument string strictly Masami Hiramatsu
2022-02-11 17:06 ` Tom Rini
2022-08-09 14:11 ` Michal Simek
2022-08-10 0:19 ` AKASHI Takahiro
2022-01-31 2:52 ` [PATCH v2 4/5] doc: usage: DFU: Fix dfu_alt_info document Masami Hiramatsu
2022-02-11 17:06 ` Tom Rini
2022-01-31 2:52 ` [PATCH v2 5/5] cmd/dfu: Enable 'dfu list' command without DFU_OVER_USB Masami Hiramatsu
2022-02-11 17:06 ` Tom Rini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).