[U-Boot] [PATCH 1/2] tools: Fix return code of fit_image_process_sig()

[Mario Six]

Hi Simon,

On Fri, Jul 22, 2016 at 5:21 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi Mario,
>
> On 19 July 2016 at 03:07, Mario Six <mario.six at gdsys.cc> wrote:
>> When signing images, we repeatedly call fit_add_file_data() with
>> successively increasing size values to include the keys in the DTB.
>>
>> Unfortunately, if large keys are used (such as 4096 bit RSA keys), this
>> process fails sometimes, and mkimage needs to be called repeatedly to
>> integrate the keys into the DTB.
>>
>> This is because fit_add_file_data actually returns the wrong error
>> code, and the loop terminates prematurely, instead of trying again with
>> a larger size value.
>>
>> This patch corrects the return value and also removes a error message,
>> which is misleading, since we actually allow the function to fail. A
>> (hopefully helpful) comment is also added to explain the lack of error
>> message.
>>
>> This is probably related to 1152a05 ("tools: Correct error handling in
>> fit_image_process_hash()") and the corresponding error reported here:
>>
>> https://www.mail-archive.com/u-boot@lists.denx.de/msg217417.html
>>
>> Signed-off-by: Mario Six <mario.six at gdsys.cc>
>> ---
>>  tools/image-host.c | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/image-host.c b/tools/image-host.c
>> index 3e14fdc..399ec94 100644
>> --- a/tools/image-host.c
>> +++ b/tools/image-host.c
>> @@ -238,12 +238,13 @@ static int fit_image_process_sig(const char *keydir, void *keydest,
>>         /* Get keyname again, as FDT has changed and invalidated our pointer */
>>         info.keyname = fdt_getprop(fit, noffset, "key-name-hint", NULL);
>>
>> -       /* Write the public key into the supplied FDT file */
>> -       if (keydest && info.algo->add_verify_data(&info, keydest)) {
>> -               printf("Failed to add verification data for '%s' signature node in '%s' image node\n",
>> -                      node_name, image_name);
>> -               return -1;
>> -       }
>> +       ret = info.algo->add_verify_data(&info, keydest);
>
> What happens if keydest is NULL here? Don't you need to check for that?
>

In this case keydest cannot be NULL, since the (unique) call hierarchy looks
like this:

fit_add_file_data in tools/fit_image.c:60
-> fit_add_verification_data in tools/image-host.c:680
-> fit_image_add_verification_data in tools/image-host.c:322
-> fit_image_process_sig in tools/image-host.c:242

And the keydest parameter, hence, ultimately comes from the mmap_fdt call in
fit_add_file_data at line 44, and mmap_fdt only returns a non-negative value
iff the 4th parameter (here the dest_blob pointer) has been assigned a
reasonable value.

But, it is admittedly a bit brittle, so I'll just add a check to be sure (I
need to respin the series anyway to fix the comment).

>> +
>> +       /* Write the public key into the supplied FDT file; this might fail
>
> /*
>  * Write the ...
>

Will fix in v2.

>> +        * several times, since we try signing with successively increasing
>> +        * size values */
>> +       if (keydest && ret)
>> +               return ret;
>>
>>         return 0;
>>  }
>> --
>> 2.9.0
>>
>
> Regards,
> Simon

Thanks for reviewing!

Best regards,

Mario