[U-Boot] [PATCH] Fix hash verification

[Nikolay Dimitrov]

Hi Simon,

On 12/17/2014 12:02 AM, Simon Glass wrote:
> Hi Nikolay,
>
> On 12 December 2014 at 11:01,  <picmaster at mail.bg> wrote:
>> From: Nikolay Dimitrov <picmaster at 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 at 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