Hi Simon, On Fri, Jul 22, 2016 at 5:21 AM, Simon Glass <s...@chromium.org> wrote: > Hi Mario, > > On 19 July 2016 at 03:07, Mario Six <mario....@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....@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot