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