diff mbox series

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

Message ID 164359755758.280839.2217137492784656400.stgit@localhost
State Accepted
Commit 53b406369e9d0ba2da1df9b2488976c41acc6332
Headers show
Series DFU: Update dfu_alt_info parser etc. | expand

Commit Message

Masami Hiramatsu Jan. 31, 2022, 2:52 a.m. UTC
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(-)

Comments

Tom Rini Feb. 11, 2022, 5:06 p.m. UTC | #1
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!
Michal Simek Aug. 9, 2022, 2:11 p.m. UTC | #2
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);
AKASHI Takahiro Aug. 10, 2022, 12:19 a.m. UTC | #3
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
diff mbox series

Patch

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;