diff mbox

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

Message ID 20160719090707.27081-3-mario.six@gdsys.cc
State Accepted
Delegated to: Tom Rini
Headers show

Commit Message

Mario Six July 19, 2016, 9:07 a.m. UTC
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@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(-)

--
2.9.0

Comments

Tom Rini July 23, 2016, 12:13 a.m. UTC | #1
On Tue, Jul 19, 2016 at 11:07:07AM +0200, 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 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@lists.denx.de/msg217417.html
> 
> Signed-off-by: Mario Six <mario.six@gdsys.cc>

Applied to u-boot/master, thanks!
diff mbox

Patch

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);
 	}