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?