[scan-admin@coverity.com: New Defects reported by Coverity Scan for Das U-Boot]
Hey all,
Can people please take a look? I can mark as intentional anything that really is intentional, thanks.
----- Forwarded message from scan-admin@coverity.com -----
Date: Mon, 16 Aug 2021 18:33:32 +0000 (UTC) From: scan-admin@coverity.com To: tom.rini@gmail.com Subject: New Defects reported by Coverity Scan for Das U-Boot
Hi,
Please find the latest report on new defect(s) introduced to Das U-Boot found with Coverity Scan.
7 new defect(s) introduced to Das U-Boot found with Coverity Scan. 3 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan Showing 7 of 7 defect(s)
** 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,
** CID 338490: Control flow issues (DEADCODE) /drivers/tpm/sandbox_common.c: 34 in sb_tpm_index_to_seq()
________________________________________________________________________________________________________ *** CID 338490: Control flow issues (DEADCODE) /drivers/tpm/sandbox_common.c: 34 in sb_tpm_index_to_seq() 28 case FWMP_NV_INDEX: 29 return NV_SEQ_FWMP; 30 case MRC_REC_HASH_NV_INDEX: 31 return NV_SEQ_REC_HASH; 32 case 0: 33 return NV_SEQ_GLOBAL_LOCK;
CID 338490: Control flow issues (DEADCODE) Execution cannot reach this statement: "case TPM_NV_INDEX_LOCK:".
34 case TPM_NV_INDEX_LOCK: 35 return NV_SEQ_ENABLE_LOCKING; 36 } 37 38 printf("Invalid nv index %#x\n", index); 39 return -1;
** CID 338489: Control flow issues (DEADCODE) /drivers/tpm/tpm2_tis_sandbox.c: 652 in sandbox_tpm2_xfer()
________________________________________________________________________________________________________ *** CID 338489: Control flow issues (DEADCODE) /drivers/tpm/tpm2_tis_sandbox.c: 652 in sandbox_tpm2_xfer() 646 647 for (i = 0; i < SANDBOX_TPM_PCR_NB; i++) 648 if (pcr_map & BIT(i)) 649 pcr_index = i; 650 651 if (pcr_index >= SANDBOX_TPM_PCR_NB) {
CID 338489: Control flow issues (DEADCODE) Execution cannot reach this statement: "printf("Invalid index %d, s...".
652 printf("Invalid index %d, sandbox TPM handles up to %d PCR(s)\n", 653 pcr_index, SANDBOX_TPM_PCR_NB); 654 rc = TPM2_RC_VALUE; 655 return sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); 656 } 657
** 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),
** CID 338487: Null pointer dereferences (FORWARD_NULL)
________________________________________________________________________________________________________ *** CID 338487: Null pointer dereferences (FORWARD_NULL) /test/dm/ecdsa.c: 34 in dm_test_ecdsa_verify() 28 struct image_sign_info info = { 29 .checksum = &algo, 30 }; 31 32 ut_assertok(uclass_get(UCLASS_ECDSA, &ucp)); 33 ut_assertnonnull(ucp);
CID 338487: Null pointer dereferences (FORWARD_NULL) Passing "&info" to "ecdsa_verify", which dereferences null "info.fdt_blob".
34 ut_asserteq(-ENODEV, ecdsa_verify(&info, NULL, 0, NULL, 0)); 35 36 return 0; 37 }
** 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
** CID 338485: Security best practices violations (STRING_OVERFLOW) /test/str_ut.c: 126 in run_strtoull()
________________________________________________________________________________________________________ *** CID 338485: Security best practices violations (STRING_OVERFLOW) /test/str_ut.c: 126 in run_strtoull() 120 bool upper) 121 { 122 char out[TEST_STR_SIZE]; 123 char *endp; 124 unsigned long long val; 125
CID 338485: Security best practices violations (STRING_OVERFLOW) You might overrun the 200-character fixed-size string "out" by copying "str" without checking the length.
126 strcpy(out, str); 127 if (upper) 128 str_to_upper(out, out, -1); 129 130 val = simple_strtoull(out, &endp, base); 131 ut_asserteq(expect_val, val);
________________________________________________________________________________________________________ To view the defects in Coverity Scan visit, https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0...
To manage Coverity Scan email notifications for "tom.rini@gmail.com", click https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0...
----- End forwarded message -----
+ 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.
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?
participants (2)
-
Pali Rohár -
Tom Rini