[U-Boot] [PATCH 1/2] tools: Fix return code of fit_image_process_sig()
Mario Six
mario.six at gdsys.cc
Fri Jul 22 08:36:54 CEST 2016
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
More information about the U-Boot
mailing list