* [U-Boot] [PATCH] Fix hash verification @ 2014-12-12 18:01 picmaster at mail.bg 2014-12-16 22:02 ` Simon Glass 2014-12-30 2:26 ` [U-Boot] " Tom Rini 0 siblings, 2 replies; 6+ messages in thread From: picmaster at mail.bg @ 2014-12-12 18:01 UTC (permalink / raw) To: u-boot 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 " -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] Fix hash verification 2014-12-12 18:01 [U-Boot] [PATCH] Fix hash verification picmaster at mail.bg @ 2014-12-16 22:02 ` Simon Glass 2014-12-16 22:25 ` Nikolay Dimitrov 2014-12-30 2:26 ` [U-Boot] " Tom Rini 1 sibling, 1 reply; 6+ messages in thread From: Simon Glass @ 2014-12-16 22:02 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] Fix hash verification 2014-12-16 22:02 ` Simon Glass @ 2014-12-16 22:25 ` Nikolay Dimitrov 2014-12-16 22:29 ` Nikolay Dimitrov 0 siblings, 1 reply; 6+ messages in thread From: Nikolay Dimitrov @ 2014-12-16 22:25 UTC (permalink / raw) To: u-boot 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; >> #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" 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] Fix hash verification 2014-12-16 22:25 ` Nikolay Dimitrov @ 2014-12-16 22:29 ` Nikolay Dimitrov 2014-12-16 22:30 ` Simon Glass 0 siblings, 1 reply; 6+ messages in thread From: Nikolay Dimitrov @ 2014-12-16 22:29 UTC (permalink / raw) To: u-boot 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; >>> #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" > > 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. 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 a >>> valid " >>> -- >>> 1.7.10.4 >>> >> >> Regards, >> Simon > > Kind regards, > Nikolay Kind regards, Nikolay ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] Fix hash verification 2014-12-16 22:29 ` Nikolay Dimitrov @ 2014-12-16 22:30 ` Simon Glass 0 siblings, 0 replies; 6+ messages in thread From: Simon Glass @ 2014-12-16 22:30 UTC (permalink / raw) To: u-boot 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; >>>> #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" >> >> >> 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. > > > 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 a >>>> valid " >>>> -- >>>> 1.7.10.4 Thanks for the explanation, it looks right to me. Reviewed-by: Simon Glass <sjg@chromium.org> Regards, Simon ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] Fix hash verification 2014-12-12 18:01 [U-Boot] [PATCH] Fix hash verification picmaster at mail.bg 2014-12-16 22:02 ` Simon Glass @ 2014-12-30 2:26 ` Tom Rini 1 sibling, 0 replies; 6+ messages in thread From: Tom Rini @ 2014-12-30 2:26 UTC (permalink / raw) To: u-boot 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! -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141229/3af43ba5/attachment.pgp> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-12-30 2:26 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-12-12 18:01 [U-Boot] [PATCH] Fix hash verification picmaster at mail.bg 2014-12-16 22:02 ` Simon Glass 2014-12-16 22:25 ` Nikolay Dimitrov 2014-12-16 22:29 ` Nikolay Dimitrov 2014-12-16 22:30 ` Simon Glass 2014-12-30 2:26 ` [U-Boot] " Tom Rini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.