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;

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

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

diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 18154774f9..516dda6179 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -500,12 +500,29 @@ int dfu_read(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num) static int dfu_fill_entity(struct dfu_entity *dfu, char *s, int alt, char *interface, char *devstr) { + char *argv[DFU_MAX_ENTITY_ARGS]; + int argc; char *st;

debug("%s: %s interface: %s dev: %s\n", __func__, s, interface, devstr); st = strsep(&s, " \t"); strlcpy(dfu->name, st, DFU_NAME_SIZE); - s = skip_spaces(s); + + /* Parse arguments */ + for (argc = 0; s && argc < DFU_MAX_ENTITY_ARGS; argc++) { + s = skip_spaces(s); + if (!*s) + break; + argv[argc] = strsep(&s, " \t"); + } + + if (argc == DFU_MAX_ENTITY_ARGS && s) { + s = skip_spaces(s); + if (*s) { + log_err("Too many arguments for %s\n", dfu->name); + return -EINVAL; + } + }

dfu->alt = alt; dfu->max_buf_size = 0; @@ -513,22 +530,22 @@ static int dfu_fill_entity(struct dfu_entity *dfu, char *s, int alt,

/* Specific for mmc device */ if (strcmp(interface, "mmc") == 0) { - if (dfu_fill_entity_mmc(dfu, devstr, s)) + if (dfu_fill_entity_mmc(dfu, devstr, argv, argc)) return -1; } else if (strcmp(interface, "mtd") == 0) { - if (dfu_fill_entity_mtd(dfu, devstr, s)) + if (dfu_fill_entity_mtd(dfu, devstr, argv, argc)) return -1; } else if (strcmp(interface, "nand") == 0) { - if (dfu_fill_entity_nand(dfu, devstr, s)) + if (dfu_fill_entity_nand(dfu, devstr, argv, argc)) return -1; } else if (strcmp(interface, "ram") == 0) { - if (dfu_fill_entity_ram(dfu, devstr, s)) + if (dfu_fill_entity_ram(dfu, devstr, argv, argc)) return -1; } else if (strcmp(interface, "sf") == 0) { - if (dfu_fill_entity_sf(dfu, devstr, s)) + if (dfu_fill_entity_sf(dfu, devstr, argv, argc)) return -1; } else if (strcmp(interface, "virt") == 0) { - if (dfu_fill_entity_virt(dfu, devstr, s)) + if (dfu_fill_entity_virt(dfu, devstr, argv, argc)) return -1; } else { printf("%s: Device %s not (yet) supported!\n", diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c index d747ede66c..a91da972d4 100644 --- a/drivers/dfu/dfu_mmc.c +++ b/drivers/dfu/dfu_mmc.c @@ -337,35 +337,34 @@ void dfu_free_entity_mmc(struct dfu_entity *dfu) * 4th (optional): * mmcpart <num> (access to HW eMMC partitions) */ -int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s) +int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char **argv, int argc) { const char *entity_type; ssize_t second_arg; size_t third_arg; - struct mmc *mmc; + char *s;

- const char *argv[3]; - const char **parg = argv; - - dfu->data.mmc.dev_num = dectoul(devstr, NULL); - - for (; parg < argv + sizeof(argv) / sizeof(*argv); ++parg) { - *parg = strsep(&s, " \t"); - if (*parg == NULL) { - pr_err("Invalid number of arguments.\n"); - return -ENODEV; - } - s = skip_spaces(s); + if (argc < 3) { + pr_err("The number of parameters are not enough.\n"); + return -EINVAL; }

+ dfu->data.mmc.dev_num = dectoul(devstr, &s); + if (*s) + return -EINVAL; + entity_type = argv[0]; /* * Base 0 means we'll accept (prefixed with 0x or 0) base 16, 8, * with default 10. */ - second_arg = simple_strtol(argv[1], NULL, 0); - third_arg = simple_strtoul(argv[2], NULL, 0); + second_arg = simple_strtol(argv[1], &s, 0); + if (*s) + return -EINVAL; + third_arg = simple_strtoul(argv[2], &s, 0); + if (*s) + return -EINVAL;

mmc = find_mmc_device(dfu->data.mmc.dev_num); if (mmc == NULL) { @@ -390,12 +389,14 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s) * Check for an extra entry at dfu_alt_info env variable * specifying the mmc HW defined partition number */ - if (s) - if (!strcmp(strsep(&s, " \t"), "mmcpart")) { - s = skip_spaces(s); - dfu->data.mmc.hw_partition = - simple_strtoul(s, NULL, 0); + if (argc > 3) { + if (argc != 5 || strcmp(argv[3], "mmcpart")) { + pr_err("DFU mmc raw accept 'mmcpart <partnum>' option.\n"); + return -EINVAL; } + dfu->data.mmc.hw_partition = + simple_strtoul(argv[4], NULL, 0); + }

} else if (!strcmp(entity_type, "part")) { struct disk_partition partinfo; @@ -414,15 +415,18 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s) * Check for an extra entry at dfu_alt_info env variable * specifying the mmc HW defined partition number */ - if (s) - if (!strcmp(strsep(&s, " \t"), "offset")) { - s = skip_spaces(s); - offset = simple_strtoul(s, NULL, 0); + if (argc > 3) { + if (argc != 5 || strcmp(argv[3], "offset")) { + pr_err("DFU mmc raw accept 'mmcpart <partnum>' option.\n"); + return -EINVAL; } + dfu->data.mmc.hw_partition = + simple_strtoul(argv[4], NULL, 0); + }

dfu->layout = DFU_RAW_ADDR; dfu->data.mmc.lba_start = partinfo.start + offset; - dfu->data.mmc.lba_size = partinfo.size-offset; + dfu->data.mmc.lba_size = partinfo.size - offset; dfu->data.mmc.lba_blk_size = partinfo.blksz; } else if (!strcmp(entity_type, "fat")) { dfu->layout = DFU_FS_FAT; diff --git a/drivers/dfu/dfu_mtd.c b/drivers/dfu/dfu_mtd.c index ee534a9d10..81357769ff 100644 --- a/drivers/dfu/dfu_mtd.c +++ b/drivers/dfu/dfu_mtd.c @@ -251,9 +251,9 @@ static unsigned int dfu_polltimeout_mtd(struct dfu_entity *dfu) return DFU_DEFAULT_POLL_TIMEOUT; }

-int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, char *s) +int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, char **argv, int argc) { - char *st; + char *s; struct mtd_info *mtd; int ret, part;

@@ -265,25 +265,33 @@ int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, char *s) dfu->dev_type = DFU_DEV_MTD; dfu->data.mtd.info = mtd; dfu->max_buf_size = mtd->erasesize; + if (argc < 1) + return -EINVAL;

- st = strsep(&s, " \t"); - s = skip_spaces(s); - if (!strcmp(st, "raw")) { + if (!strcmp(argv[0], "raw")) { + if (argc != 3) + return -EINVAL; dfu->layout = DFU_RAW_ADDR; - dfu->data.mtd.start = hextoul(s, &s); - if (!isspace(*s)) - return -1; - s = skip_spaces(s); - dfu->data.mtd.size = hextoul(s, &s); - } else if ((!strcmp(st, "part")) || (!strcmp(st, "partubi"))) { + dfu->data.mtd.start = hextoul(argv[1], &s); + if (*s) + return -EINVAL; + dfu->data.mtd.size = hextoul(argv[2], &s); + if (*s) + return -EINVAL; + } else if ((!strcmp(argv[0], "part")) || (!strcmp(argv[0], "partubi"))) { char mtd_id[32]; struct mtd_device *mtd_dev; u8 part_num; struct part_info *pi;

+ if (argc != 2) + return -EINVAL; + dfu->layout = DFU_RAW_ADDR;

- part = dectoul(s, &s); + part = dectoul(argv[1], &s); + if (*s) + return -EINVAL;

sprintf(mtd_id, "%s,%d", devstr, part - 1); printf("using id '%s'\n", mtd_id); @@ -298,10 +306,10 @@ int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, char *s)

dfu->data.mtd.start = pi->offset; dfu->data.mtd.size = pi->size; - if (!strcmp(st, "partubi")) + if (!strcmp(argv[0], "partubi")) dfu->data.mtd.ubi = 1; } else { - printf("%s: Memory layout (%s) not supported!\n", __func__, st); + printf("%s: Memory layout (%s) not supported!\n", __func__, argv[0]); return -1; }

diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c index 76761939ab..08e8cf5cdb 100644 --- a/drivers/dfu/dfu_nand.c +++ b/drivers/dfu/dfu_nand.c @@ -194,23 +194,25 @@ unsigned int dfu_polltimeout_nand(struct dfu_entity *dfu) return DFU_DEFAULT_POLL_TIMEOUT; }

-int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char *s) +int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char **argv, int argc) { - char *st; + char *s; int ret, dev, part;

dfu->data.nand.ubi = 0; dfu->dev_type = DFU_DEV_NAND; - st = strsep(&s, " \t"); - s = skip_spaces(s); - if (!strcmp(st, "raw")) { + if (argc != 3) + return -EINVAL; + + if (!strcmp(argv[0], "raw")) { dfu->layout = DFU_RAW_ADDR; - dfu->data.nand.start = hextoul(s, &s); - if (!isspace(*s)) - return -1; - s = skip_spaces(s); - dfu->data.nand.size = hextoul(s, &s); - } else if ((!strcmp(st, "part")) || (!strcmp(st, "partubi"))) { + dfu->data.nand.start = hextoul(argv[1], &s); + if (*s) + return -EINVAL; + dfu->data.nand.size = hextoul(argv[2], &s); + if (*s) + return -EINVAL; + } else if ((!strcmp(argv[0], "part")) || (!strcmp(argv[0], "partubi"))) { char mtd_id[32]; struct mtd_device *mtd_dev; u8 part_num; @@ -218,11 +220,12 @@ int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char *s)

dfu->layout = DFU_RAW_ADDR;

- dev = dectoul(s, &s); - if (!isspace(*s)) - return -1; - s = skip_spaces(s); - part = dectoul(s, &s); + dev = dectoul(argv[1], &s); + if (*s) + return -EINVAL; + part = dectoul(argv[2], &s); + if (*s) + return -EINVAL;

sprintf(mtd_id, "%s%d,%d", "nand", dev, part - 1); debug("using id '%s'\n", mtd_id); @@ -237,10 +240,10 @@ int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char *s)

dfu->data.nand.start = pi->offset; dfu->data.nand.size = pi->size; - if (!strcmp(st, "partubi")) + if (!strcmp(argv[0], "partubi")) dfu->data.nand.ubi = 1; } else { - printf("%s: Memory layout (%s) not supported!\n", __func__, st); + printf("%s: Memory layout (%s) not supported!\n", __func__, argv[0]); return -1; }

