Skip to content

Commit

Permalink
Correctly free memory allocated in handle_image()
Browse files Browse the repository at this point in the history
Currently pe's handle_image() function has two related issues, which are
a memory leak in most error paths and an incorrect FreePool() call in
some error paths.

This patch adds the correct FreePages() calls to most error paths, and
switches the FreePool() call to match them.

[commit message re-written to be more informative by pjones]

Signed-off-by: Dennis Tseng <dennis.tseng@suse.com>
  • Loading branch information
dennis-tseng99 authored and vathpela committed Jul 19, 2023
1 parent fd43eda commit 1e985a3
Showing 1 changed file with 11 additions and 2 deletions.
13 changes: 11 additions & 2 deletions pe.c
Original file line number Diff line number Diff line change
Expand Up @@ -747,6 +747,7 @@ handle_image (void *data, unsigned int datasize,
(Section->Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE) &&
(mok_policy & MOK_POLICY_REQUIRE_NX)) {
perror(L"Section %d is writable and executable\n", i);
BS->FreePages(*alloc_address, *alloc_pages);
return EFI_UNSUPPORTED;
}

Expand All @@ -772,6 +773,7 @@ handle_image (void *data, unsigned int datasize,
if (CompareMem(Section->Name, ".reloc\0\0", 8) == 0) {
if (RelocSection) {
perror(L"Image has multiple relocation sections\n");
BS->FreePages(*alloc_address, *alloc_pages);
return EFI_UNSUPPORTED;
}
/* If it has nonzero sizes, and our bounds check
Expand All @@ -785,6 +787,7 @@ handle_image (void *data, unsigned int datasize,
RelocSection = Section;
} else {
perror(L"Relocation section is invalid \n");
BS->FreePages(*alloc_address, *alloc_pages);
return EFI_UNSUPPORTED;
}
}
Expand All @@ -795,17 +798,20 @@ handle_image (void *data, unsigned int datasize,

if (!base) {
perror(L"Section %d has invalid base address\n", i);
BS->FreePages(*alloc_address, *alloc_pages);
return EFI_UNSUPPORTED;
}
if (!end) {
perror(L"Section %d has zero size\n", i);
BS->FreePages(*alloc_address, *alloc_pages);
return EFI_UNSUPPORTED;
}

if (!(Section->Characteristics & EFI_IMAGE_SCN_CNT_UNINITIALIZED_DATA) &&
(Section->VirtualAddress < context.SizeOfHeaders ||
Section->PointerToRawData < context.SizeOfHeaders)) {
perror(L"Section %d is inside image headers\n", i);
BS->FreePages(*alloc_address, *alloc_pages);
return EFI_UNSUPPORTED;
}

Expand All @@ -814,6 +820,7 @@ handle_image (void *data, unsigned int datasize,
} else {
if (Section->PointerToRawData < context.SizeOfHeaders) {
perror(L"Section %d is inside image headers\n", i);
BS->FreePages(*alloc_address, *alloc_pages);
return EFI_UNSUPPORTED;
}

Expand All @@ -831,7 +838,7 @@ handle_image (void *data, unsigned int datasize,

if (context.NumberOfRvaAndSizes <= EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC) {
perror(L"Image has no relocation entry\n");
FreePool(buffer);
BS->FreePages(*alloc_address, *alloc_pages);
return EFI_UNSUPPORTED;
}

Expand All @@ -844,7 +851,7 @@ handle_image (void *data, unsigned int datasize,

if (EFI_ERROR(efi_status)) {
perror(L"Relocation failed: %r\n", efi_status);
FreePool(buffer);
BS->FreePages(*alloc_address, *alloc_pages);
return efi_status;
}
}
Expand Down Expand Up @@ -910,10 +917,12 @@ handle_image (void *data, unsigned int datasize,

if (!found_entry_point) {
perror(L"Entry point is not within sections\n");
BS->FreePages(*alloc_address, *alloc_pages);
return EFI_UNSUPPORTED;
}
if (found_entry_point > 1) {
perror(L"%d sections contain entry point\n", found_entry_point);
BS->FreePages(*alloc_address, *alloc_pages);
return EFI_UNSUPPORTED;
}

Expand Down

0 comments on commit 1e985a3

Please sign in to comment.