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