diff --git a/drivers/dfu/dfu_ram.c b/drivers/dfu/dfu_ram.c index 361a3ff8af..9d10303164 100644 --- a/drivers/dfu/dfu_ram.c +++ b/drivers/dfu/dfu_ram.c @@ -54,18 +54,13 @@ static int dfu_read_medium_ram(struct dfu_entity *dfu, u64 offset, return dfu_transfer_medium_ram(DFU_OP_READ, dfu, offset, buf, len); }

-int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr, char *s) +int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr, char **argv, int argc) { - const char *argv[3]; - const char **parg = argv; - - for (; parg < argv + sizeof(argv) / sizeof(*argv); ++parg) { - *parg = strsep(&s, " \t"); - if (*parg == NULL) { - pr_err("Invalid number of arguments.\n"); - return -ENODEV; - } - s = skip_spaces(s); + char *s; + + if (argc != 3) { + pr_err("Invalid number of arguments.\n"); + return -EINVAL; }

dfu->dev_type = DFU_DEV_RAM; @@ -75,8 +70,12 @@ int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr, char *s) }

dfu->layout = DFU_RAM_ADDR; - dfu->data.ram.start = hextoul(argv[1], NULL); - dfu->data.ram.size = hextoul(argv[2], NULL); + dfu->data.ram.start = hextoul(argv[1], &s); + if (*s) + return -EINVAL; + dfu->data.ram.size = hextoul(argv[2], &s); + if (*s) + return -EINVAL;

dfu->write_medium = dfu_write_medium_ram; dfu->get_medium_size = dfu_get_medium_size_ram; diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c index 993e951bc3..25a9c81850 100644 --- a/drivers/dfu/dfu_sf.c +++ b/drivers/dfu/dfu_sf.c @@ -166,9 +166,9 @@ static struct spi_flash *parse_dev(char *devstr) return dev; }

-int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char *s) +int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char **argv, int argc) { - char *st; + char *s; char *devstr_bkup = strdup(devstr);

dfu->data.sf.dev = parse_dev(devstr_bkup); @@ -179,17 +179,18 @@ int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char *s) dfu->dev_type = DFU_DEV_SF; dfu->max_buf_size = dfu->data.sf.dev->sector_size;

- st = strsep(&s, " \t"); - s = skip_spaces(s); - if (!strcmp(st, "raw")) { + if (argc != 3) + return -EINVAL; + if (!strcmp(argv[0], "raw")) { dfu->layout = DFU_RAW_ADDR; - dfu->data.sf.start = hextoul(s, &s); - if (!isspace(*s)) - return -1; - s = skip_spaces(s); - dfu->data.sf.size = hextoul(s, &s); + dfu->data.sf.start = hextoul(argv[1], &s); + if (*s) + return -EINVAL; + dfu->data.sf.size = hextoul(argv[2], &s); + if (*s) + return -EINVAL; } else if (CONFIG_IS_ENABLED(DFU_SF_PART) && - (!strcmp(st, "part") || !strcmp(st, "partubi"))) { + (!strcmp(argv[0], "part") || !strcmp(argv[0], "partubi"))) { char mtd_id[32]; struct mtd_device *mtd_dev; u8 part_num; @@ -198,11 +199,12 @@ int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char *s)

dfu->layout = DFU_RAW_ADDR;

- dev = dectoul(s, &s); - if (!isspace(*s)) - return -1; - s = skip_spaces(s); - part = dectoul(s, &s); + dev = dectoul(argv[1], &s); + if (*s) + return -EINVAL; + part = dectoul(argv[2], &s); + if (*s) + return -EINVAL;

sprintf(mtd_id, "%s%d,%d", "nor", dev, part - 1); printf("using id '%s'\n", mtd_id); @@ -216,10 +218,10 @@ int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char *s) } dfu->data.sf.start = pi->offset; dfu->data.sf.size = pi->size; - if (!strcmp(st, "partubi")) + if (!strcmp(argv[0], "partubi")) dfu->data.sf.ubi = 1; } else { - printf("%s: Memory layout (%s) not supported!\n", __func__, st); + printf("%s: Memory layout (%s) not supported!\n", __func__, argv[0]); spi_flash_free(dfu->data.sf.dev); return -1; } diff --git a/drivers/dfu/dfu_virt.c b/drivers/dfu/dfu_virt.c index 80c99cb06e..29f7a08f67 100644 --- a/drivers/dfu/dfu_virt.c +++ b/drivers/dfu/dfu_virt.c @@ -32,10 +32,13 @@ int __weak dfu_read_medium_virt(struct dfu_entity *dfu, u64 offset, return 0; }

-int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr, char *s) +int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr, char **argv, int argc) { debug("%s: devstr = %s\n", __func__, devstr);

+ if (argc != 0) + return -EINVAL; + dfu->dev_type = DFU_DEV_VIRT; dfu->layout = DFU_RAW_ADDR; dfu->data.virt.dev_num = dectoul(devstr, NULL); diff --git a/include/dfu.h b/include/dfu.h index f6868982df..dcb9cd9d79 100644 --- a/include/dfu.h +++ b/include/dfu.h @@ -432,11 +432,15 @@ static inline void dfu_set_defer_flush(struct dfu_entity *dfu) int dfu_write_from_mem_addr(struct dfu_entity *dfu, void *buf, int size);

/* Device specific */ +/* Each entity has 5 arguments in maximum. */ +#define DFU_MAX_ENTITY_ARGS 5 + #if CONFIG_IS_ENABLED(DFU_MMC) -extern int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s); +extern int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, + char **argv, int argc); #else static inline int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, - char *s) + char **argv, int argc) { puts("MMC support not available!\n"); return -1; @@ -444,10 +448,11 @@ static inline int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, #endif

#if CONFIG_IS_ENABLED(DFU_NAND) -extern int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char *s); +extern int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, + char **argv, int argc); #else static inline int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, - char *s) + char **argv, int argc) { puts("NAND support not available!\n"); return -1; @@ -455,10 +460,11 @@ static inline int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, #endif

#if CONFIG_IS_ENABLED(DFU_RAM) -extern int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr, char *s); +extern int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr, + char **argv, int argc); #else static inline int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr, - char *s) + char **argv, int argc) { puts("RAM support not available!\n"); return -1; @@ -466,10 +472,11 @@ static inline int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr, #endif

#if CONFIG_IS_ENABLED(DFU_SF) -extern int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char *s); +extern int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, + char **argv, int argc); #else static inline int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, - char *s) + char **argv, int argc) { puts("SF support not available!\n"); return -1; @@ -477,10 +484,11 @@ static inline int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, #endif

#if CONFIG_IS_ENABLED(DFU_MTD) -int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, char *s); +extern int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, + char **argv, int argc); #else static inline int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, - char *s) + char **argv, int argc) { puts("MTD support not available!\n"); return -1; @@ -488,7 +496,8 @@ static inline int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, #endif

#ifdef CONFIG_DFU_VIRT -int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr, char *s); +int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr, + char **argv, int argc); int dfu_write_medium_virt(struct dfu_entity *dfu, u64 offset, void *buf, long *len); int dfu_get_medium_size_virt(struct dfu_entity *dfu, u64 *size); @@ -496,7 +505,7 @@ int dfu_read_medium_virt(struct dfu_entity *dfu, u64 offset, void *buf, long *len); #else static inline int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr, - char *s) + char **argv, int argc) { puts("VIRT support not available!\n"); return -1;

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

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

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

diff --git a/cmd/dfu.c b/cmd/dfu.c index 4a288f74c2..208555c888 100644 --- a/cmd/dfu.c +++ b/cmd/dfu.c @@ -50,7 +50,6 @@ static int do_dfu(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) if (!strcmp(argv[1], "tftp")) return update_tftp(value, interface, devstring); #endif -#ifdef CONFIG_DFU_OVER_USB ret = dfu_init_env_entities(interface, devstring); if (ret) goto done; @@ -65,6 +64,7 @@ static int do_dfu(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) goto done; }

+#ifdef CONFIG_DFU_OVER_USB int controller_index = simple_strtoul(usb_controller, NULL, 0); bool retry = false; do { @@ -79,9 +79,9 @@ static int do_dfu(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) } } while (retry);

+#endif done: dfu_free_entities(); -#endif return ret; }

@@ -100,8 +100,8 @@ U_BOOT_CMD(dfu, CONFIG_SYS_MAXARGS, 1, do_dfu, #ifdef CONFIG_DFU_TIMEOUT " [<timeout>] - specify inactivity timeout in seconds\n" #endif - " [list] - list available alt settings\n" #endif + " [list] - list available alt settings\n" #ifdef CONFIG_DFU_OVER_TFTP #ifdef CONFIG_DFU_OVER_USB "dfu "

It is better if a function which returns an error then release all allocated memory resources. This simplifies the mind model and less chance to forgot to free memory and double free.

Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org --- lib/fwu_updates/fwu_mdata_gpt_blk.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/lib/fwu_updates/fwu_mdata_gpt_blk.c b/lib/fwu_updates/fwu_mdata_gpt_blk.c index 0259da37c3..7f8b8b68fc 100644 --- a/lib/fwu_updates/fwu_mdata_gpt_blk.c +++ b/lib/fwu_updates/fwu_mdata_gpt_blk.c @@ -215,7 +215,8 @@ static int gpt_get_mdata(struct fwu_mdata **mdata) ret = gpt_read_mdata(desc, *mdata, primary_mpart); if (ret < 0) { log_err("Failed to read the FWU metadata from the device\n"); - return -EIO; + ret = -EIO; + goto out; }

ret = fwu_verify_mdata(*mdata, 1); @@ -230,7 +231,8 @@ static int gpt_get_mdata(struct fwu_mdata **mdata) ret = gpt_read_mdata(desc, *mdata, secondary_mpart); if (ret < 0) { log_err("Failed to read the FWU metadata from the device\n"); - return -EIO; + ret = -EIO; + goto out; }

ret = fwu_verify_mdata(*mdata, 0); @@ -238,7 +240,10 @@ static int gpt_get_mdata(struct fwu_mdata **mdata) return 0;

/* Both the FWU metadata copies are corrupted. */ - return -1; + ret = -1; +out: + free(*mdata); + return ret; }

static int gpt_check_mdata_validity(void)

This changes SPI NOR flash partition layout for TBBR and also make the U-Boot as position independent executable again because BL33 is loaded on the memory.

With enabling TBBR, TF-A BL2 loads all BL3x images from FIP image, and the U-Boot image is added to the FIP image as BL33, and loaded to memory when boot instead of XIP on SPI NOR flash. To avoid mixing up with the legacy images, this new FIP image is stored on unused area (0x600000-) and the U-Boot env vars are also stored at 0x580000 so that it will not break existing EDK2 area.

Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org --- .../dts/synquacer-sc2a11-developerbox-u-boot.dtsi | 26 ++++++++++++++------ configs/synquacer_developerbox_defconfig | 5 ++-- include/configs/synquacer.h | 4 +-- 3 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi b/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi index 7a56116d6f..095727e03c 100644 --- a/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi +++ b/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi @@ -56,7 +56,7 @@ };

