[U-Boot] [PATCH] Fix hash verification

[Simon Glass]

Hi Nikolay,

On 16 December 2014 at 15:29, Nikolay Dimitrov <picmaster at 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 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

Thanks for the explanation, it looks right to me.

Reviewed-by: Simon Glass <sjg at chromium.org>

Regards,
Simon