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

[Pali Rohár]

+ 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...

> ** 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.