[PATCH] tools: imagetool: Remove unnecessary check from toc0_verify_cert_item()

[Andre Przywara]

On Thu, 1 Aug 2024 18:46:33 +0900
"Seung-Woo Kim" <sw0312.kim at samsung.com> wrote:

> Hi,
> 
> > -----Original Message-----
> > From: Andre Przywara <andre.przywara at arm.com>
> > 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 <sw0312.kim at samsung.com> 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 <sw0312.kim at samsung.com>

Reviewed-by: Andre Przywara <andre.przywara at arm.com>

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