Yo dog this ain't right. #1
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -6,8 +6,8 @@ | |||
| typedef | |||
| EFI_STATUS | |||
| (EFIAPI *EFI_SHIM_LOCK_VERIFY) ( | |||
| IN VOID *buffer; | |||
There was a problem hiding this comment.
there's a dot over the comma here that isn't right.
vathpela
referenced
this pull request
in vathpela/mallory
Oct 18, 2017
…an less. Because they don't believe code should be defensive against future changes, covscan believes: 520 out_free: 521 FreePool(dmp); CID 182824 (#1 of 1): Dereference before null check (REVERSE_INULL)check_after_deref: Null-checking entries suggests that it may be null, but it has already been dereferenced on all paths leading to the check. 522 if (entries) { 523 free_entries(entries, count); 524 FreePool(entries); 525 } 526 out_free_name: 527 FreePool(name); 528} Which is technically correct, but still kind of dumb. So this patch combines the two error out paths into just being out_free, so that the first path there is before entries is allocated. (It also initializes dmp to NULL and checks that before freeing it.) I also Lindent-ed that function. Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 18, 2017
Covscan daftly claims:
288. var_compare_op: Comparing MokSB to null implies that MokSB might be null.
2330 if (MokSB) {
2331 menu_strings[i] = L"Change Secure Boot state";
2332 menu_item[i] = MOK_CHANGE_SB;
2333 i++;
2334 }
2335
...
2358 choice = console_select(perform_mok_mgmt, menu_strings, 0);
2359 if (choice < 0)
2360 goto out;
...
2362 switch (menu_item[choice]) {
...
2395 case MOK_CHANGE_SB:
CID 182841 (#1 of 1): Dereference after null check
(FORWARD_NULL)293. var_deref_model: Passing null pointer MokSB to
mok_sb_prompt, which dereferences it. [show details]
2396 efi_status = mok_sb_prompt(MokSB, MokSBSize);
Which is, of course, entirely false, beause for menu_item[choice] to be
MOK_CHANGE_SB, MokSB must be !NULL. And then:
252. Condition efi_status == 0, taking true branch.
2397 if (efi_status == EFI_SUCCESS)
2398 MokSB = NULL;
This guarantees it won't be in the list the next time through the loop.
This adds tests for NULLness before mok_sb_prompt(), just to make it
more clear to covscan what's going on.
Also do the same thing for all of:
MOK_CHANGE_SB
MOK_SET_PW
MOK_CHANGE_DB
MOK_ENROLL_MOKX
MOK_DELETE_MOKX
I also Lindent-ed everything I had to touch, and changed a pile of
(efi_status == EFI_SUCCESS) and (efi_status != EFI_SUCCESS) conditionals
to (!EFI_ERROR(efi_status)) and (EFI_ERROR(efi_status)), respectively.
Three other minor errors are also fixed:
1) the loop in enter_mok_menu() leaked the menu allocations each time
through the loop
2) mok_sb_prompt(), mok_pw_prompt(), and mok_db_prompt() all call
FreePool() on their respective variables (MokSB, etc), and
check_mok_request() also calls FreePool() on these. This sounds
horrible, but it turns out it's not an issue, because they only free
them in their EFI_SUCCESS paths, and enter_mok_menu() resets the
system if any of the mok_XX_prompt() calls actually returned
EFI_SUCCESS, so we never get back to check_mok_request() for it to do
its FreePool() calls.
3) the loop in enter_mok_menu() winds up introducing a double free in
the call to free_menu(), but we also can't hit this bug, because all
the exit paths from the loop are "goto out" (or return error) rather
than actually exiting on the loop conditional.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 18, 2017
Covscan believes the following:
782 if ((EFI_ERROR(rc) || !bootarchcsv) && bootcsv) {
783 EFI_FILE_HANDLE fh2;
784 rc = uefi_call_wrapper(fh->Open, 5, fh, &fh2,
785 bootcsv, EFI_FILE_READ_ONLY, 0);
786 if (EFI_ERROR(rc) || fh2 == NULL) {
787 Print(L"Couldn't open \\EFI\\%s\\%s: %d\n",
788 dirname, bootcsv, rc);
789 } else {
CID 182829 (#1 of 1): Unused value (UNUSED_VALUE)returned_value:
Assigning value from try_boot_csv(fh2, dirname, bootcsv) to rc here,
but that stored value is overwritten before it can be used.
790 rc = try_boot_csv(fh2, dirname, bootcsv);
791 uefi_call_wrapper(fh2->Close, 1, fh2);
792 }
793 }
value_overwrite: Overwriting previous write to rc with value 0UL.
794 rc = EFI_SUCCESS;
795
796 return rc;
797}
Which isn't untrue, we just don't happen to be using the return code for
anything, before we intentionally return success to our caller.
So that's annoying, but whatever. Just print the error as well.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 18, 2017
Covscan sez:
720 FreePool(buffer);
assignment: Assigning: buffer = NULL.
721 buffer = NULL;
722
723 CHAR16 *bootcsv=NULL, *bootarchcsv=NULL;
724
725 bs = 0;
726 do {
727 bs = 0;
728 rc = uefi_call_wrapper(fh->Read, 3, fh, &bs, NULL);
729 if (EFI_ERROR(rc) && rc != EFI_BUFFER_TOO_SMALL) {
730 Print(L"Could not read \\EFI\\%s\\: %d\n", dirname, rc);
null: At condition buffer, the value of buffer must be NULL.
dead_error_condition: The condition buffer cannot be true.
731 if (buffer)
CID 182851 (#1 of 1): Logically dead code (DEADCODE)dead_error_line:
Execution cannot reach this statement: FreePool(buffer);.
732 FreePool(buffer);
733 return rc;
734 }
And it's right; buffer can never be non-NULL there. So just take that
out.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 18, 2017
Covscan believes all this stuff:
852 bs = 0;
853 rc = uefi_call_wrapper(fh2->Read, 3, fh2, &bs, NULL);
7. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
8. Condition rc == 0, taking false branch.
15. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
16. Condition rc == 0, taking true branch.
17. Condition bs != 0, taking true branch.
30. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
31. Condition rc == 0, taking false branch.
854 if (rc == EFI_BUFFER_TOO_SMALL ||
855 (rc == EFI_SUCCESS && bs != 0)) {
856 buffer = AllocateZeroPool(bs);
18. Condition !buffer, taking false branch.
857 if (!buffer) {
858 Print(L"Could not allocate memory\n");
859 /* sure, this might work, why not? */
860 uefi_call_wrapper(fh2->Close, 1, fh2);
861 uefi_call_wrapper(fh->Close, 1, fh);
862 return EFI_OUT_OF_RESOURCES;
863 }
864
865 rc = uefi_call_wrapper(fh2->Read, 3, fh2, &bs, buffer);
866 }
9. Condition bs == 0, taking false branch.
19. Condition bs == 0, taking false branch.
32. Condition bs == 0, taking false branch.
867 if (bs == 0)
868 break;
869
10. Condition (INTN)rc < 0, taking false branch.
20. Condition (INTN)rc < 0, taking false branch.
33. Condition (INTN)rc < 0, taking false branch.
870 if (EFI_ERROR(rc)) {
871 Print(L"Could not read \\EFI\\: %d\n", rc);
872 if (buffer) {
873 FreePool(buffer);
874 buffer = NULL;
875 }
876 uefi_call_wrapper(fh2->Close, 1, fh2);
877 uefi_call_wrapper(fh->Close, 1, fh);
878 return rc;
879 }
34. alias_transfer: Assigning: fi = buffer.
880 EFI_FILE_INFO *fi = buffer;
881
11. Condition !(fi->Attribute & 16), taking false branch.
21. Condition !(fi->Attribute & 16), taking false branch.
CID 182858 (#1-3 of 3): Explicit null dereferenced (FORWARD_NULL)35. var_deref_op: Dereferencing null pointer fi.
882 if (!(fi->Attribute & EFI_FILE_DIRECTORY)) {
883 FreePool(buffer);
884 buffer = NULL;
885 continue;
886 }
Because it doesn't know that when bs==0, fh2->Read() will return
EFI_BUFFER_TOO_SMALL and set bs to the size we need to allocate, so the
allocation path is always taken. Instead, handle our exit/error paths
directly there, and make the allocation path nonconditional.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 18, 2017
…mber.
Covscan noticed:
746static EFI_STATUS generate_hash (char *data, unsigned int datasize_in,
747 PE_COFF_LOADER_IMAGE_CONTEXT *context,
748 UINT8 *sha256hash, UINT8 *sha1hash)
749
750{
...
764
CID 182849 (#1 of 1): Unsigned compared against 0
(NO_EFFECT)unsigned_compare: This less-than-zero comparison of an
unsigned value is never true. datasize_in < 0U.
765 if (datasize_in < 0) {
766 perror(L"Invalid data size\n");
767 return EFI_INVALID_PARAMETER;
768 }
And I guess that's a fair point, but some of the callers take the size
as a signed integer. So we should be handling that on all the input
cases instead of getting that far.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 18, 2017
Covscan says:
455 if (IsFound) {
456 tpm_measure_variable(dbname, guid, CertSize, Cert->SignatureData);
457 return DATA_FOUND;
CID 182850 (#1 of 1): Structurally dead code (UNREACHABLE)unreachable: This code cannot be reached: drain_openssl_errors();.
458 drain_openssl_errors();
459 } else {
460 LogError(L"AuthenticodeVerify(): %d\n", IsFound);
461 }
And, well... woops.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 18, 2017
"FixupData" in the edk2 tree is a log of the relocations that happened,
which is allocated by the "client" calling relocate, and written into
while it does relocations. Since we never allocate that log anywhere,
FixupData is always NULL, and so covscan says:
318 case EFI_IMAGE_REL_BASED_HIGH:
319 Fixup16 = (UINT16 *) Fixup;
320 *Fixup16 = (UINT16) (*Fixup16 + ((UINT16) ((UINT32) Adjust >> 16)));
null: At condition FixupData != NULL, the value of FixupData must be
NULL. dead_error_condition: The condition FixupData != NULL cannot
be true.
321 if (FixupData != NULL) {
CID 182859 (#1 of 4): Logically dead code (DEADCODE)dead_error_begin:
Execution cannot reach this statement: *((UINT16 *)FixupData) =
*F....
322 *(UINT16 *) FixupData = *Fixup16;
323 FixupData = FixupData + sizeof (UINT16);
324 }
325 break;
And it's right; all four occurrances are deadcode that never do anything
but confuse the reader.
Kill it with fire.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 19, 2017
…an less. Because they don't believe code should be defensive against future changes, covscan believes: 520 out_free: 521 FreePool(dmp); CID 182824 (#1 of 1): Dereference before null check (REVERSE_INULL)check_after_deref: Null-checking entries suggests that it may be null, but it has already been dereferenced on all paths leading to the check. 522 if (entries) { 523 free_entries(entries, count); 524 FreePool(entries); 525 } 526 out_free_name: 527 FreePool(name); 528} Which is technically correct, but still kind of dumb. So this patch combines the two error out paths into just being out_free, so that the first path there is before entries is allocated. (It also initializes dmp to NULL and checks that before freeing it.) I also Lindent-ed that function. Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 19, 2017
Covscan daftly claims:
288. var_compare_op: Comparing MokSB to null implies that MokSB might be null.
2330 if (MokSB) {
2331 menu_strings[i] = L"Change Secure Boot state";
2332 menu_item[i] = MOK_CHANGE_SB;
2333 i++;
2334 }
2335
...
2358 choice = console_select(perform_mok_mgmt, menu_strings, 0);
2359 if (choice < 0)
2360 goto out;
...
2362 switch (menu_item[choice]) {
...
2395 case MOK_CHANGE_SB:
CID 182841 (#1 of 1): Dereference after null check
(FORWARD_NULL)293. var_deref_model: Passing null pointer MokSB to
mok_sb_prompt, which dereferences it. [show details]
2396 efi_status = mok_sb_prompt(MokSB, MokSBSize);
Which is, of course, entirely false, beause for menu_item[choice] to be
MOK_CHANGE_SB, MokSB must be !NULL. And then:
252. Condition efi_status == 0, taking true branch.
2397 if (efi_status == EFI_SUCCESS)
2398 MokSB = NULL;
This guarantees it won't be in the list the next time through the loop.
This adds tests for NULLness before mok_sb_prompt(), just to make it
more clear to covscan what's going on.
Also do the same thing for all of:
MOK_CHANGE_SB
MOK_SET_PW
MOK_CHANGE_DB
MOK_ENROLL_MOKX
MOK_DELETE_MOKX
I also Lindent-ed everything I had to touch, and changed a pile of
(efi_status == EFI_SUCCESS) and (efi_status != EFI_SUCCESS) conditionals
to (!EFI_ERROR(efi_status)) and (EFI_ERROR(efi_status)), respectively.
Three other minor errors are also fixed:
1) the loop in enter_mok_menu() leaked the menu allocations each time
through the loop
2) mok_sb_prompt(), mok_pw_prompt(), and mok_db_prompt() all call
FreePool() on their respective variables (MokSB, etc), and
check_mok_request() also calls FreePool() on these. This sounds
horrible, but it turns out it's not an issue, because they only free
them in their EFI_SUCCESS paths, and enter_mok_menu() resets the
system if any of the mok_XX_prompt() calls actually returned
EFI_SUCCESS, so we never get back to check_mok_request() for it to do
its FreePool() calls.
3) the loop in enter_mok_menu() winds up introducing a double free in
the call to free_menu(), but we also can't hit this bug, because all
the exit paths from the loop are "goto out" (or return error) rather
than actually exiting on the loop conditional.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 19, 2017
Covscan believes the following:
782 if ((EFI_ERROR(rc) || !bootarchcsv) && bootcsv) {
783 EFI_FILE_HANDLE fh2;
784 rc = uefi_call_wrapper(fh->Open, 5, fh, &fh2,
785 bootcsv, EFI_FILE_READ_ONLY, 0);
786 if (EFI_ERROR(rc) || fh2 == NULL) {
787 Print(L"Couldn't open \\EFI\\%s\\%s: %d\n",
788 dirname, bootcsv, rc);
789 } else {
CID 182829 (#1 of 1): Unused value (UNUSED_VALUE)returned_value:
Assigning value from try_boot_csv(fh2, dirname, bootcsv) to rc here,
but that stored value is overwritten before it can be used.
790 rc = try_boot_csv(fh2, dirname, bootcsv);
791 uefi_call_wrapper(fh2->Close, 1, fh2);
792 }
793 }
value_overwrite: Overwriting previous write to rc with value 0UL.
794 rc = EFI_SUCCESS;
795
796 return rc;
797}
Which isn't untrue, we just don't happen to be using the return code for
anything, before we intentionally return success to our caller.
So that's annoying, but whatever. Just print the error as well.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 19, 2017
Covscan sez:
720 FreePool(buffer);
assignment: Assigning: buffer = NULL.
721 buffer = NULL;
722
723 CHAR16 *bootcsv=NULL, *bootarchcsv=NULL;
724
725 bs = 0;
726 do {
727 bs = 0;
728 rc = uefi_call_wrapper(fh->Read, 3, fh, &bs, NULL);
729 if (EFI_ERROR(rc) && rc != EFI_BUFFER_TOO_SMALL) {
730 Print(L"Could not read \\EFI\\%s\\: %d\n", dirname, rc);
null: At condition buffer, the value of buffer must be NULL.
dead_error_condition: The condition buffer cannot be true.
731 if (buffer)
CID 182851 (#1 of 1): Logically dead code (DEADCODE)dead_error_line:
Execution cannot reach this statement: FreePool(buffer);.
732 FreePool(buffer);
733 return rc;
734 }
And it's right; buffer can never be non-NULL there. So just take that
out.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 19, 2017
Covscan believes all this stuff:
852 bs = 0;
853 rc = uefi_call_wrapper(fh2->Read, 3, fh2, &bs, NULL);
7. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
8. Condition rc == 0, taking false branch.
15. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
16. Condition rc == 0, taking true branch.
17. Condition bs != 0, taking true branch.
30. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
31. Condition rc == 0, taking false branch.
854 if (rc == EFI_BUFFER_TOO_SMALL ||
855 (rc == EFI_SUCCESS && bs != 0)) {
856 buffer = AllocateZeroPool(bs);
18. Condition !buffer, taking false branch.
857 if (!buffer) {
858 Print(L"Could not allocate memory\n");
859 /* sure, this might work, why not? */
860 uefi_call_wrapper(fh2->Close, 1, fh2);
861 uefi_call_wrapper(fh->Close, 1, fh);
862 return EFI_OUT_OF_RESOURCES;
863 }
864
865 rc = uefi_call_wrapper(fh2->Read, 3, fh2, &bs, buffer);
866 }
9. Condition bs == 0, taking false branch.
19. Condition bs == 0, taking false branch.
32. Condition bs == 0, taking false branch.
867 if (bs == 0)
868 break;
869
10. Condition (INTN)rc < 0, taking false branch.
20. Condition (INTN)rc < 0, taking false branch.
33. Condition (INTN)rc < 0, taking false branch.
870 if (EFI_ERROR(rc)) {
871 Print(L"Could not read \\EFI\\: %d\n", rc);
872 if (buffer) {
873 FreePool(buffer);
874 buffer = NULL;
875 }
876 uefi_call_wrapper(fh2->Close, 1, fh2);
877 uefi_call_wrapper(fh->Close, 1, fh);
878 return rc;
879 }
34. alias_transfer: Assigning: fi = buffer.
880 EFI_FILE_INFO *fi = buffer;
881
11. Condition !(fi->Attribute & 16), taking false branch.
21. Condition !(fi->Attribute & 16), taking false branch.
CID 182858 (#1-3 of 3): Explicit null dereferenced (FORWARD_NULL)35. var_deref_op: Dereferencing null pointer fi.
882 if (!(fi->Attribute & EFI_FILE_DIRECTORY)) {
883 FreePool(buffer);
884 buffer = NULL;
885 continue;
886 }
Because it doesn't know that when bs==0, fh2->Read() will return
EFI_BUFFER_TOO_SMALL and set bs to the size we need to allocate, so the
allocation path is always taken. Instead, handle our exit/error paths
directly there, and make the allocation path nonconditional.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 19, 2017
…mber.
Covscan noticed:
746static EFI_STATUS generate_hash (char *data, unsigned int datasize_in,
747 PE_COFF_LOADER_IMAGE_CONTEXT *context,
748 UINT8 *sha256hash, UINT8 *sha1hash)
749
750{
...
764
CID 182849 (#1 of 1): Unsigned compared against 0
(NO_EFFECT)unsigned_compare: This less-than-zero comparison of an
unsigned value is never true. datasize_in < 0U.
765 if (datasize_in < 0) {
766 perror(L"Invalid data size\n");
767 return EFI_INVALID_PARAMETER;
768 }
And I guess that's a fair point, but some of the callers take the size
as a signed integer. So we should be handling that on all the input
cases instead of getting that far.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 19, 2017
Covscan says:
455 if (IsFound) {
456 tpm_measure_variable(dbname, guid, CertSize, Cert->SignatureData);
457 return DATA_FOUND;
CID 182850 (#1 of 1): Structurally dead code (UNREACHABLE)unreachable: This code cannot be reached: drain_openssl_errors();.
458 drain_openssl_errors();
459 } else {
460 LogError(L"AuthenticodeVerify(): %d\n", IsFound);
461 }
And, well... woops.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 19, 2017
"FixupData" in the edk2 tree is a log of the relocations that happened,
which is allocated by the "client" calling relocate, and written into
while it does relocations. Since we never allocate that log anywhere,
FixupData is always NULL, and so covscan says:
318 case EFI_IMAGE_REL_BASED_HIGH:
319 Fixup16 = (UINT16 *) Fixup;
320 *Fixup16 = (UINT16) (*Fixup16 + ((UINT16) ((UINT32) Adjust >> 16)));
null: At condition FixupData != NULL, the value of FixupData must be
NULL. dead_error_condition: The condition FixupData != NULL cannot
be true.
321 if (FixupData != NULL) {
CID 182859 (#1 of 4): Logically dead code (DEADCODE)dead_error_begin:
Execution cannot reach this statement: *((UINT16 *)FixupData) =
*F....
322 *(UINT16 *) FixupData = *Fixup16;
323 FixupData = FixupData + sizeof (UINT16);
324 }
325 break;
And it's right; all four occurrances are deadcode that never do anything
but confuse the reader.
Kill it with fire.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 19, 2017
…an less. Because they don't believe code should be defensive against future changes, covscan believes: 520 out_free: 521 FreePool(dmp); CID 182824 (#1 of 1): Dereference before null check (REVERSE_INULL)check_after_deref: Null-checking entries suggests that it may be null, but it has already been dereferenced on all paths leading to the check. 522 if (entries) { 523 free_entries(entries, count); 524 FreePool(entries); 525 } 526 out_free_name: 527 FreePool(name); 528} Which is technically correct, but still kind of dumb. So this patch combines the two error out paths into just being out_free, so that the first path there is before entries is allocated. (It also initializes dmp to NULL and checks that before freeing it.) I also Lindent-ed that function. Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 19, 2017
Covscan daftly claims:
288. var_compare_op: Comparing MokSB to null implies that MokSB might be null.
2330 if (MokSB) {
2331 menu_strings[i] = L"Change Secure Boot state";
2332 menu_item[i] = MOK_CHANGE_SB;
2333 i++;
2334 }
2335
...
2358 choice = console_select(perform_mok_mgmt, menu_strings, 0);
2359 if (choice < 0)
2360 goto out;
...
2362 switch (menu_item[choice]) {
...
2395 case MOK_CHANGE_SB:
CID 182841 (#1 of 1): Dereference after null check
(FORWARD_NULL)293. var_deref_model: Passing null pointer MokSB to
mok_sb_prompt, which dereferences it. [show details]
2396 efi_status = mok_sb_prompt(MokSB, MokSBSize);
Which is, of course, entirely false, beause for menu_item[choice] to be
MOK_CHANGE_SB, MokSB must be !NULL. And then:
252. Condition efi_status == 0, taking true branch.
2397 if (efi_status == EFI_SUCCESS)
2398 MokSB = NULL;
This guarantees it won't be in the list the next time through the loop.
This adds tests for NULLness before mok_sb_prompt(), just to make it
more clear to covscan what's going on.
Also do the same thing for all of:
MOK_CHANGE_SB
MOK_SET_PW
MOK_CHANGE_DB
MOK_ENROLL_MOKX
MOK_DELETE_MOKX
I also Lindent-ed everything I had to touch, and changed a pile of
(efi_status == EFI_SUCCESS) and (efi_status != EFI_SUCCESS) conditionals
to (!EFI_ERROR(efi_status)) and (EFI_ERROR(efi_status)), respectively.
Three other minor errors are also fixed:
1) the loop in enter_mok_menu() leaked the menu allocations each time
through the loop
2) mok_sb_prompt(), mok_pw_prompt(), and mok_db_prompt() all call
FreePool() on their respective variables (MokSB, etc), and
check_mok_request() also calls FreePool() on these. This sounds
horrible, but it turns out it's not an issue, because they only free
them in their EFI_SUCCESS paths, and enter_mok_menu() resets the
system if any of the mok_XX_prompt() calls actually returned
EFI_SUCCESS, so we never get back to check_mok_request() for it to do
its FreePool() calls.
3) the loop in enter_mok_menu() winds up introducing a double free in
the call to free_menu(), but we also can't hit this bug, because all
the exit paths from the loop are "goto out" (or return error) rather
than actually exiting on the loop conditional.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 19, 2017
Covscan believes the following:
782 if ((EFI_ERROR(rc) || !bootarchcsv) && bootcsv) {
783 EFI_FILE_HANDLE fh2;
784 rc = uefi_call_wrapper(fh->Open, 5, fh, &fh2,
785 bootcsv, EFI_FILE_READ_ONLY, 0);
786 if (EFI_ERROR(rc) || fh2 == NULL) {
787 Print(L"Couldn't open \\EFI\\%s\\%s: %d\n",
788 dirname, bootcsv, rc);
789 } else {
CID 182829 (#1 of 1): Unused value (UNUSED_VALUE)returned_value:
Assigning value from try_boot_csv(fh2, dirname, bootcsv) to rc here,
but that stored value is overwritten before it can be used.
790 rc = try_boot_csv(fh2, dirname, bootcsv);
791 uefi_call_wrapper(fh2->Close, 1, fh2);
792 }
793 }
value_overwrite: Overwriting previous write to rc with value 0UL.
794 rc = EFI_SUCCESS;
795
796 return rc;
797}
Which isn't untrue, we just don't happen to be using the return code for
anything, before we intentionally return success to our caller.
So that's annoying, but whatever. Just print the error as well.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 19, 2017
Covscan sez:
720 FreePool(buffer);
assignment: Assigning: buffer = NULL.
721 buffer = NULL;
722
723 CHAR16 *bootcsv=NULL, *bootarchcsv=NULL;
724
725 bs = 0;
726 do {
727 bs = 0;
728 rc = uefi_call_wrapper(fh->Read, 3, fh, &bs, NULL);
729 if (EFI_ERROR(rc) && rc != EFI_BUFFER_TOO_SMALL) {
730 Print(L"Could not read \\EFI\\%s\\: %d\n", dirname, rc);
null: At condition buffer, the value of buffer must be NULL.
dead_error_condition: The condition buffer cannot be true.
731 if (buffer)
CID 182851 (#1 of 1): Logically dead code (DEADCODE)dead_error_line:
Execution cannot reach this statement: FreePool(buffer);.
732 FreePool(buffer);
733 return rc;
734 }
And it's right; buffer can never be non-NULL there. So just take that
out.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 19, 2017
Covscan believes all this stuff:
852 bs = 0;
853 rc = uefi_call_wrapper(fh2->Read, 3, fh2, &bs, NULL);
7. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
8. Condition rc == 0, taking false branch.
15. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
16. Condition rc == 0, taking true branch.
17. Condition bs != 0, taking true branch.
30. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
31. Condition rc == 0, taking false branch.
854 if (rc == EFI_BUFFER_TOO_SMALL ||
855 (rc == EFI_SUCCESS && bs != 0)) {
856 buffer = AllocateZeroPool(bs);
18. Condition !buffer, taking false branch.
857 if (!buffer) {
858 Print(L"Could not allocate memory\n");
859 /* sure, this might work, why not? */
860 uefi_call_wrapper(fh2->Close, 1, fh2);
861 uefi_call_wrapper(fh->Close, 1, fh);
862 return EFI_OUT_OF_RESOURCES;
863 }
864
865 rc = uefi_call_wrapper(fh2->Read, 3, fh2, &bs, buffer);
866 }
9. Condition bs == 0, taking false branch.
19. Condition bs == 0, taking false branch.
32. Condition bs == 0, taking false branch.
867 if (bs == 0)
868 break;
869
10. Condition (INTN)rc < 0, taking false branch.
20. Condition (INTN)rc < 0, taking false branch.
33. Condition (INTN)rc < 0, taking false branch.
870 if (EFI_ERROR(rc)) {
871 Print(L"Could not read \\EFI\\: %d\n", rc);
872 if (buffer) {
873 FreePool(buffer);
874 buffer = NULL;
875 }
876 uefi_call_wrapper(fh2->Close, 1, fh2);
877 uefi_call_wrapper(fh->Close, 1, fh);
878 return rc;
879 }
34. alias_transfer: Assigning: fi = buffer.
880 EFI_FILE_INFO *fi = buffer;
881
11. Condition !(fi->Attribute & 16), taking false branch.
21. Condition !(fi->Attribute & 16), taking false branch.
CID 182858 (#1-3 of 3): Explicit null dereferenced (FORWARD_NULL)35. var_deref_op: Dereferencing null pointer fi.
882 if (!(fi->Attribute & EFI_FILE_DIRECTORY)) {
883 FreePool(buffer);
884 buffer = NULL;
885 continue;
886 }
Because it doesn't know that when bs==0, fh2->Read() will return
EFI_BUFFER_TOO_SMALL and set bs to the size we need to allocate, so the
allocation path is always taken. Instead, handle our exit/error paths
directly there, and make the allocation path nonconditional.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 19, 2017
…mber.
Covscan noticed:
746static EFI_STATUS generate_hash (char *data, unsigned int datasize_in,
747 PE_COFF_LOADER_IMAGE_CONTEXT *context,
748 UINT8 *sha256hash, UINT8 *sha1hash)
749
750{
...
764
CID 182849 (#1 of 1): Unsigned compared against 0
(NO_EFFECT)unsigned_compare: This less-than-zero comparison of an
unsigned value is never true. datasize_in < 0U.
765 if (datasize_in < 0) {
766 perror(L"Invalid data size\n");
767 return EFI_INVALID_PARAMETER;
768 }
And I guess that's a fair point, but some of the callers take the size
as a signed integer. So we should be handling that on all the input
cases instead of getting that far.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 19, 2017
Covscan says:
455 if (IsFound) {
456 tpm_measure_variable(dbname, guid, CertSize, Cert->SignatureData);
457 return DATA_FOUND;
CID 182850 (#1 of 1): Structurally dead code (UNREACHABLE)unreachable: This code cannot be reached: drain_openssl_errors();.
458 drain_openssl_errors();
459 } else {
460 LogError(L"AuthenticodeVerify(): %d\n", IsFound);
461 }
And, well... woops.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 19, 2017
"FixupData" in the edk2 tree is a log of the relocations that happened,
which is allocated by the "client" calling relocate, and written into
while it does relocations. Since we never allocate that log anywhere,
FixupData is always NULL, and so covscan says:
318 case EFI_IMAGE_REL_BASED_HIGH:
319 Fixup16 = (UINT16 *) Fixup;
320 *Fixup16 = (UINT16) (*Fixup16 + ((UINT16) ((UINT32) Adjust >> 16)));
null: At condition FixupData != NULL, the value of FixupData must be
NULL. dead_error_condition: The condition FixupData != NULL cannot
be true.
321 if (FixupData != NULL) {
CID 182859 (#1 of 4): Logically dead code (DEADCODE)dead_error_begin:
Execution cannot reach this statement: *((UINT16 *)FixupData) =
*F....
322 *(UINT16 *) FixupData = *Fixup16;
323 FixupData = FixupData + sizeof (UINT16);
324 }
325 break;
And it's right; all four occurrances are deadcode that never do anything
but confuse the reader.
Kill it with fire.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 20, 2017
…an less. Because they don't believe code should be defensive against future changes, covscan believes: 520 out_free: 521 FreePool(dmp); CID 182824 (#1 of 1): Dereference before null check (REVERSE_INULL)check_after_deref: Null-checking entries suggests that it may be null, but it has already been dereferenced on all paths leading to the check. 522 if (entries) { 523 free_entries(entries, count); 524 FreePool(entries); 525 } 526 out_free_name: 527 FreePool(name); 528} Which is technically correct, but still kind of dumb. So this patch combines the two error out paths into just being out_free, so that the first path there is before entries is allocated. (It also initializes dmp to NULL and checks that before freeing it.) I also Lindent-ed that function. Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 27, 2017
Covscan believes all this stuff:
852 bs = 0;
853 rc = uefi_call_wrapper(fh2->Read, 3, fh2, &bs, NULL);
7. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
8. Condition rc == 0, taking false branch.
15. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
16. Condition rc == 0, taking true branch.
17. Condition bs != 0, taking true branch.
30. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
31. Condition rc == 0, taking false branch.
854 if (rc == EFI_BUFFER_TOO_SMALL ||
855 (rc == EFI_SUCCESS && bs != 0)) {
856 buffer = AllocateZeroPool(bs);
18. Condition !buffer, taking false branch.
857 if (!buffer) {
858 Print(L"Could not allocate memory\n");
859 /* sure, this might work, why not? */
860 uefi_call_wrapper(fh2->Close, 1, fh2);
861 uefi_call_wrapper(fh->Close, 1, fh);
862 return EFI_OUT_OF_RESOURCES;
863 }
864
865 rc = uefi_call_wrapper(fh2->Read, 3, fh2, &bs, buffer);
866 }
9. Condition bs == 0, taking false branch.
19. Condition bs == 0, taking false branch.
32. Condition bs == 0, taking false branch.
867 if (bs == 0)
868 break;
869
10. Condition (INTN)rc < 0, taking false branch.
20. Condition (INTN)rc < 0, taking false branch.
33. Condition (INTN)rc < 0, taking false branch.
870 if (EFI_ERROR(rc)) {
871 Print(L"Could not read \\EFI\\: %d\n", rc);
872 if (buffer) {
873 FreePool(buffer);
874 buffer = NULL;
875 }
876 uefi_call_wrapper(fh2->Close, 1, fh2);
877 uefi_call_wrapper(fh->Close, 1, fh);
878 return rc;
879 }
34. alias_transfer: Assigning: fi = buffer.
880 EFI_FILE_INFO *fi = buffer;
881
11. Condition !(fi->Attribute & 16), taking false branch.
21. Condition !(fi->Attribute & 16), taking false branch.
CID 182858 (#1-3 of 3): Explicit null dereferenced (FORWARD_NULL)35. var_deref_op: Dereferencing null pointer fi.
882 if (!(fi->Attribute & EFI_FILE_DIRECTORY)) {
883 FreePool(buffer);
884 buffer = NULL;
885 continue;
886 }
Because it doesn't know that when bs==0, fh2->Read() will return
EFI_BUFFER_TOO_SMALL and set bs to the size we need to allocate, so the
allocation path is always taken. Instead, handle our exit/error paths
directly there, and make the allocation path nonconditional.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 27, 2017
…mber.
Covscan noticed:
746static EFI_STATUS generate_hash (char *data, unsigned int datasize_in,
747 PE_COFF_LOADER_IMAGE_CONTEXT *context,
748 UINT8 *sha256hash, UINT8 *sha1hash)
749
750{
...
764
CID 182849 (#1 of 1): Unsigned compared against 0
(NO_EFFECT)unsigned_compare: This less-than-zero comparison of an
unsigned value is never true. datasize_in < 0U.
765 if (datasize_in < 0) {
766 perror(L"Invalid data size\n");
767 return EFI_INVALID_PARAMETER;
768 }
And I guess that's a fair point, but some of the callers take the size
as a signed integer. So we should be handling that on all the input
cases instead of getting that far.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 27, 2017
Covscan says:
455 if (IsFound) {
456 tpm_measure_variable(dbname, guid, CertSize, Cert->SignatureData);
457 return DATA_FOUND;
CID 182850 (#1 of 1): Structurally dead code (UNREACHABLE)unreachable: This code cannot be reached: drain_openssl_errors();.
458 drain_openssl_errors();
459 } else {
460 LogError(L"AuthenticodeVerify(): %d\n", IsFound);
461 }
And, well... woops.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Oct 27, 2017
"FixupData" in the edk2 tree is a log of the relocations that happened,
which is allocated by the "client" calling relocate, and written into
while it does relocations. Since we never allocate that log anywhere,
FixupData is always NULL, and so covscan says:
318 case EFI_IMAGE_REL_BASED_HIGH:
319 Fixup16 = (UINT16 *) Fixup;
320 *Fixup16 = (UINT16) (*Fixup16 + ((UINT16) ((UINT32) Adjust >> 16)));
null: At condition FixupData != NULL, the value of FixupData must be
NULL. dead_error_condition: The condition FixupData != NULL cannot
be true.
321 if (FixupData != NULL) {
CID 182859 (#1 of 4): Logically dead code (DEADCODE)dead_error_begin:
Execution cannot reach this statement: *((UINT16 *)FixupData) =
*F....
322 *(UINT16 *) FixupData = *Fixup16;
323 FixupData = FixupData + sizeof (UINT16);
324 }
325 break;
And it's right; all four occurrances are deadcode that never do anything
but confuse the reader.
Kill it with fire.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Mar 12, 2018
…an less. Because they don't believe code should be defensive against future changes, covscan believes: 520 out_free: 521 FreePool(dmp); CID 182824 (#1 of 1): Dereference before null check (REVERSE_INULL)check_after_deref: Null-checking entries suggests that it may be null, but it has already been dereferenced on all paths leading to the check. 522 if (entries) { 523 free_entries(entries, count); 524 FreePool(entries); 525 } 526 out_free_name: 527 FreePool(name); 528} Which is technically correct, but still kind of dumb. So this patch combines the two error out paths into just being out_free, so that the first path there is before entries is allocated. (It also initializes dmp to NULL and checks that before freeing it.) I also Lindent-ed that function. Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Mar 12, 2018
Covscan daftly claims:
288. var_compare_op: Comparing MokSB to null implies that MokSB might be null.
2330 if (MokSB) {
2331 menu_strings[i] = L"Change Secure Boot state";
2332 menu_item[i] = MOK_CHANGE_SB;
2333 i++;
2334 }
2335
...
2358 choice = console_select(perform_mok_mgmt, menu_strings, 0);
2359 if (choice < 0)
2360 goto out;
...
2362 switch (menu_item[choice]) {
...
2395 case MOK_CHANGE_SB:
CID 182841 (#1 of 1): Dereference after null check
(FORWARD_NULL)293. var_deref_model: Passing null pointer MokSB to
mok_sb_prompt, which dereferences it. [show details]
2396 efi_status = mok_sb_prompt(MokSB, MokSBSize);
Which is, of course, entirely false, beause for menu_item[choice] to be
MOK_CHANGE_SB, MokSB must be !NULL. And then:
252. Condition efi_status == 0, taking true branch.
2397 if (efi_status == EFI_SUCCESS)
2398 MokSB = NULL;
This guarantees it won't be in the list the next time through the loop.
This adds tests for NULLness before mok_sb_prompt(), just to make it
more clear to covscan what's going on.
Also do the same thing for all of:
MOK_CHANGE_SB
MOK_SET_PW
MOK_CHANGE_DB
MOK_ENROLL_MOKX
MOK_DELETE_MOKX
I also Lindent-ed everything I had to touch.
Three other minor errors are also fixed:
1) the loop in enter_mok_menu() leaked the menu allocations each time
through the loop
2) mok_sb_prompt(), mok_pw_prompt(), and mok_db_prompt() all call
FreePool() on their respective variables (MokSB, etc), and
check_mok_request() also calls FreePool() on these. This sounds
horrible, but it turns out it's not an issue, because they only free
them in their EFI_SUCCESS paths, and enter_mok_menu() resets the
system if any of the mok_XX_prompt() calls actually returned
EFI_SUCCESS, so we never get back to check_mok_request() for it to do
its FreePool() calls.
3) the loop in enter_mok_menu() winds up introducing a double free in
the call to free_menu(), but we also can't hit this bug, because all
the exit paths from the loop are "goto out" (or return error) rather
than actually exiting on the loop conditional.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Mar 12, 2018
Covscan believes the following:
782 if ((EFI_ERROR(rc) || !bootarchcsv) && bootcsv) {
783 EFI_FILE_HANDLE fh2;
784 rc = uefi_call_wrapper(fh->Open, 5, fh, &fh2,
785 bootcsv, EFI_FILE_READ_ONLY, 0);
786 if (EFI_ERROR(rc) || fh2 == NULL) {
787 Print(L"Couldn't open \\EFI\\%s\\%s: %d\n",
788 dirname, bootcsv, rc);
789 } else {
CID 182829 (#1 of 1): Unused value (UNUSED_VALUE)returned_value:
Assigning value from try_boot_csv(fh2, dirname, bootcsv) to rc here,
but that stored value is overwritten before it can be used.
790 rc = try_boot_csv(fh2, dirname, bootcsv);
791 uefi_call_wrapper(fh2->Close, 1, fh2);
792 }
793 }
value_overwrite: Overwriting previous write to rc with value 0UL.
794 rc = EFI_SUCCESS;
795
796 return rc;
797}
Which isn't untrue, we just don't happen to be using the return code for
anything, before we intentionally return success to our caller.
So that's annoying, but whatever. Just print the error as well.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Mar 12, 2018
Covscan sez:
720 FreePool(buffer);
assignment: Assigning: buffer = NULL.
721 buffer = NULL;
722
723 CHAR16 *bootcsv=NULL, *bootarchcsv=NULL;
724
725 bs = 0;
726 do {
727 bs = 0;
728 rc = uefi_call_wrapper(fh->Read, 3, fh, &bs, NULL);
729 if (EFI_ERROR(rc) && rc != EFI_BUFFER_TOO_SMALL) {
730 Print(L"Could not read \\EFI\\%s\\: %d\n", dirname, rc);
null: At condition buffer, the value of buffer must be NULL.
dead_error_condition: The condition buffer cannot be true.
731 if (buffer)
CID 182851 (#1 of 1): Logically dead code (DEADCODE)dead_error_line:
Execution cannot reach this statement: FreePool(buffer);.
732 FreePool(buffer);
733 return rc;
734 }
And it's right; buffer can never be non-NULL there. So just take that
out.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Mar 12, 2018
Covscan believes all this stuff:
852 bs = 0;
853 rc = uefi_call_wrapper(fh2->Read, 3, fh2, &bs, NULL);
7. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
8. Condition rc == 0, taking false branch.
15. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
16. Condition rc == 0, taking true branch.
17. Condition bs != 0, taking true branch.
30. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
31. Condition rc == 0, taking false branch.
854 if (rc == EFI_BUFFER_TOO_SMALL ||
855 (rc == EFI_SUCCESS && bs != 0)) {
856 buffer = AllocateZeroPool(bs);
18. Condition !buffer, taking false branch.
857 if (!buffer) {
858 Print(L"Could not allocate memory\n");
859 /* sure, this might work, why not? */
860 uefi_call_wrapper(fh2->Close, 1, fh2);
861 uefi_call_wrapper(fh->Close, 1, fh);
862 return EFI_OUT_OF_RESOURCES;
863 }
864
865 rc = uefi_call_wrapper(fh2->Read, 3, fh2, &bs, buffer);
866 }
9. Condition bs == 0, taking false branch.
19. Condition bs == 0, taking false branch.
32. Condition bs == 0, taking false branch.
867 if (bs == 0)
868 break;
869
10. Condition (INTN)rc < 0, taking false branch.
20. Condition (INTN)rc < 0, taking false branch.
33. Condition (INTN)rc < 0, taking false branch.
870 if (EFI_ERROR(rc)) {
871 Print(L"Could not read \\EFI\\: %d\n", rc);
872 if (buffer) {
873 FreePool(buffer);
874 buffer = NULL;
875 }
876 uefi_call_wrapper(fh2->Close, 1, fh2);
877 uefi_call_wrapper(fh->Close, 1, fh);
878 return rc;
879 }
34. alias_transfer: Assigning: fi = buffer.
880 EFI_FILE_INFO *fi = buffer;
881
11. Condition !(fi->Attribute & 16), taking false branch.
21. Condition !(fi->Attribute & 16), taking false branch.
CID 182858 (#1-3 of 3): Explicit null dereferenced (FORWARD_NULL)35. var_deref_op: Dereferencing null pointer fi.
882 if (!(fi->Attribute & EFI_FILE_DIRECTORY)) {
883 FreePool(buffer);
884 buffer = NULL;
885 continue;
886 }
Because it doesn't know that when bs==0, fh2->Read() will return
EFI_BUFFER_TOO_SMALL and set bs to the size we need to allocate, so the
allocation path is always taken. Instead, handle our exit/error paths
directly there, and make the allocation path nonconditional.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Mar 12, 2018
…mber.
Covscan noticed:
746static EFI_STATUS generate_hash (char *data, unsigned int datasize_in,
747 PE_COFF_LOADER_IMAGE_CONTEXT *context,
748 UINT8 *sha256hash, UINT8 *sha1hash)
749
750{
...
764
CID 182849 (#1 of 1): Unsigned compared against 0
(NO_EFFECT)unsigned_compare: This less-than-zero comparison of an
unsigned value is never true. datasize_in < 0U.
765 if (datasize_in < 0) {
766 perror(L"Invalid data size\n");
767 return EFI_INVALID_PARAMETER;
768 }
And I guess that's a fair point, but some of the callers take the size
as a signed integer. So we should be handling that on all the input
cases instead of getting that far.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Mar 12, 2018
Covscan says:
455 if (IsFound) {
456 tpm_measure_variable(dbname, guid, CertSize, Cert->SignatureData);
457 return DATA_FOUND;
CID 182850 (#1 of 1): Structurally dead code (UNREACHABLE)unreachable: This code cannot be reached: drain_openssl_errors();.
458 drain_openssl_errors();
459 } else {
460 LogError(L"AuthenticodeVerify(): %d\n", IsFound);
461 }
And, well... woops.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Mar 12, 2018
"FixupData" in the edk2 tree is a log of the relocations that happened,
which is allocated by the "client" calling relocate, and written into
while it does relocations. Since we never allocate that log anywhere,
FixupData is always NULL, and so covscan says:
318 case EFI_IMAGE_REL_BASED_HIGH:
319 Fixup16 = (UINT16 *) Fixup;
320 *Fixup16 = (UINT16) (*Fixup16 + ((UINT16) ((UINT32) Adjust >> 16)));
null: At condition FixupData != NULL, the value of FixupData must be
NULL. dead_error_condition: The condition FixupData != NULL cannot
be true.
321 if (FixupData != NULL) {
CID 182859 (#1 of 4): Logically dead code (DEADCODE)dead_error_begin:
Execution cannot reach this statement: *((UINT16 *)FixupData) =
*F....
322 *(UINT16 *) FixupData = *Fixup16;
323 FixupData = FixupData + sizeof (UINT16);
324 }
325 break;
And it's right; all four occurrances are deadcode that never do anything
but confuse the reader.
Kill it with fire.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Mar 12, 2018
…an less. Because they don't believe code should be defensive against future changes, covscan believes: 520 out_free: 521 FreePool(dmp); CID 182824 (#1 of 1): Dereference before null check (REVERSE_INULL)check_after_deref: Null-checking entries suggests that it may be null, but it has already been dereferenced on all paths leading to the check. 522 if (entries) { 523 free_entries(entries, count); 524 FreePool(entries); 525 } 526 out_free_name: 527 FreePool(name); 528} Which is technically correct, but still kind of dumb. So this patch combines the two error out paths into just being out_free, so that the first path there is before entries is allocated. (It also initializes dmp to NULL and checks that before freeing it.) I also Lindent-ed that function. Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Mar 12, 2018
Covscan daftly claims:
288. var_compare_op: Comparing MokSB to null implies that MokSB might be null.
2330 if (MokSB) {
2331 menu_strings[i] = L"Change Secure Boot state";
2332 menu_item[i] = MOK_CHANGE_SB;
2333 i++;
2334 }
2335
...
2358 choice = console_select(perform_mok_mgmt, menu_strings, 0);
2359 if (choice < 0)
2360 goto out;
...
2362 switch (menu_item[choice]) {
...
2395 case MOK_CHANGE_SB:
CID 182841 (#1 of 1): Dereference after null check
(FORWARD_NULL)293. var_deref_model: Passing null pointer MokSB to
mok_sb_prompt, which dereferences it. [show details]
2396 efi_status = mok_sb_prompt(MokSB, MokSBSize);
Which is, of course, entirely false, beause for menu_item[choice] to be
MOK_CHANGE_SB, MokSB must be !NULL. And then:
252. Condition efi_status == 0, taking true branch.
2397 if (efi_status == EFI_SUCCESS)
2398 MokSB = NULL;
This guarantees it won't be in the list the next time through the loop.
This adds tests for NULLness before mok_sb_prompt(), just to make it
more clear to covscan what's going on.
Also do the same thing for all of:
MOK_CHANGE_SB
MOK_SET_PW
MOK_CHANGE_DB
MOK_ENROLL_MOKX
MOK_DELETE_MOKX
I also Lindent-ed everything I had to touch.
Three other minor errors are also fixed:
1) the loop in enter_mok_menu() leaked the menu allocations each time
through the loop
2) mok_sb_prompt(), mok_pw_prompt(), and mok_db_prompt() all call
FreePool() on their respective variables (MokSB, etc), and
check_mok_request() also calls FreePool() on these. This sounds
horrible, but it turns out it's not an issue, because they only free
them in their EFI_SUCCESS paths, and enter_mok_menu() resets the
system if any of the mok_XX_prompt() calls actually returned
EFI_SUCCESS, so we never get back to check_mok_request() for it to do
its FreePool() calls.
3) the loop in enter_mok_menu() winds up introducing a double free in
the call to free_menu(), but we also can't hit this bug, because all
the exit paths from the loop are "goto out" (or return error) rather
than actually exiting on the loop conditional.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Mar 12, 2018
Covscan believes the following:
782 if ((EFI_ERROR(rc) || !bootarchcsv) && bootcsv) {
783 EFI_FILE_HANDLE fh2;
784 rc = uefi_call_wrapper(fh->Open, 5, fh, &fh2,
785 bootcsv, EFI_FILE_READ_ONLY, 0);
786 if (EFI_ERROR(rc) || fh2 == NULL) {
787 Print(L"Couldn't open \\EFI\\%s\\%s: %d\n",
788 dirname, bootcsv, rc);
789 } else {
CID 182829 (#1 of 1): Unused value (UNUSED_VALUE)returned_value:
Assigning value from try_boot_csv(fh2, dirname, bootcsv) to rc here,
but that stored value is overwritten before it can be used.
790 rc = try_boot_csv(fh2, dirname, bootcsv);
791 uefi_call_wrapper(fh2->Close, 1, fh2);
792 }
793 }
value_overwrite: Overwriting previous write to rc with value 0UL.
794 rc = EFI_SUCCESS;
795
796 return rc;
797}
Which isn't untrue, we just don't happen to be using the return code for
anything, before we intentionally return success to our caller.
So that's annoying, but whatever. Just print the error as well.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Mar 12, 2018
Covscan sez:
720 FreePool(buffer);
assignment: Assigning: buffer = NULL.
721 buffer = NULL;
722
723 CHAR16 *bootcsv=NULL, *bootarchcsv=NULL;
724
725 bs = 0;
726 do {
727 bs = 0;
728 rc = uefi_call_wrapper(fh->Read, 3, fh, &bs, NULL);
729 if (EFI_ERROR(rc) && rc != EFI_BUFFER_TOO_SMALL) {
730 Print(L"Could not read \\EFI\\%s\\: %d\n", dirname, rc);
null: At condition buffer, the value of buffer must be NULL.
dead_error_condition: The condition buffer cannot be true.
731 if (buffer)
CID 182851 (#1 of 1): Logically dead code (DEADCODE)dead_error_line:
Execution cannot reach this statement: FreePool(buffer);.
732 FreePool(buffer);
733 return rc;
734 }
And it's right; buffer can never be non-NULL there. So just take that
out.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Mar 12, 2018
Covscan believes all this stuff:
852 bs = 0;
853 rc = uefi_call_wrapper(fh2->Read, 3, fh2, &bs, NULL);
7. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
8. Condition rc == 0, taking false branch.
15. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
16. Condition rc == 0, taking true branch.
17. Condition bs != 0, taking true branch.
30. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
31. Condition rc == 0, taking false branch.
854 if (rc == EFI_BUFFER_TOO_SMALL ||
855 (rc == EFI_SUCCESS && bs != 0)) {
856 buffer = AllocateZeroPool(bs);
18. Condition !buffer, taking false branch.
857 if (!buffer) {
858 Print(L"Could not allocate memory\n");
859 /* sure, this might work, why not? */
860 uefi_call_wrapper(fh2->Close, 1, fh2);
861 uefi_call_wrapper(fh->Close, 1, fh);
862 return EFI_OUT_OF_RESOURCES;
863 }
864
865 rc = uefi_call_wrapper(fh2->Read, 3, fh2, &bs, buffer);
866 }
9. Condition bs == 0, taking false branch.
19. Condition bs == 0, taking false branch.
32. Condition bs == 0, taking false branch.
867 if (bs == 0)
868 break;
869
10. Condition (INTN)rc < 0, taking false branch.
20. Condition (INTN)rc < 0, taking false branch.
33. Condition (INTN)rc < 0, taking false branch.
870 if (EFI_ERROR(rc)) {
871 Print(L"Could not read \\EFI\\: %d\n", rc);
872 if (buffer) {
873 FreePool(buffer);
874 buffer = NULL;
875 }
876 uefi_call_wrapper(fh2->Close, 1, fh2);
877 uefi_call_wrapper(fh->Close, 1, fh);
878 return rc;
879 }
34. alias_transfer: Assigning: fi = buffer.
880 EFI_FILE_INFO *fi = buffer;
881
11. Condition !(fi->Attribute & 16), taking false branch.
21. Condition !(fi->Attribute & 16), taking false branch.
CID 182858 (#1-3 of 3): Explicit null dereferenced (FORWARD_NULL)35. var_deref_op: Dereferencing null pointer fi.
882 if (!(fi->Attribute & EFI_FILE_DIRECTORY)) {
883 FreePool(buffer);
884 buffer = NULL;
885 continue;
886 }
Because it doesn't know that when bs==0, fh2->Read() will return
EFI_BUFFER_TOO_SMALL and set bs to the size we need to allocate, so the
allocation path is always taken. Instead, handle our exit/error paths
directly there, and make the allocation path nonconditional.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Mar 12, 2018
…mber.
Covscan noticed:
746static EFI_STATUS generate_hash (char *data, unsigned int datasize_in,
747 PE_COFF_LOADER_IMAGE_CONTEXT *context,
748 UINT8 *sha256hash, UINT8 *sha1hash)
749
750{
...
764
CID 182849 (#1 of 1): Unsigned compared against 0
(NO_EFFECT)unsigned_compare: This less-than-zero comparison of an
unsigned value is never true. datasize_in < 0U.
765 if (datasize_in < 0) {
766 perror(L"Invalid data size\n");
767 return EFI_INVALID_PARAMETER;
768 }
And I guess that's a fair point, but some of the callers take the size
as a signed integer. So we should be handling that on all the input
cases instead of getting that far.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Mar 12, 2018
Covscan says:
455 if (IsFound) {
456 tpm_measure_variable(dbname, guid, CertSize, Cert->SignatureData);
457 return DATA_FOUND;
CID 182850 (#1 of 1): Structurally dead code (UNREACHABLE)unreachable: This code cannot be reached: drain_openssl_errors();.
458 drain_openssl_errors();
459 } else {
460 LogError(L"AuthenticodeVerify(): %d\n", IsFound);
461 }
And, well... woops.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
referenced
this pull request
in vathpela/mallory
Mar 12, 2018
"FixupData" in the edk2 tree is a log of the relocations that happened,
which is allocated by the "client" calling relocate, and written into
while it does relocations. Since we never allocate that log anywhere,
FixupData is always NULL, and so covscan says:
318 case EFI_IMAGE_REL_BASED_HIGH:
319 Fixup16 = (UINT16 *) Fixup;
320 *Fixup16 = (UINT16) (*Fixup16 + ((UINT16) ((UINT32) Adjust >> 16)));
null: At condition FixupData != NULL, the value of FixupData must be
NULL. dead_error_condition: The condition FixupData != NULL cannot
be true.
321 if (FixupData != NULL) {
CID 182859 (#1 of 4): Logically dead code (DEADCODE)dead_error_begin:
Execution cannot reach this statement: *((UINT16 *)FixupData) =
*F....
322 *(UINT16 *) FixupData = *Fixup16;
323 FixupData = FixupData + sizeof (UINT16);
324 }
325 break;
And it's right; all four occurrances are deadcode that never do anything
but confuse the reader.
Kill it with fire.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
added a commit
that referenced
this pull request
Mar 12, 2018
…an less. Because they don't believe code should be defensive against future changes, covscan believes: 520 out_free: 521 FreePool(dmp); CID 182824 (#1 of 1): Dereference before null check (REVERSE_INULL)check_after_deref: Null-checking entries suggests that it may be null, but it has already been dereferenced on all paths leading to the check. 522 if (entries) { 523 free_entries(entries, count); 524 FreePool(entries); 525 } 526 out_free_name: 527 FreePool(name); 528} Which is technically correct, but still kind of dumb. So this patch combines the two error out paths into just being out_free, so that the first path there is before entries is allocated. (It also initializes dmp to NULL and checks that before freeing it.) I also Lindent-ed that function. Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
added a commit
that referenced
this pull request
Mar 12, 2018
Covscan daftly claims:
288. var_compare_op: Comparing MokSB to null implies that MokSB might be null.
2330 if (MokSB) {
2331 menu_strings[i] = L"Change Secure Boot state";
2332 menu_item[i] = MOK_CHANGE_SB;
2333 i++;
2334 }
2335
...
2358 choice = console_select(perform_mok_mgmt, menu_strings, 0);
2359 if (choice < 0)
2360 goto out;
...
2362 switch (menu_item[choice]) {
...
2395 case MOK_CHANGE_SB:
CID 182841 (#1 of 1): Dereference after null check
(FORWARD_NULL)293. var_deref_model: Passing null pointer MokSB to
mok_sb_prompt, which dereferences it. [show details]
2396 efi_status = mok_sb_prompt(MokSB, MokSBSize);
Which is, of course, entirely false, beause for menu_item[choice] to be
MOK_CHANGE_SB, MokSB must be !NULL. And then:
252. Condition efi_status == 0, taking true branch.
2397 if (efi_status == EFI_SUCCESS)
2398 MokSB = NULL;
This guarantees it won't be in the list the next time through the loop.
This adds tests for NULLness before mok_sb_prompt(), just to make it
more clear to covscan what's going on.
Also do the same thing for all of:
MOK_CHANGE_SB
MOK_SET_PW
MOK_CHANGE_DB
MOK_ENROLL_MOKX
MOK_DELETE_MOKX
I also Lindent-ed everything I had to touch.
Three other minor errors are also fixed:
1) the loop in enter_mok_menu() leaked the menu allocations each time
through the loop
2) mok_sb_prompt(), mok_pw_prompt(), and mok_db_prompt() all call
FreePool() on their respective variables (MokSB, etc), and
check_mok_request() also calls FreePool() on these. This sounds
horrible, but it turns out it's not an issue, because they only free
them in their EFI_SUCCESS paths, and enter_mok_menu() resets the
system if any of the mok_XX_prompt() calls actually returned
EFI_SUCCESS, so we never get back to check_mok_request() for it to do
its FreePool() calls.
3) the loop in enter_mok_menu() winds up introducing a double free in
the call to free_menu(), but we also can't hit this bug, because all
the exit paths from the loop are "goto out" (or return error) rather
than actually exiting on the loop conditional.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
added a commit
that referenced
this pull request
Mar 12, 2018
Covscan believes the following:
782 if ((EFI_ERROR(rc) || !bootarchcsv) && bootcsv) {
783 EFI_FILE_HANDLE fh2;
784 rc = uefi_call_wrapper(fh->Open, 5, fh, &fh2,
785 bootcsv, EFI_FILE_READ_ONLY, 0);
786 if (EFI_ERROR(rc) || fh2 == NULL) {
787 Print(L"Couldn't open \\EFI\\%s\\%s: %d\n",
788 dirname, bootcsv, rc);
789 } else {
CID 182829 (#1 of 1): Unused value (UNUSED_VALUE)returned_value:
Assigning value from try_boot_csv(fh2, dirname, bootcsv) to rc here,
but that stored value is overwritten before it can be used.
790 rc = try_boot_csv(fh2, dirname, bootcsv);
791 uefi_call_wrapper(fh2->Close, 1, fh2);
792 }
793 }
value_overwrite: Overwriting previous write to rc with value 0UL.
794 rc = EFI_SUCCESS;
795
796 return rc;
797}
Which isn't untrue, we just don't happen to be using the return code for
anything, before we intentionally return success to our caller.
So that's annoying, but whatever. Just print the error as well.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
added a commit
that referenced
this pull request
Mar 12, 2018
Covscan sez:
720 FreePool(buffer);
assignment: Assigning: buffer = NULL.
721 buffer = NULL;
722
723 CHAR16 *bootcsv=NULL, *bootarchcsv=NULL;
724
725 bs = 0;
726 do {
727 bs = 0;
728 rc = uefi_call_wrapper(fh->Read, 3, fh, &bs, NULL);
729 if (EFI_ERROR(rc) && rc != EFI_BUFFER_TOO_SMALL) {
730 Print(L"Could not read \\EFI\\%s\\: %d\n", dirname, rc);
null: At condition buffer, the value of buffer must be NULL.
dead_error_condition: The condition buffer cannot be true.
731 if (buffer)
CID 182851 (#1 of 1): Logically dead code (DEADCODE)dead_error_line:
Execution cannot reach this statement: FreePool(buffer);.
732 FreePool(buffer);
733 return rc;
734 }
And it's right; buffer can never be non-NULL there. So just take that
out.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
added a commit
that referenced
this pull request
Mar 12, 2018
Covscan believes all this stuff:
852 bs = 0;
853 rc = uefi_call_wrapper(fh2->Read, 3, fh2, &bs, NULL);
7. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
8. Condition rc == 0, taking false branch.
15. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
16. Condition rc == 0, taking true branch.
17. Condition bs != 0, taking true branch.
30. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
31. Condition rc == 0, taking false branch.
854 if (rc == EFI_BUFFER_TOO_SMALL ||
855 (rc == EFI_SUCCESS && bs != 0)) {
856 buffer = AllocateZeroPool(bs);
18. Condition !buffer, taking false branch.
857 if (!buffer) {
858 Print(L"Could not allocate memory\n");
859 /* sure, this might work, why not? */
860 uefi_call_wrapper(fh2->Close, 1, fh2);
861 uefi_call_wrapper(fh->Close, 1, fh);
862 return EFI_OUT_OF_RESOURCES;
863 }
864
865 rc = uefi_call_wrapper(fh2->Read, 3, fh2, &bs, buffer);
866 }
9. Condition bs == 0, taking false branch.
19. Condition bs == 0, taking false branch.
32. Condition bs == 0, taking false branch.
867 if (bs == 0)
868 break;
869
10. Condition (INTN)rc < 0, taking false branch.
20. Condition (INTN)rc < 0, taking false branch.
33. Condition (INTN)rc < 0, taking false branch.
870 if (EFI_ERROR(rc)) {
871 Print(L"Could not read \\EFI\\: %d\n", rc);
872 if (buffer) {
873 FreePool(buffer);
874 buffer = NULL;
875 }
876 uefi_call_wrapper(fh2->Close, 1, fh2);
877 uefi_call_wrapper(fh->Close, 1, fh);
878 return rc;
879 }
34. alias_transfer: Assigning: fi = buffer.
880 EFI_FILE_INFO *fi = buffer;
881
11. Condition !(fi->Attribute & 16), taking false branch.
21. Condition !(fi->Attribute & 16), taking false branch.
CID 182858 (#1-3 of 3): Explicit null dereferenced (FORWARD_NULL)35. var_deref_op: Dereferencing null pointer fi.
882 if (!(fi->Attribute & EFI_FILE_DIRECTORY)) {
883 FreePool(buffer);
884 buffer = NULL;
885 continue;
886 }
Because it doesn't know that when bs==0, fh2->Read() will return
EFI_BUFFER_TOO_SMALL and set bs to the size we need to allocate, so the
allocation path is always taken. Instead, handle our exit/error paths
directly there, and make the allocation path nonconditional.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
added a commit
that referenced
this pull request
Mar 12, 2018
…mber.
Covscan noticed:
746static EFI_STATUS generate_hash (char *data, unsigned int datasize_in,
747 PE_COFF_LOADER_IMAGE_CONTEXT *context,
748 UINT8 *sha256hash, UINT8 *sha1hash)
749
750{
...
764
CID 182849 (#1 of 1): Unsigned compared against 0
(NO_EFFECT)unsigned_compare: This less-than-zero comparison of an
unsigned value is never true. datasize_in < 0U.
765 if (datasize_in < 0) {
766 perror(L"Invalid data size\n");
767 return EFI_INVALID_PARAMETER;
768 }
And I guess that's a fair point, but some of the callers take the size
as a signed integer. So we should be handling that on all the input
cases instead of getting that far.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
added a commit
that referenced
this pull request
Mar 12, 2018
Covscan says:
455 if (IsFound) {
456 tpm_measure_variable(dbname, guid, CertSize, Cert->SignatureData);
457 return DATA_FOUND;
CID 182850 (#1 of 1): Structurally dead code (UNREACHABLE)unreachable: This code cannot be reached: drain_openssl_errors();.
458 drain_openssl_errors();
459 } else {
460 LogError(L"AuthenticodeVerify(): %d\n", IsFound);
461 }
And, well... woops.
Signed-off-by: Peter Jones <pjones@redhat.com>
vathpela
added a commit
that referenced
this pull request
Mar 12, 2018
"FixupData" in the edk2 tree is a log of the relocations that happened,
which is allocated by the "client" calling relocate, and written into
while it does relocations. Since we never allocate that log anywhere,
FixupData is always NULL, and so covscan says:
318 case EFI_IMAGE_REL_BASED_HIGH:
319 Fixup16 = (UINT16 *) Fixup;
320 *Fixup16 = (UINT16) (*Fixup16 + ((UINT16) ((UINT32) Adjust >> 16)));
null: At condition FixupData != NULL, the value of FixupData must be
NULL. dead_error_condition: The condition FixupData != NULL cannot
be true.
321 if (FixupData != NULL) {
CID 182859 (#1 of 4): Logically dead code (DEADCODE)dead_error_begin:
Execution cannot reach this statement: *((UINT16 *)FixupData) =
*F....
322 *(UINT16 *) FixupData = *Fixup16;
323 FixupData = FixupData + sizeof (UINT16);
324 }
325 break;
And it's right; all four occurrances are deadcode that never do anything
but confuse the reader.
Kill it with fire.
Signed-off-by: Peter Jones <pjones@redhat.com>
martinezjavier
pushed a commit
that referenced
this pull request
Mar 12, 2021
For some reason when we try to ever use the builtins, even with the symbol there as a fallback, something goes horribly wrong somewhere around here: | (gdb) bt | #0 strcmp (s1=0x7d492359 "MD5", s2=0x7d492359 "MD5") at include/system/string.h:57 | #1 0x000000007d460419 in getrn (lh=lh@entry=0x7e081318, data=data@entry=0x7e084398, rhash=rhash@entry=0x7f7c9268) at crypto/lhash/lhash.c:415 | #2 0x000000007d46076e in lh_insert (lh=0x7e081318, data=data@entry=0x7e084398) at crypto/lhash/lhash.c:188 | #3 0x000000007d43e027 in OBJ_NAME_add (name=name@entry=0x7d492359 "MD5", type=type@entry=1, data=data@entry=0x7d4ad3a0 <md5_md> "\004") at crypto/objects/o_names.c:202 As much as I love a Sisyphean challenge, in the interest of not having bugs or time, this patch changes it to just not use them for anything other than guaranteeing our implementations have the exact same types as you would expect. Signed-off-by: Peter Jones <pjones@redhat.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.