On Thu, 1 Aug 2024 18:46:33 +0900 "Seung-Woo Kim" <[email protected]> wrote:
> Hi, > > > -----Original Message----- > > From: Andre Przywara <[email protected]> > > Sent: Thursday, August 1, 2024 6:28 PM > > Subject: Re: [PATCH] tools: imagetool: Remove unnecessary check from > > toc0_verify_cert_item() > > > > On Thu, 1 Aug 2024 10:01:00 +0900 > > Seung-Woo Kim <[email protected]> wrote: > > > > Hi, > > > > > Remove unnecessary null check from toc0_verify_cert_item() because the > > > array digest is always not null. > > > > Do you mean it's always not NULL *as currently used*? Because digest is a > > function parameter, so within the scope of this function definition can be > > NULL. > > I agree that *currently* there is only one caller, and this passes the > > addresses of a local variable from the stack, though it's never NULL. > > > > But I don't think this check hurts (looks like the compiler removes it > > anyway), and it makes the code more robust. So is there any problem with > > this check? Why do you want it to be removed? > > Because in my gcc-14 environment, it gives -Wnonnull-compare warning. Ah, I see, it's GCC 14 reporting this now. And after some digging I found digest cannot be NULL, as the digest parameter is declared as "digest[static SHA256_DIGEST_LENGTH]" (mind the "static" within the brackets). So yes, the patch is correct. I will amend the commit message while applying. > tools/sunxi_toc0.c: In function 'toc0_verify_cert_item': > tools/sunxi_toc0.c:447:12: warning: 'nonnull' argument 'digest' compared to > NULL [-Wnonnull-compare] > 447 | if (digest && memcmp(&extension->digest, digest, > SHA256_DIGEST_LENGTH)) { > | ^ > > Regards, > - Seung-Woo Kim > > > > > Cheers, > > Andre > > > > > > > > Signed-off-by: Seung-Woo Kim <[email protected]> Reviewed-by: Andre Przywara <[email protected]> Cheers, Andre > > > --- > > > tools/sunxi_toc0.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/tools/sunxi_toc0.c b/tools/sunxi_toc0.c index > > > 292649fe90f1..76693647a095 100644 > > > --- a/tools/sunxi_toc0.c > > > +++ b/tools/sunxi_toc0.c > > > @@ -444,7 +444,7 @@ static int toc0_verify_cert_item(const uint8_t > > > *buf, uint32_t len, RSA *fw_key, > > > > > > /* If a digest was provided, compare it to the embedded digest. */ > > > extension = &totalSequence->mainSequence.explicit3.extension; > > > - if (digest && memcmp(&extension->digest, digest, > > SHA256_DIGEST_LENGTH)) { > > > + if (memcmp(&extension->digest, digest, SHA256_DIGEST_LENGTH)) { > > > pr_err("Wrong firmware digest in certificate\n"); > > > goto err; > > > } > >
