[PATCH] lib: rsa: Add debug message on algo mismatch

[PATCH] lib: rsa: Add debug message on algo mismatch

[Sean Anderson]

Currently we fail silently if there is an algorithm mismatch. To help
distinguish this failure condition.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 lib/rsa/rsa-verify.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
index e34d3293d1..aee76f42d5 100644
--- a/lib/rsa/rsa-verify.c
+++ b/lib/rsa/rsa-verify.c
@@ -447,8 +447,11 @@ static int rsa_verify_with_keynode(struct image_sign_info *info,
 	}
 
 	algo = fdt_getprop(blob, node, "algo", NULL);
-	if (strcmp(info->name, algo))
+	if (strcmp(info->name, algo)) {
+		debug("%s: Wrong algo: have %s, expected %s", __func__,
+		      info->name, algo);
 		return -EFAULT;
+	}
 
 	prop.num_bits = fdtdec_get_int(blob, node, "rsa,num-bits", 0);
 
-- 
2.25.1

[PATCH] lib: rsa: Add debug message on algo mismatch

[Wolfgang Denk]

Dear Sean Anderson,

In message <20210216164016.635125-1-sean.anderson@seco.com> you wrote:
> Currently we fail silently if there is an algorithm mismatch. To help
> distinguish this failure condition.
>
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
>
>  lib/rsa/rsa-verify.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
> index e34d3293d1..aee76f42d5 100644
> --- a/lib/rsa/rsa-verify.c
> +++ b/lib/rsa/rsa-verify.c
> @@ -447,8 +447,11 @@ static int rsa_verify_with_keynode(struct image_sign_info *info,
>  	}
>  
>  	algo = fdt_getprop(blob, node, "algo", NULL);
> -	if (strcmp(info->name, algo))
> +	if (strcmp(info->name, algo)) {
> +		debug("%s: Wrong algo: have %s, expected %s", __func__,
> +		      info->name, algo);
>  		return -EFAULT;
> +	}

If this is considered an error, should that not be a printf() then
instead of a debug() which users will never see?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It is impractical for  the  standard  to  attempt  to  constrain  the
behavior  of code that does not obey the constraints of the standard.
                                                          - Doug Gwyn

[PATCH] lib: rsa: Add debug message on algo mismatch

[Sean Anderson]

On 2/16/21 12:01 PM, Wolfgang Denk wrote:
 > Dear Sean Anderson,
 >
 > In message <20210216164016.635125-1-sean.anderson@seco.com> you wrote:
 >> Currently we fail silently if there is an algorithm mismatch. To help
 >> distinguish this failure condition.
 >>
 >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >> ---
 >>
 >>   lib/rsa/rsa-verify.c | 5 ++++-
 >>   1 file changed, 4 insertions(+), 1 deletion(-)
 >>
 >> diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
 >> index e34d3293d1..aee76f42d5 100644
 >> --- a/lib/rsa/rsa-verify.c
 >> +++ b/lib/rsa/rsa-verify.c
 >> @@ -447,8 +447,11 @@ static int rsa_verify_with_keynode(struct image_sign_info *info,
 >>   	}
 >>
 >>   	algo = fdt_getprop(blob, node, "algo", NULL);
 >> -	if (strcmp(info->name, algo))
 >> +	if (strcmp(info->name, algo)) {
 >> +		debug("%s: Wrong algo: have %s, expected %s", __func__,
 >> +		      info->name, algo);
 >>   		return -EFAULT;
 >> +	}
 >
 > If this is considered an error, should that not be a printf() then
 > instead of a debug() which users will never see?

I also thought that, but the much of the rest of this file also uses
debug() to report errors. Perhaps there are security implications? Or
perhaps it was done to reduce binary size? Simon, can you comment on
this?

--Sean

 >
 > Best regards,
 >
 > Wolfgang Denk
 >

[PATCH] lib: rsa: Add debug message on algo mismatch

[Simon Glass]

Hi Sean,

On Tue, 16 Feb 2021 at 10:05, Sean Anderson <sean.anderson@seco.com> wrote:
>
>
>
> On 2/16/21 12:01 PM, Wolfgang Denk wrote:
>  > Dear Sean Anderson,
>  >
>  > In message <20210216164016.635125-1-sean.anderson@seco.com> you wrote:
>  >> Currently we fail silently if there is an algorithm mismatch. To help
>  >> distinguish this failure condition.
>  >>
>  >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>  >> ---
>  >>
>  >>   lib/rsa/rsa-verify.c | 5 ++++-
>  >>   1 file changed, 4 insertions(+), 1 deletion(-)
>  >>
>  >> diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
>  >> index e34d3293d1..aee76f42d5 100644
>  >> --- a/lib/rsa/rsa-verify.c
>  >> +++ b/lib/rsa/rsa-verify.c
>  >> @@ -447,8 +447,11 @@ static int rsa_verify_with_keynode(struct image_sign_info *info,
>  >>     }
>  >>
>  >>     algo = fdt_getprop(blob, node, "algo", NULL);
>  >> -   if (strcmp(info->name, algo))
>  >> +   if (strcmp(info->name, algo)) {
>  >> +           debug("%s: Wrong algo: have %s, expected %s", __func__,
>  >> +                 info->name, algo);
>  >>             return -EFAULT;
>  >> +   }
>  >
>  > If this is considered an error, should that not be a printf() then
>  > instead of a debug() which users will never see?
>
> I also thought that, but the much of the rest of this file also uses
> debug() to report errors. Perhaps there are security implications? Or
> perhaps it was done to reduce binary size? Simon, can you comment on
> this?

In general should not print messages in the bowels of the code, since
then there is no way to control what is printed. So long as the error
is produced it can be reported and propagated up, and you can document
what the different error codes mean. It also bloats the code to
include strings everywhere.

I suggest adding logging around the return value as it makes it easy
to trace things:

CONFIG_LOG_ERROR_RETURN=y

and return the error with:

   return log_msg_ret("algo", -EFAULT)

Regards,
Simon

[PATCH] lib: rsa: Add debug message on algo mismatch

[Tom Rini]

On Tue, Feb 16, 2021 at 11:40:15AM -0500, Sean Anderson wrote:

> Currently we fail silently if there is an algorithm mismatch. To help
> distinguish this failure condition.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210225/d31f8c43/attachment.sig>