[U-Boot] [PATCH] Fix hash verification
From: Nikolay Dimitrov picmaster@mail.bg
Fix issue in parse_verify_sum() which swaps handling of env-var and *address. Move hash_command() argc check earlier. Cosmetic change on do_hash() variable declaration. Improved help message for "hash" command.
Signed-off-by: Nikolay Dimitrov picmaster@mail.bg --- common/cmd_hash.c | 28 +++++++++++++--------------- common/hash.c | 6 ++---- 2 files changed, 15 insertions(+), 19 deletions(-)
diff --git a/common/cmd_hash.c b/common/cmd_hash.c index 90facbb..704d21e 100644 --- a/common/cmd_hash.c +++ b/common/cmd_hash.c @@ -18,9 +18,9 @@ static int do_hash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { char *s; -#ifdef CONFIG_HASH_VERIFY int flags = HASH_FLAG_ENV;
+#ifdef CONFIG_HASH_VERIFY if (argc < 4) return CMD_RET_USAGE; if (!strcmp(argv[1], "-v")) { @@ -28,8 +28,6 @@ static int do_hash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) argc--; argv++; } -#else - const int flags = HASH_FLAG_ENV; #endif /* Move forward to 'algorithm' parameter */ argc--; @@ -40,19 +38,19 @@ static int do_hash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) }
#ifdef CONFIG_HASH_VERIFY -U_BOOT_CMD( - hash, 6, 1, do_hash, - "compute hash message digest", - "algorithm address count [[*]sum_dest]\n" - " - compute message digest [save to env var / *address]\n" - "hash -v algorithm address count [*]sum\n" - " - verify hash of memory area with env var / *address" -); +#define HARGS 6 #else +#define HARGS 5 +#endif + U_BOOT_CMD( - hash, 5, 1, do_hash, - "compute message digest", - "algorithm address count [[*]sum_dest]\n" + hash, HARGS, 1, do_hash, + "compute hash message digest", + "algorithm address count [[*]hash_dest]\n" " - compute message digest [save to env var / *address]" -); +#ifdef CONFIG_HASH_VERIFY + "\nhash -v algorithm address count [*]hash\n" + " - verify message digest of memory area to immediate value, \n" + " env var or *address" #endif +); diff --git a/common/hash.c b/common/hash.c index 12d6759..aceabc5 100644 --- a/common/hash.c +++ b/common/hash.c @@ -256,7 +256,7 @@ static int parse_verify_sum(struct hash_algo *algo, char *verify_str, env_var = 1; }
- if (env_var) { + if (!env_var) { ulong addr; void *buf;
@@ -347,7 +347,7 @@ int hash_command(const char *algo_name, int flags, cmd_tbl_t *cmdtp, int flag, { ulong addr, len;
- if (argc < 2) + if ((argc < 2) || ((flags & HASH_FLAG_VERIFY) && (argc < 3))) return CMD_RET_USAGE;
addr = simple_strtoul(*argv++, NULL, 16); @@ -380,8 +380,6 @@ int hash_command(const char *algo_name, int flags, cmd_tbl_t *cmdtp, int flag, #else if (0) { #endif - if (!argc) - return CMD_RET_USAGE; if (parse_verify_sum(algo, *argv, vsum, flags & HASH_FLAG_ENV)) { printf("ERROR: %s does not contain a valid "
Hi Nikolay,
On 12 December 2014 at 11:01, picmaster@mail.bg wrote:
From: Nikolay Dimitrov picmaster@mail.bg
Fix issue in parse_verify_sum() which swaps handling of env-var and *address. Move hash_command() argc check earlier. Cosmetic change on do_hash() variable declaration. Improved help message for "hash" command.
Signed-off-by: Nikolay Dimitrov picmaster@mail.bg
Thanks for this. Main change looks good, a few nits.
common/cmd_hash.c | 28 +++++++++++++--------------- common/hash.c | 6 ++---- 2 files changed, 15 insertions(+), 19 deletions(-)
diff --git a/common/cmd_hash.c b/common/cmd_hash.c index 90facbb..704d21e 100644 --- a/common/cmd_hash.c +++ b/common/cmd_hash.c @@ -18,9 +18,9 @@ static int do_hash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { char *s; -#ifdef CONFIG_HASH_VERIFY int flags = HASH_FLAG_ENV;
+#ifdef CONFIG_HASH_VERIFY if (argc < 4) return CMD_RET_USAGE; if (!strcmp(argv[1], "-v")) { @@ -28,8 +28,6 @@ static int do_hash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) argc--; argv++; } -#else
const int flags = HASH_FLAG_ENV;#endif /* Move forward to 'algorithm' parameter */ argc--; @@ -40,19 +38,19 @@ static int do_hash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) }
#ifdef CONFIG_HASH_VERIFY -U_BOOT_CMD(
hash, 6, 1, do_hash,"compute hash message digest","algorithm address count [[*]sum_dest]\n"" - compute message digest [save to env var / *address]\n""hash -v algorithm address count [*]sum\n"" - verify hash of memory area with env var / *address"-); +#define HARGS 6 #else +#define HARGS 5 +#endif
U_BOOT_CMD(
hash, 5, 1, do_hash,"compute message digest","algorithm address count [[*]sum_dest]\n"
hash, HARGS, 1, do_hash,"compute hash message digest","algorithm address count [[*]hash_dest]\n" " - compute message digest [save to env var / *address]"-); +#ifdef CONFIG_HASH_VERIFY
"\nhash -v algorithm address count [*]hash\n"" - verify message digest of memory area to immediate value, \n"
Perhaps " verify message digest of memory area, display result or write to env var or *address"
#endif +); diff --git a/common/hash.c b/common/hash.c index 12d6759..aceabc5 100644 --- a/common/hash.c +++ b/common/hash.c @@ -256,7 +256,7 @@ static int parse_verify_sum(struct hash_algo *algo, char *verify_str, env_var = 1; }
if (env_var) {
if (!env_var) { ulong addr; void *buf;@@ -347,7 +347,7 @@ int hash_command(const char *algo_name, int flags, cmd_tbl_t *cmdtp, int flag, { ulong addr, len;
if (argc < 2)
if ((argc < 2) || ((flags & HASH_FLAG_VERIFY) && (argc < 3))) return CMD_RET_USAGE; addr = simple_strtoul(*argv++, NULL, 16);@@ -380,8 +380,6 @@ int hash_command(const char *algo_name, int flags, cmd_tbl_t *cmdtp, int flag, #else if (0) { #endif
if (!argc)return CMD_RET_USAGE;
What does this change achieve?
if (parse_verify_sum(algo, *argv, vsum, flags & HASH_FLAG_ENV)) { printf("ERROR: %s does not contain a valid "-- 1.7.10.4
Regards, Simon
Hi Simon,
On 12/17/2014 12:02 AM, Simon Glass wrote:
Hi Nikolay,
On 12 December 2014 at 11:01, picmaster@mail.bg wrote:
From: Nikolay Dimitrov picmaster@mail.bg
Fix issue in parse_verify_sum() which swaps handling of env-var and *address. Move hash_command() argc check earlier. Cosmetic change on do_hash() variable declaration. Improved help message for "hash" command.
Signed-off-by: Nikolay Dimitrov picmaster@mail.bg
Thanks for this. Main change looks good, a few nits.
common/cmd_hash.c | 28 +++++++++++++--------------- common/hash.c | 6 ++---- 2 files changed, 15 insertions(+), 19 deletions(-)
diff --git a/common/cmd_hash.c b/common/cmd_hash.c index 90facbb..704d21e 100644 --- a/common/cmd_hash.c +++ b/common/cmd_hash.c @@ -18,9 +18,9 @@ static int do_hash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { char *s; -#ifdef CONFIG_HASH_VERIFY int flags = HASH_FLAG_ENV;
+#ifdef CONFIG_HASH_VERIFY if (argc < 4) return CMD_RET_USAGE; if (!strcmp(argv[1], "-v")) { @@ -28,8 +28,6 @@ static int do_hash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) argc--; argv++; } -#else
const int flags = HASH_FLAG_ENV;@@ -40,19 +38,19 @@ static int do_hash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) }
#ifdef CONFIG_HASH_VERIFY -U_BOOT_CMD(
hash, 6, 1, do_hash,"compute hash message digest","algorithm address count [[*]sum_dest]\n"" - compute message digest [save to env var / *address]\n""hash -v algorithm address count [*]sum\n"" - verify hash of memory area with env var / *address"-); +#define HARGS 6 #else +#define HARGS 5 +#endif
- U_BOOT_CMD(
hash, 5, 1, do_hash,"compute message digest","algorithm address count [[*]sum_dest]\n"
hash, HARGS, 1, do_hash,"compute hash message digest","algorithm address count [[*]hash_dest]\n" " - compute message digest [save to env var / *address]"-); +#ifdef CONFIG_HASH_VERIFY
"\nhash -v algorithm address count [*]hash\n"" - verify message digest of memory area to immediate value, \n"Perhaps " verify message digest of memory area, display result or write to env var or *address"
I think that the initial description was appropriate, because the "hash" command supports 3 modes of operation (I chose crc32 for shorter examples):
1. Verify hash against immediate value, like this
hash -v crc32 0x20000000 $filesize e9b11acd
2. Verify hash against environment variable, which holds the actual hash value
hash -v crc32 0x20000000 $filesize env_var_name
3, Verify hash against value, placed at specific memory address
hash -v crc32 0x20000000 $filesize *0x30000000
As far as I observed the code and its behavior, the only case when the "hash -v" result was printed was when the verification failed. Please correct me if I'm wrong.
#endif +); diff --git a/common/hash.c b/common/hash.c index 12d6759..aceabc5 100644 --- a/common/hash.c +++ b/common/hash.c @@ -256,7 +256,7 @@ static int parse_verify_sum(struct hash_algo *algo, char *verify_str, env_var = 1; }
if (env_var) {
if (!env_var) { ulong addr; void *buf;@@ -347,7 +347,7 @@ int hash_command(const char *algo_name, int flags, cmd_tbl_t *cmdtp, int flag, { ulong addr, len;
if (argc < 2)
if ((argc < 2) || ((flags & HASH_FLAG_VERIFY) && (argc < 3))) return CMD_RET_USAGE; addr = simple_strtoul(*argv++, NULL, 16);@@ -380,8 +380,6 @@ int hash_command(const char *algo_name, int flags, cmd_tbl_t *cmdtp, int flag, #else if (0) { #endif
if (!argc)return CMD_RET_USAGE;What does this change achieve?
I moved the verification earlier, because the original implementation first calculated the hash and just then complained about the last missing argument. My personal understanding is that errors should be caught as early as possible, and work should be done only as necessary :D.
if (parse_verify_sum(algo, *argv, vsum, flags & HASH_FLAG_ENV)) { printf("ERROR: %s does not contain a valid "-- 1.7.10.4
Regards, Simon
Kind regards, Nikolay
Hi Simon,
I omitted one clarification, which I think it's important.
On 12/17/2014 12:25 AM, Nikolay Dimitrov wrote:
Hi Simon,
On 12/17/2014 12:02 AM, Simon Glass wrote:
Hi Nikolay,
On 12 December 2014 at 11:01, picmaster@mail.bg wrote:
From: Nikolay Dimitrov picmaster@mail.bg
Fix issue in parse_verify_sum() which swaps handling of env-var and *address. Move hash_command() argc check earlier. Cosmetic change on do_hash() variable declaration. Improved help message for "hash" command.
Signed-off-by: Nikolay Dimitrov picmaster@mail.bg
Thanks for this. Main change looks good, a few nits.
common/cmd_hash.c | 28 +++++++++++++--------------- common/hash.c | 6 ++---- 2 files changed, 15 insertions(+), 19 deletions(-)
diff --git a/common/cmd_hash.c b/common/cmd_hash.c index 90facbb..704d21e 100644 --- a/common/cmd_hash.c +++ b/common/cmd_hash.c @@ -18,9 +18,9 @@ static int do_hash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { char *s; -#ifdef CONFIG_HASH_VERIFY int flags = HASH_FLAG_ENV;
+#ifdef CONFIG_HASH_VERIFY if (argc < 4) return CMD_RET_USAGE; if (!strcmp(argv[1], "-v")) { @@ -28,8 +28,6 @@ static int do_hash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) argc--; argv++; } -#else
const int flags = HASH_FLAG_ENV;@@ -40,19 +38,19 @@ static int do_hash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) }
#ifdef CONFIG_HASH_VERIFY -U_BOOT_CMD(
hash, 6, 1, do_hash,"compute hash message digest","algorithm address count [[*]sum_dest]\n"" - compute message digest [save to env var /*address]\n"
"hash -v algorithm address count [*]sum\n"" - verify hash of memory area with env var /*address" -); +#define HARGS 6 #else +#define HARGS 5 +#endif
- U_BOOT_CMD(
hash, 5, 1, do_hash,"compute message digest","algorithm address count [[*]sum_dest]\n"
hash, HARGS, 1, do_hash,"compute hash message digest","algorithm address count [[*]hash_dest]\n" " - compute message digest [save to env var /*address]" -); +#ifdef CONFIG_HASH_VERIFY
"\nhash -v algorithm address count [*]hash\n"" - verify message digest of memory area toimmediate value, \n"
Perhaps " verify message digest of memory area, display result or write to env var or *address"
I think that the initial description was appropriate, because the "hash" command supports 3 modes of operation (I chose crc32 for shorter examples):
- Verify hash against immediate value, like this
hash -v crc32 0x20000000 $filesize e9b11acd
- Verify hash against environment variable, which holds the actual
hash value
hash -v crc32 0x20000000 $filesize env_var_name
3, Verify hash against value, placed at specific memory address
hash -v crc32 0x20000000 $filesize *0x30000000
As far as I observed the code and its behavior, the only case when the "hash -v" result was printed was when the verification failed. Please correct me if I'm wrong.
In all these modes of verification, the last argument is used as the "reference value" to compare against, this is another reason why it would be inappropriate to say "...or write to env var or *address".
#endif +); diff --git a/common/hash.c b/common/hash.c index 12d6759..aceabc5 100644 --- a/common/hash.c +++ b/common/hash.c @@ -256,7 +256,7 @@ static int parse_verify_sum(struct hash_algo *algo, char *verify_str, env_var = 1; }
if (env_var) {
if (!env_var) { ulong addr; void *buf;@@ -347,7 +347,7 @@ int hash_command(const char *algo_name, int flags, cmd_tbl_t *cmdtp, int flag, { ulong addr, len;
if (argc < 2)
if ((argc < 2) || ((flags & HASH_FLAG_VERIFY) && (argc < 3))) return CMD_RET_USAGE; addr = simple_strtoul(*argv++, NULL, 16);@@ -380,8 +380,6 @@ int hash_command(const char *algo_name, int flags, cmd_tbl_t *cmdtp, int flag, #else if (0) { #endif
if (!argc)return CMD_RET_USAGE;What does this change achieve?
I moved the verification earlier, because the original implementation first calculated the hash and just then complained about the last missing argument. My personal understanding is that errors should be caught as early as possible, and work should be done only as necessary :D.
if (parse_verify_sum(algo, *argv, vsum, flags & HASH_FLAG_ENV)) { printf("ERROR: %s does not contain avalid "
1.7.10.4
Regards, Simon
Kind regards, Nikolay
Kind regards, Nikolay
Hi Nikolay,
On 16 December 2014 at 15:29, Nikolay Dimitrov picmaster@mail.bg wrote:
Hi Simon,
I omitted one clarification, which I think it's important.
On 12/17/2014 12:25 AM, Nikolay Dimitrov wrote:
Hi Simon,
On 12/17/2014 12:02 AM, Simon Glass wrote:
Hi Nikolay,
On 12 December 2014 at 11:01, picmaster@mail.bg wrote:
From: Nikolay Dimitrov picmaster@mail.bg
Fix issue in parse_verify_sum() which swaps handling of env-var and *address. Move hash_command() argc check earlier. Cosmetic change on do_hash() variable declaration. Improved help message for "hash" command.
Signed-off-by: Nikolay Dimitrov picmaster@mail.bg
Thanks for this. Main change looks good, a few nits.
common/cmd_hash.c | 28 +++++++++++++--------------- common/hash.c | 6 ++---- 2 files changed, 15 insertions(+), 19 deletions(-)
diff --git a/common/cmd_hash.c b/common/cmd_hash.c index 90facbb..704d21e 100644 --- a/common/cmd_hash.c +++ b/common/cmd_hash.c @@ -18,9 +18,9 @@ static int do_hash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { char *s; -#ifdef CONFIG_HASH_VERIFY int flags = HASH_FLAG_ENV;
+#ifdef CONFIG_HASH_VERIFY if (argc < 4) return CMD_RET_USAGE; if (!strcmp(argv[1], "-v")) { @@ -28,8 +28,6 @@ static int do_hash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) argc--; argv++; } -#else
const int flags = HASH_FLAG_ENV;@@ -40,19 +38,19 @@ static int do_hash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) }
#ifdef CONFIG_HASH_VERIFY -U_BOOT_CMD(
hash, 6, 1, do_hash,"compute hash message digest","algorithm address count [[*]sum_dest]\n"" - compute message digest [save to env var /*address]\n"
"hash -v algorithm address count [*]sum\n"" - verify hash of memory area with env var /*address" -); +#define HARGS 6 #else +#define HARGS 5 +#endif
- U_BOOT_CMD(
hash, 5, 1, do_hash,"compute message digest","algorithm address count [[*]sum_dest]\n"
hash, HARGS, 1, do_hash,"compute hash message digest","algorithm address count [[*]hash_dest]\n" " - compute message digest [save to env var /*address]" -); +#ifdef CONFIG_HASH_VERIFY
"\nhash -v algorithm address count [*]hash\n"" - verify message digest of memory area toimmediate value, \n"
Perhaps " verify message digest of memory area, display result or write to env var or *address"
I think that the initial description was appropriate, because the "hash" command supports 3 modes of operation (I chose crc32 for shorter examples):
- Verify hash against immediate value, like this
hash -v crc32 0x20000000 $filesize e9b11acd
- Verify hash against environment variable, which holds the actual
hash value
hash -v crc32 0x20000000 $filesize env_var_name
3, Verify hash against value, placed at specific memory address
hash -v crc32 0x20000000 $filesize *0x30000000
As far as I observed the code and its behavior, the only case when the "hash -v" result was printed was when the verification failed. Please correct me if I'm wrong.
In all these modes of verification, the last argument is used as the "reference value" to compare against, this is another reason why it would be inappropriate to say "...or write to env var or *address".
#endif +); diff --git a/common/hash.c b/common/hash.c index 12d6759..aceabc5 100644 --- a/common/hash.c +++ b/common/hash.c @@ -256,7 +256,7 @@ static int parse_verify_sum(struct hash_algo *algo, char *verify_str, env_var = 1; }
if (env_var) {
if (!env_var) { ulong addr; void *buf;@@ -347,7 +347,7 @@ int hash_command(const char *algo_name, int flags, cmd_tbl_t *cmdtp, int flag, { ulong addr, len;
if (argc < 2)
if ((argc < 2) || ((flags & HASH_FLAG_VERIFY) && (argc < 3))) return CMD_RET_USAGE; addr = simple_strtoul(*argv++, NULL, 16);@@ -380,8 +380,6 @@ int hash_command(const char *algo_name, int flags, cmd_tbl_t *cmdtp, int flag, #else if (0) { #endif
if (!argc)return CMD_RET_USAGE;What does this change achieve?
I moved the verification earlier, because the original implementation first calculated the hash and just then complained about the last missing argument. My personal understanding is that errors should be caught as early as possible, and work should be done only as necessary :D.
if (parse_verify_sum(algo, *argv, vsum, flags & HASH_FLAG_ENV)) { printf("ERROR: %s does not contain avalid "
1.7.10.4
Thanks for the explanation, it looks right to me.
Reviewed-by: Simon Glass sjg@chromium.org
Regards, Simon
On Fri, Dec 12, 2014 at 08:01:23PM +0200, Nikolay Dimitrov wrote:
From: Nikolay Dimitrov picmaster@mail.bg
Fix issue in parse_verify_sum() which swaps handling of env-var and *address. Move hash_command() argc check earlier. Cosmetic change on do_hash() variable declaration. Improved help message for "hash" command.
Signed-off-by: Nikolay Dimitrov picmaster@mail.bg Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
participants (4)
-
Nikolay Dimitrov -
picmaster＠mail.bg
-
Simon Glass -
Tom Rini