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