From 32a711dbdbec9aed6777d1059d115b1c83d36fca Mon Sep 17 00:00:00 2001 From: Jan Kiszka Date: Sat, 5 Feb 2022 13:19:36 +0100 Subject: mkimage: Improve documentation of algo-name parameter Addresses the feedback provided on 5902a397d029 ("mkimage: Allow to specify the signature algorithm on the command line") which raced with the merge. Signed-off-by: Jan Kiszka Reviewed-by: Simon Glass --- tools/imagetool.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'tools') diff --git a/tools/imagetool.h b/tools/imagetool.h index a4105515d81..c3f80fc64e8 100644 --- a/tools/imagetool.h +++ b/tools/imagetool.h @@ -71,7 +71,9 @@ struct image_tool_params { const char *keydest; /* Destination .dtb for public key */ const char *keyfile; /* Filename of private or public key */ const char *comment; /* Comment to add to signature node */ - const char *algo_name; /* Algorithm name to use hashing/signing */ + /* Algorithm name to use for hashing/signing or NULL to use the one + * specified in the its */ + const char *algo_name; int require_keys; /* 1 to mark signing keys as 'required' */ int file_size; /* Total size of output file */ int orig_file_size; /* Original size for file before padding */ -- cgit v1.3.1 From f178f7c9550c4fd9c644f79a1eb2dafa5bcdce25 Mon Sep 17 00:00:00 2001 From: Rafał Miłecki Date: Wed, 12 Jan 2022 12:47:05 +0100 Subject: fw_env: make flash_io() take buffer as an argument MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It's usually easier to understand code & follow it if all arguments are passed explicitly. Many coding styles also discourage using global variables. Behaviour of flash_io() was a bit unintuitive as it was writing to a buffer referenced in a global struct. That required developers to remember how it works and sometimes required hacking "environment" global struct variable to read data into a proper buffer. Signed-off-by: Rafał Miłecki --- tools/env/fw_env.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) (limited to 'tools') diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 3da75be7830..a738b910c29 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -346,7 +346,7 @@ static int ubi_write(int fd, const void *buf, size_t count) return 0; } -static int flash_io(int mode); +static int flash_io(int mode, void *buf, size_t count); static int parse_config(struct env_opts *opts); #if defined(CONFIG_FILE) @@ -516,7 +516,7 @@ int fw_env_flush(struct env_opts *opts) *environment.crc = crc32(0, (uint8_t *) environment.data, ENV_SIZE); /* write environment back to flash */ - if (flash_io(O_RDWR)) { + if (flash_io(O_RDWR, environment.image, CUR_ENVSIZE)) { fprintf(stderr, "Error: can't write fw_env to flash\n"); return -1; } @@ -1185,7 +1185,8 @@ static int flash_flag_obsolete(int dev, int fd, off_t offset) return rc; } -static int flash_write(int fd_current, int fd_target, int dev_target) +static int flash_write(int fd_current, int fd_target, int dev_target, void *buf, + size_t count) { int rc; @@ -1212,11 +1213,10 @@ static int flash_write(int fd_current, int fd_target, int dev_target) if (IS_UBI(dev_target)) { if (ubi_update_start(fd_target, CUR_ENVSIZE) < 0) return -1; - return ubi_write(fd_target, environment.image, CUR_ENVSIZE); + return ubi_write(fd_target, buf, count); } - rc = flash_write_buf(dev_target, fd_target, environment.image, - CUR_ENVSIZE); + rc = flash_write_buf(dev_target, fd_target, buf, count); if (rc < 0) return rc; @@ -1235,17 +1235,17 @@ static int flash_write(int fd_current, int fd_target, int dev_target) return 0; } -static int flash_read(int fd) +static int flash_read(int fd, void *buf, size_t count) { int rc; if (IS_UBI(dev_current)) { DEVTYPE(dev_current) = MTD_ABSENT; - return ubi_read(fd, environment.image, CUR_ENVSIZE); + return ubi_read(fd, buf, count); } - rc = flash_read_buf(dev_current, fd, environment.image, CUR_ENVSIZE, + rc = flash_read_buf(dev_current, fd, buf, count, DEVOFFSET(dev_current)); if (rc != CUR_ENVSIZE) return -1; @@ -1291,7 +1291,7 @@ err: return rc; } -static int flash_io_write(int fd_current) +static int flash_io_write(int fd_current, void *buf, size_t count) { int fd_target = -1, rc, dev_target; const char *dname, *target_temp = NULL; @@ -1322,7 +1322,7 @@ static int flash_io_write(int fd_current) fd_target = fd_current; } - rc = flash_write(fd_current, fd_target, dev_target); + rc = flash_write(fd_current, fd_target, dev_target, buf, count); if (fsync(fd_current) && !(errno == EINVAL || errno == EROFS)) { fprintf(stderr, @@ -1377,7 +1377,7 @@ static int flash_io_write(int fd_current) return rc; } -static int flash_io(int mode) +static int flash_io(int mode, void *buf, size_t count) { int fd_current, rc; @@ -1391,9 +1391,9 @@ static int flash_io(int mode) } if (mode == O_RDWR) { - rc = flash_io_write(fd_current); + rc = flash_io_write(fd_current, buf, count); } else { - rc = flash_read(fd_current); + rc = flash_read(fd_current, buf, count); } if (close(fd_current)) { @@ -1455,7 +1455,7 @@ int fw_env_open(struct env_opts *opts) } dev_current = 0; - if (flash_io(O_RDONLY)) { + if (flash_io(O_RDONLY, environment.image, CUR_ENVSIZE)) { ret = -EIO; goto open_cleanup; } @@ -1490,7 +1490,7 @@ int fw_env_open(struct env_opts *opts) * other pointers in environment still point inside addr0 */ environment.image = addr1; - if (flash_io(O_RDONLY)) { + if (flash_io(O_RDONLY, environment.image, CUR_ENVSIZE)) { ret = -EIO; goto open_cleanup; } -- cgit v1.3.1 From 07c79dd5fdeaefb39c9e7a97f3b66de63109a18d Mon Sep 17 00:00:00 2001 From: Rafał Miłecki Date: Wed, 12 Jan 2022 12:47:06 +0100 Subject: fw_env: simplify logic & code paths in the fw_env_open() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Environment variables can be stored in two formats: 1. Single entry with header containing CRC32 2. Two entries with extra flags field in each entry header For that reason fw_env_open() has two main code paths and there are pointers for CRC32/flags/data. Previous implementation was a bit hard to follow: 1. It was checking for used format twice (in reversed order each time) 2. It was setting "environment" global struct fields to some temporary values that required extra comments explaining it This change simplifies that code: 1. It introduces two clear code paths 2. It sets "environment" global struct fields values only once it really knows them To be fair there are *two* crc32() calls now and an extra pointer variable but that should be cheap enough and worth it. Signed-off-by: Rafał Miłecki --- tools/env/fw_env.c | 77 ++++++++++++++++++++++-------------------------------- 1 file changed, 31 insertions(+), 46 deletions(-) (limited to 'tools') diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index a738b910c29..31afef6f3b1 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -1421,9 +1421,6 @@ int fw_env_open(struct env_opts *opts) int ret; - struct env_image_single *single; - struct env_image_redundant *redundant; - if (!opts) opts = &default_opts; @@ -1439,40 +1436,37 @@ int fw_env_open(struct env_opts *opts) goto open_cleanup; } - /* read environment from FLASH to local buffer */ - environment.image = addr0; - - if (have_redund_env) { - redundant = addr0; - environment.crc = &redundant->crc; - environment.flags = &redundant->flags; - environment.data = redundant->data; - } else { - single = addr0; - environment.crc = &single->crc; - environment.flags = NULL; - environment.data = single->data; - } - dev_current = 0; - if (flash_io(O_RDONLY, environment.image, CUR_ENVSIZE)) { + if (flash_io(O_RDONLY, addr0, CUR_ENVSIZE)) { ret = -EIO; goto open_cleanup; } - crc0 = crc32(0, (uint8_t *)environment.data, ENV_SIZE); - - crc0_ok = (crc0 == *environment.crc); if (!have_redund_env) { + struct env_image_single *single = addr0; + + crc0 = crc32(0, (uint8_t *)single->data, ENV_SIZE); + crc0_ok = (crc0 == single->crc); if (!crc0_ok) { fprintf(stderr, "Warning: Bad CRC, using default environment\n"); - memcpy(environment.data, default_environment, + memcpy(single->data, default_environment, sizeof(default_environment)); environment.dirty = 1; } + + environment.image = addr0; + environment.crc = &single->crc; + environment.flags = NULL; + environment.data = single->data; } else { - flag0 = *environment.flags; + struct env_image_redundant *redundant0 = addr0; + struct env_image_redundant *redundant1; + + crc0 = crc32(0, (uint8_t *)redundant0->data, ENV_SIZE); + crc0_ok = (crc0 == redundant0->crc); + + flag0 = redundant0->flags; dev_current = 1; addr1 = calloc(1, CUR_ENVSIZE); @@ -1483,14 +1477,9 @@ int fw_env_open(struct env_opts *opts) ret = -ENOMEM; goto open_cleanup; } - redundant = addr1; + redundant1 = addr1; - /* - * have to set environment.image for flash_read(), careful - - * other pointers in environment still point inside addr0 - */ - environment.image = addr1; - if (flash_io(O_RDONLY, environment.image, CUR_ENVSIZE)) { + if (flash_io(O_RDONLY, addr1, CUR_ENVSIZE)) { ret = -EIO; goto open_cleanup; } @@ -1518,18 +1507,12 @@ int fw_env_open(struct env_opts *opts) goto open_cleanup; } - crc1 = crc32(0, (uint8_t *)redundant->data, ENV_SIZE); + crc1 = crc32(0, (uint8_t *)redundant1->data, ENV_SIZE); - crc1_ok = (crc1 == redundant->crc); - flag1 = redundant->flags; + crc1_ok = (crc1 == redundant1->crc); + flag1 = redundant1->flags; - /* - * environment.data still points to ((struct - * env_image_redundant *)addr0)->data. If the two - * environments differ, or one has bad crc, force a - * write-out by marking the environment dirty. - */ - if (memcmp(environment.data, redundant->data, ENV_SIZE) || + if (memcmp(redundant0->data, redundant1->data, ENV_SIZE) || !crc0_ok || !crc1_ok) environment.dirty = 1; @@ -1540,7 +1523,7 @@ int fw_env_open(struct env_opts *opts) } else if (!crc0_ok && !crc1_ok) { fprintf(stderr, "Warning: Bad CRC, using default environment\n"); - memcpy(environment.data, default_environment, + memcpy(redundant0->data, default_environment, sizeof(default_environment)); environment.dirty = 1; dev_current = 0; @@ -1586,13 +1569,15 @@ int fw_env_open(struct env_opts *opts) */ if (dev_current) { environment.image = addr1; - environment.crc = &redundant->crc; - environment.flags = &redundant->flags; - environment.data = redundant->data; + environment.crc = &redundant1->crc; + environment.flags = &redundant1->flags; + environment.data = redundant1->data; free(addr0); } else { environment.image = addr0; - /* Other pointers are already set */ + environment.crc = &redundant0->crc; + environment.flags = &redundant0->flags; + environment.data = redundant0->data; free(addr1); } #ifdef DEBUG -- cgit v1.3.1