partition@180000 { - label = "FIP-TFA"; + label = "LegacyFIP"; reg = <0x180000 0x78000>; };

@@ -66,18 +66,28 @@ };

partition@200000 { - label = "U-Boot"; - reg = <0x200000 0x100000>; + label = "EDK2"; + reg = <0x200000 0x200000>; };

- partition@300000 { - label = "UBoot-Env"; - reg = <0x300000 0x100000>; + partition@400000 { + label = "EDK2-Env"; + reg = <0x400000 0x100000>; };

partition@500000 { - label = "Ex-OPTEE"; - reg = <0x500000 0x200000>; + label = "Metadata"; + reg = <0x500000 0x80000>; + }; + + partition@580000 { + label = "UBoot-Env"; + reg = <0x580000 0x80000>; + }; + + partition@600000 { + label = "FIP"; + reg = <0x600000 0x400000>; }; }; }; diff --git a/configs/synquacer_developerbox_defconfig b/configs/synquacer_developerbox_defconfig index da57dc288f..484f6b7336 100644 --- a/configs/synquacer_developerbox_defconfig +++ b/configs/synquacer_developerbox_defconfig @@ -1,9 +1,10 @@ CONFIG_ARM=y CONFIG_ARCH_SYNQUACER=y -CONFIG_SYS_TEXT_BASE=0x08200000 +CONFIG_POSITION_INDEPENDENT=y +CONFIG_SYS_TEXT_BASE=0 CONFIG_SYS_MALLOC_LEN=0x1000000 CONFIG_ENV_SIZE=0x30000 -CONFIG_ENV_OFFSET=0x300000 +CONFIG_ENV_OFFSET=0x580000 CONFIG_ENV_SECT_SIZE=0x10000 CONFIG_DM_GPIO=y CONFIG_DEFAULT_DEVICE_TREE="synquacer-sc2a11-developerbox" diff --git a/include/configs/synquacer.h b/include/configs/synquacer.h index 0409589152..6d67bd2af5 100644 --- a/include/configs/synquacer.h +++ b/include/configs/synquacer.h @@ -48,9 +48,7 @@ /* Since U-Boot 64bit PCIe support is limited, disable 64bit MMIO support */

#define DEFAULT_DFU_ALT_INFO "dfu_alt_info=" \ - "mtd nor1=u-boot.bin raw 200000 100000;" \ - "fip.bin raw 180000 78000;" \ - "optee.bin raw 500000 100000\0" + "mtd nor1=fip.bin raw 600000 400000\0"

