[U-Boot] [PATCH] Fix hash verification

[Nikolay Dimitrov]

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 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.

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