Re: [scan-admin@coverity.com: New Defects reported by Coverity Scan for Das U-Boot]

From: Tom Rini <trini@konsulko.com>
To: "Pali Rohár" <pali@kernel.org>
Cc: u-boot@lists.denx.de, "Simon Glass" <sjg@chromium.org>,
	"Alexandru Gagniuc" <mr.nuke.me@gmail.com>,
	"Stefan Roese" <sr@denx.de>, "Marek Behún" <marek.behun@nic.cz>
Subject: Re: [scan-admin@coverity.com: New Defects reported by Coverity Scan for Das U-Boot]
Date: Mon, 16 Aug 2021 16:20:27 -0400	[thread overview]
Message-ID: <20210816202027.GE858@bill-the-cat> (raw)
In-Reply-To: <20210816201549.kzjonaqei2rwpefq@pali>

[-- Attachment #1: Type: text/plain, Size: 4009 bytes --]

On Mon, Aug 16, 2021 at 10:15:49PM +0200, Pali Rohár wrote:
> + Stefan and Marek
> 
> On Monday 16 August 2021 15:57:26 Tom Rini wrote:
> > Hey all,
> > 
> > Can people please take a look?  I can mark as intentional anything that
> > really is intentional, thanks.
> 
> Hello Tom!
> 
> These kwbimage issues look to be a real issues. But I do not think that
> anybody touched these parts of kwbimage code recently. So looks like
> that Coverity must have run some more tests this time...

Yeah, that happens from time to time.

> 
> > ** CID 338491:  Null pointer dereferences  (NULL_RETURNS)
> > /tools/kwbimage.c: 1066 in export_pub_kak_hash()
> > 
> > 
> > ________________________________________________________________________________________________________
> > *** CID 338491:  Null pointer dereferences  (NULL_RETURNS)
> > /tools/kwbimage.c: 1066 in export_pub_kak_hash()
> > 1060     	int res;
> > 1061     
> > 1062     	hashf = fopen("pub_kak_hash.txt", "w");
> > 1063     
> > 1064     	res = kwb_export_pubkey(kak, &secure_hdr->kak, hashf, "KAK");
> > 1065     
> > >>>     CID 338491:  Null pointer dereferences  (NULL_RETURNS)
> > >>>     Dereferencing a pointer that might be "NULL" "hashf" when calling "fclose".
> > 1066     	fclose(hashf);
> > 1067     
> > 1068     	return res < 0 ? 1 : 0;
> > 1069     }
> > 1070     
> > 1071     int kwb_sign_csk_with_kak(struct image_tool_params *params,
> 
> There is really missing check that fopen() succeeded.
> 
> > ** CID 338488:  Memory - illegal accesses  (NEGATIVE_RETURNS)
> > /tools/kwbimage.c: 1093 in kwb_sign_csk_with_kak()
> > 
> > 
> > ________________________________________________________________________________________________________
> > *** CID 338488:  Memory - illegal accesses  (NEGATIVE_RETURNS)
> > /tools/kwbimage.c: 1093 in kwb_sign_csk_with_kak()
> > 1087     	if (export_pub_kak_hash(kak, secure_hdr))
> > 1088     		return 1;
> > 1089     
> > 1090     	if (kwb_import_pubkey(&kak_pub, &secure_hdr->kak, "KAK") < 0)
> > 1091     		return 1;
> > 1092     
> > >>>     CID 338488:  Memory - illegal accesses  (NEGATIVE_RETURNS)
> > >>>     Using variable "csk_idx" as an index to array "secure_hdr->csk".
> > 1093     	if (kwb_export_pubkey(csk, &secure_hdr->csk[csk_idx], NULL, "CSK") < 0)
> > 1094     		return 1;
> > 1095     
> > 1096     	if (kwb_sign_and_verify(kak, &secure_hdr->csk,
> > 1097     				sizeof(secure_hdr->csk) +
> > 1098     				sizeof(secure_hdr->csksig),
> 
> There is code:
> 
>   int csk_idx = image_get_csk_index();
>   ...
>   if (csk_idx >= 16) {
>     ...
>     return 1;
>   }
>   ... &secure_hdr->csk[csk_idx] ...
> 
> And ->csk is defined as:
> 
>   struct secure_hdr_v1 {
>     ..
>     struct pubkey_der_v1 csk[16]
>     ..
>   };
> 
> image_get_csk_index() returns int and it may returns also negative value
> on error. So there is really possible illegal memory access.
> 
> > ** CID 338486:  Null pointer dereferences  (NULL_RETURNS)
> > /tools/kwbimage.c: 836 in kwb_dump_fuse_cmds()
> > 
> > 
> > ________________________________________________________________________________________________________
> > *** CID 338486:  Null pointer dereferences  (NULL_RETURNS)
> > /tools/kwbimage.c: 836 in kwb_dump_fuse_cmds()
> > 830     		return 0;
> > 831     
> > 832     	if (!strcmp(e->name, "a38x")) {
> > 833     		FILE *out = fopen("kwb_fuses_a38x.txt", "w+");
> > 834     
> > 835     		kwb_dump_fuse_cmds_38x(out, sec_hdr);
> > >>>     CID 338486:  Null pointer dereferences  (NULL_RETURNS)
> > >>>     Dereferencing a pointer that might be "NULL" "out" when calling "fclose".
> > 836     		fclose(out);
> > 837     		goto done;
> > 838     	}
> > 839     
> > 840     	ret = -ENOSYS;
> > 841     
> 
> And there is also missing check that fopen() succeeded.

Since you've been in here and analyzed things (thanks!) can you make a
few patches for things?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]