/* Distro boot settings */ #ifndef CONFIG_SPL_BUILD

Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org --- include/fwu.h | 13 ++ lib/fwu_updates/Kconfig | 34 ++++++ lib/fwu_updates/Makefile | 5 - lib/fwu_updates/fwu_mdata_sf.c | 241 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 288 insertions(+), 5 deletions(-) create mode 100644 lib/fwu_updates/fwu_mdata_sf.c

diff --git a/include/fwu.h b/include/fwu.h index 7af94988b7..a013170321 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -64,9 +64,18 @@ int fwu_accept_image(efi_guid_t *img_type_id, u32 bank); int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank);

int fwu_plat_get_update_index(u32 *update_idx); -int fwu_plat_get_blk_desc(struct blk_desc **desc); -int fwu_plat_get_alt_num(void *identifier); int fwu_plat_fill_partition_guids(efi_guid_t **part_guid_arr); void fwu_plat_get_bootidx(void *boot_idx);

+#ifdef CONFIG_FWU_MULTI_BANK_UPDATE_GPT_BLK +int fwu_plat_get_blk_desc(struct blk_desc **desc); +int fwu_plat_get_alt_num(void *identifier); +#endif + +#ifdef CONFIG_FWU_MULTI_BANK_UPDATE_SF +struct fwu_mdata_ops *fwu_sf_get_fwu_mdata_ops(void); +int fwu_plat_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank, + int *alt_no); +#endif + #endif /* _FWU_H_ */ diff --git a/lib/fwu_updates/Kconfig b/lib/fwu_updates/Kconfig index 0940a90747..8a369b9872 100644 --- a/lib/fwu_updates/Kconfig +++ b/lib/fwu_updates/Kconfig @@ -38,3 +38,37 @@ config FWU_REBOOT_AFTER_UPDATE Reboot the machine soon after installing a new firmware and start trial boot. You can disable this option for debugging or FWU development, but recommended to enable it. + +config FWU_MULTI_BANK_UPDATE_GPT_BLK + bool "Enable FWU Multi Bank Update for GPT on blockdev" + depends on FWU_MULTI_BANK_UPDATE + select EFI_PARTITION + help + Enable FWU Multi Bank Update for GPT on a block device. This + driver depends on GPT and the block device. FWU meatadata and + firmware will be stored on a block device with GPT. + +config FWU_MULTI_BANK_UPDATE_SF + bool "Enable FWU Multi Bank Update for SPI Flash" + depends on FWU_MULTI_BANK_UPDATE + help + Enable FWU Multi Bank Update for SPI flash driver. This + driver does not depend on GPT. Instead, the platform must + provide some APIs and define the offset of the primary and + the secondary metadata. + +config FWU_SF_PRIMARY_MDATA_OFFSET + hex "The offset of the primary metadata" + depends on FWU_MULTI_BANK_UPDATE_SF + help + The offset of the primary metadata on SPI Flash in + hexadecimal. + +config FWU_SF_SECONDARY_MDATA_OFFSET + hex "The offset of the secondary metadata" + depends on FWU_MULTI_BANK_UPDATE_SF + help + The offset of the secondary metadata on SPI Flash in + hexadecimal. + This must NOT be the same offset of the primary + metadata. diff --git a/lib/fwu_updates/Makefile b/lib/fwu_updates/Makefile index 73099a30cb..80669344f2 100644 --- a/lib/fwu_updates/Makefile +++ b/lib/fwu_updates/Makefile @@ -6,6 +6,5 @@ obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu_mdata.o obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu.o

