[U-Boot] [PATCH 0/2] Fix signing problems with large keys

[U-Boot] [PATCH 0/2] Fix signing problems with large keys

[Mario Six]

This patch series fixes a problem that occurs when signing FIT images with
large keys (e.g. RSA with 4096 bits).

Signing sometimes fails because of unexpected return values from
fit_add_file_data() and the functions called by it.

Some error messages are also removed, since we tolerate failure of the
corresponding functions.

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 at lists.denx.de/msg217417.html

Mario Six (2):
  tools: Fix return code of fit_image_process_sig()
  rsa: Fix return value and masked error

 lib/rsa/rsa-sign.c | 10 +++++-----
 tools/image-host.c | 13 +++++++------
 2 files changed, 12 insertions(+), 11 deletions(-)

--
2.9.0

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

[Mario Six]

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 at lists.denx.de/msg217417.html

Signed-off-by: Mario Six <mario.six@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);
+
+	/* Write the public key into the supplied FDT file; this might fail
+	 * several times, since we try signing with successively increasing
+	 * size values */
+	if (keydest && ret)
+		return ret;

 	return 0;
 }
--
2.9.0

[U-Boot] [PATCH 2/2] rsa: Fix return value and masked error

[Mario Six]

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 by fixing the return value of
fdt_add_bignum, fixes a case where an error is masked by a unconditional
setting of a return value variable, 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 at lists.denx.de/msg217417.html

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 lib/rsa/rsa-sign.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c
index 5d9716f..d4a1a5e 100644
--- a/lib/rsa/rsa-sign.c
+++ b/lib/rsa/rsa-sign.c
@@ -420,11 +420,11 @@ static int fdt_add_bignum(void *blob, int noffset, const char *prop_name,
 		BN_rshift(num, num, 32); /*  N = N/B */
 	}

+	/* We try signing with successively increasing size values, so this
+	 * might fail several times */
 	ret = fdt_setprop(blob, noffset, prop_name, buf, size);
-	if (ret) {
-		fprintf(stderr, "Failed to write public key to FIT\n");
-		return -ENOSPC;
-	}
+	if (ret)
+		return -FDT_ERR_NOSPACE;
 	free(buf);
 	BN_free(tmp);
 	BN_free(big2);
@@ -508,7 +508,7 @@ int rsa_add_verify_data(struct image_sign_info *info, void *keydest)
 		ret = fdt_setprop_string(keydest, node, FIT_ALGO_PROP,
 					 info->algo->name);
 	}
-	if (info->require_keys) {
+	if (!ret && info->require_keys) {
 		ret = fdt_setprop_string(keydest, node, "required",
 					 info->require_keys);
 	}
--
2.9.0

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

[Simon Glass]

Hi Mario,

On 19 July 2016 at 03:07, Mario Six <mario.six@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 at lists.denx.de/msg217417.html
>
> Signed-off-by: Mario Six <mario.six@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?

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

/*
 * Write the ...

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

Regards,
Simon

[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@chromium.org> wrote:
> Hi Mario,
>
> On 19 July 2016 at 03:07, Mario Six <mario.six@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 at lists.denx.de/msg217417.html
>>
>> Signed-off-by: Mario Six <mario.six@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] [U-Boot, 1/2] tools: Fix return code of fit_image_process_sig()

[Tom Rini]

On Tue, Jul 19, 2016 at 11:07:06AM +0200, 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 at lists.denx.de/msg217417.html
> 
> Signed-off-by: Mario Six <mario.six@gdsys.cc>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160722/4139aa4b/attachment.sig>

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

[Tom Rini]

On Tue, Jul 19, 2016 at 11:07:06AM +0200, 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 at lists.denx.de/msg217417.html
> 
> Signed-off-by: Mario Six <mario.six@gdsys.cc>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160722/908a1f2a/attachment.sig>

[U-Boot] [U-Boot,2/2] rsa: Fix return value and masked error

[Tom Rini]

On Tue, Jul 19, 2016 at 11:07:07AM +0200, 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 by fixing the return value of
> fdt_add_bignum, fixes a case where an error is masked by a unconditional
> setting of a return value variable, 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 at lists.denx.de/msg217417.html
> 
> Signed-off-by: Mario Six <mario.six@gdsys.cc>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160722/bb8081e5/attachment.sig>