-ifdef CONFIG_EFI_PARTITION -obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu_mdata_gpt_blk.o -endif +obj-$(CONFIG_FWU_MULTI_BANK_UPDATE_GPT_BLK) += fwu_mdata_gpt_blk.o +obj-$(CONFIG_FWU_MULTI_BANK_UPDATE_SF) += fwu_mdata_sf.o diff --git a/lib/fwu_updates/fwu_mdata_sf.c b/lib/fwu_updates/fwu_mdata_sf.c new file mode 100644 index 0000000000..9e3cae7b68 --- /dev/null +++ b/lib/fwu_updates/fwu_mdata_sf.c @@ -0,0 +1,241 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (c) 2021, Linaro Limited + */ + +#include <efi_loader.h> +#include <fwu.h> +#include <fwu_mdata.h> +#include <malloc.h> +#include <memalign.h> +#include <spi.h> +#include <spi_flash.h> +#include <flash.h> + +#include <linux/errno.h> +#include <linux/types.h> +#include <u-boot/crc.h> + +/* + * For the FWU SPI flash driver, the platform must define below functions + * according to its dfu_alt_info. + */ +extern int fwu_plat_get_image_alt_num(efi_guid_t image_type_id, + u32 update_bank, int *alt_no); + +static struct spi_flash *fwu_spi_flash; + +static int __fwu_sf_get_flash(void) +{ + if (IS_ENABLED(CONFIG_DM_SPI_FLASH)) { + struct udevice *new; + int ret; + + ret = spi_flash_probe_bus_cs(CONFIG_SF_DEFAULT_BUS, CONFIG_SF_DEFAULT_CS, + CONFIG_SF_DEFAULT_SPEED, CONFIG_SF_DEFAULT_MODE, + &new); + if (ret) + return ret; + + fwu_spi_flash = dev_get_uclass_priv(new); + } else { + fwu_spi_flash = spi_flash_probe(CONFIG_SF_DEFAULT_BUS, CONFIG_SF_DEFAULT_CS, + CONFIG_SF_DEFAULT_SPEED, CONFIG_SF_DEFAULT_MODE); + if (!fwu_spi_flash) + return -EIO; + } + return 0; +} + +static int fwu_sf_get_flash(struct spi_flash **flash) +{ + int ret = 0; + + if (!fwu_spi_flash) + ret = __fwu_sf_get_flash(); + + *flash = fwu_spi_flash; + + return ret; +} + +static int sf_load_data(u32 offs, u32 size, void **data) +{ + struct spi_flash *flash; + int ret; + + ret = fwu_sf_get_flash(&flash); + if (ret < 0) + return ret; + + *data = memalign(ARCH_DMA_MINALIGN, size); + if (!*data) + return -ENOMEM; + + ret = spi_flash_read(flash, offs, size, *data); + if (ret < 0) { + free(*data); + *data = NULL; + } + + return ret; +} + +static int sf_save_data(u32 offs, u32 size, void *data) +{ + struct spi_flash *flash; + u32 sect_size, nsect; + void *buf; + int ret; + + ret = fwu_sf_get_flash(&flash); + if (ret < 0) + return ret; + + sect_size = flash->mtd.erasesize; + nsect = DIV_ROUND_UP(size, sect_size); + ret = spi_flash_erase(flash, offs, nsect * sect_size); + if (ret < 0) + return ret; + + buf = memalign(ARCH_DMA_MINALIGN, size); + if (!buf) + return -ENOMEM; + memcpy(buf, data, size); + + ret = spi_flash_write(flash, offs, size, buf); + + free(buf); + + return ret; +} + +static int fwu_sf_load_mdata(struct fwu_mdata **mdata, u32 offs) +{ + int ret; + + ret = sf_load_data(offs, sizeof(struct fwu_mdata), (void **)mdata); + + if (ret >= 0) { + ret = fwu_verify_mdata(*mdata, + offs == CONFIG_FWU_SF_PRIMARY_MDATA_OFFSET); + if (ret < 0) { + free(*mdata); + *mdata = NULL; + } + } + + return ret; +} + +static int fwu_sf_load_primary_mdata(struct fwu_mdata **mdata) +{ + return fwu_sf_load_mdata(mdata, CONFIG_FWU_SF_PRIMARY_MDATA_OFFSET); +} + +static int fwu_sf_load_secondary_mdata(struct fwu_mdata **mdata) +{ + return fwu_sf_load_mdata(mdata, CONFIG_FWU_SF_SECONDARY_MDATA_OFFSET); +} + +static int fwu_sf_save_primary_mdata(struct fwu_mdata *mdata) +{ + return sf_save_data(CONFIG_FWU_SF_PRIMARY_MDATA_OFFSET, + sizeof(struct fwu_mdata), mdata); +} + +static int fwu_sf_save_secondary_mdata(struct fwu_mdata *mdata) +{ + return sf_save_data(CONFIG_FWU_SF_SECONDARY_MDATA_OFFSET, + sizeof(struct fwu_mdata), mdata); +} + +static int fwu_sf_get_valid_mdata(struct fwu_mdata **mdata) +{ + if (fwu_sf_load_primary_mdata(mdata) == 0) + return 0; + + log_err("Failed to load/verify primary mdata. Try secondary.\n"); + + if (fwu_sf_load_secondary_mdata(mdata) == 0) + return 0; + + log_err("Failed to load/verify secondary mdata.\n"); + + return -1; +} + +static int fwu_sf_update_mdata(struct fwu_mdata *mdata) +{ + int ret; + + /* Update mdata crc32 field */ + mdata->crc32 = crc32(0, (void *)&mdata->version, + sizeof(*mdata) - sizeof(u32)); + + /* First write the primary mdata */ + ret = fwu_sf_save_primary_mdata(mdata); + if (ret < 0) { + log_err("Failed to update the primary mdata.\n"); + return ret; + } + + /* And now the replica */ + ret = fwu_sf_save_secondary_mdata(mdata); + if (ret < 0) { + log_err("Failed to update the secondary mdata.\n"); + return ret; + } + + return 0; +} + +static int fwu_sf_mdata_check(void) +{ + struct fwu_mdata *primary = NULL, *secondary = NULL; + int ret; + + ret = fwu_sf_load_primary_mdata(&primary); + if (ret < 0) + log_err("Failed to read the primary mdata: %d\n", ret); + + ret = fwu_sf_load_secondary_mdata(&secondary); + if (ret < 0) + log_err("Failed to read the secondary mdata: %d\n", ret); + + if (primary && secondary) { + if (memcmp(primary, secondary, sizeof(struct fwu_mdata))) { + log_err("The primary and the secondary mdata are different\n"); + ret = -1; + } + } else if (primary) { + ret = fwu_sf_save_secondary_mdata(primary); + if (ret < 0) + log_err("Restoring secondary mdata partition failed\n"); + } else if (secondary) { + ret = fwu_sf_save_primary_mdata(secondary); + if (ret < 0) + log_err("Restoring primary mdata partition failed\n"); + } + + free(primary); + free(secondary); + return ret; +} + +static int fwu_sf_get_mdata(struct fwu_mdata **mdata) +{ + return fwu_sf_get_valid_mdata(mdata); +} + +static struct fwu_mdata_ops fwu_sf_mdata_ops = { + .get_image_alt_num = fwu_plat_get_image_alt_num, + .mdata_check = fwu_sf_mdata_check, + .get_mdata = fwu_sf_get_mdata, + .update_mdata = fwu_sf_update_mdata, +}; + +struct fwu_mdata_ops *fwu_sf_get_fwu_mdata_ops(void) +{ + return &fwu_sf_mdata_ops; +}

Since the FWU metadata is not initialized at the installation, if it is broken, it should be initialized. Usually, the FWU metadata is not covered by capsule update, so it is safe to initialize the metadata portion if it seems broken.

But for the production device, usually firmware will be installed with initialized metadata, and the broken metadata means the device can be compromized. In that case, build U-Boot without this option.

Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org --- board/socionext/developerbox/Kconfig | 12 ++++++ board/socionext/developerbox/fwu_plat.c | 59 +++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+)

diff --git a/board/socionext/developerbox/Kconfig b/board/socionext/developerbox/Kconfig index 4e2c341aad..1b36e10f3b 100644 --- a/board/socionext/developerbox/Kconfig +++ b/board/socionext/developerbox/Kconfig @@ -51,4 +51,16 @@ config FWU_SF_PRIMARY_MDATA_OFFSET config FWU_SF_SECONDARY_MDATA_OFFSET default 0x520000

+config FWU_INIT_BROKEN_METADATA + bool "Initialize FWU metadata if broken" + select BOARD_LATE_INIT + default n + help + Initialize FWU metadata if the metadata is broken. + This option is only for the development environment, since if the + metadata is broken, it means someone may compromize it. In that case + the production device must be bricked. + But for the development environment, or initial installation of the + FWU multi-bank update firmware, this will be useful. + endif diff --git a/board/socionext/developerbox/fwu_plat.c b/board/socionext/developerbox/fwu_plat.c index dbb814f1fd..2982e47a16 100644 --- a/board/socionext/developerbox/fwu_plat.c +++ b/board/socionext/developerbox/fwu_plat.c @@ -156,3 +156,62 @@ struct fwu_mdata_ops *get_plat_fwu_mdata_ops(void) return fwu_sf_get_fwu_mdata_ops(); }

+#ifdef CONFIG_FWU_INIT_BROKEN_METADATA + +static void devbox_init_fwu_mdata(void) +{ + const efi_guid_t null_guid = NULL_GUID; + struct fwu_image_bank_info *bank; + struct fwu_mdata *metadata; + int i, j, ret; + + metadata = memalign(ARCH_DMA_MINALIGN, sizeof(*metadata)); + if (!metadata) { + log_err("Failed to allocate initial metadata.\n"); + return; + } + + metadata->version = 1; + metadata->active_index = 0; + metadata->previous_active_index = 0; + + /* + * Since the DeveloperBox doesn't use GPT, both of + * fwu_image_entry::location_uuid and + * fwu_img_bank_info::image_uuid are null GUID. + */ + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { + guidcpy(&metadata->img_entry[i].image_type_uuid, + &devbox_fip_image_type_guid); + guidcpy(&metadata->img_entry[i].location_uuid, + &null_guid); + bank = metadata->img_entry[i].img_bank_info; + + for (j = 0; j < CONFIG_FWU_NUM_BANKS; j++) { + guidcpy(&bank[j].image_uuid, &null_guid); + bank[j].accepted = (j == 0) ? 1 : 0; + bank[j].reserved = 0; + } + } + + ret = fwu_update_mdata(metadata); + if (ret < 0) + log_err("Failed to initialize FWU metadata\n"); + else + log_err("Initialized FWU metadata\n"); + free(metadata); +} + +int board_late_init(void) +{ + struct fwu_mdata *metadata; + + if (fwu_get_mdata(&metadata) < 0) { + // Initialize FWU metadata if broken + log_err("Unable to get a valid metadata. Initialize it.\n"); + devbox_init_fwu_mdata(); + } + return 0; +} + +#endif