Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pe: only process RelocDir->Size of reloc section #562

Closed
wants to merge 1 commit into from
Closed

pe: only process RelocDir->Size of reloc section #562

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions pe.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ relocate_coff (PE_COFF_LOADER_IMAGE_CONTEXT *context,
/* RelocBaseEnd here is the address of the first entry /past/ the
* table. */
RelocBaseEnd = ImageAddress(orig, size, Section->PointerToRawData +
Section->Misc.VirtualSize);
context->RelocDir->Size);
Copy link
Contributor Author

@mikebeaton mikebeaton Apr 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the test is modifed to accept the .reloc section, we also have to process only the used size.


if (!RelocBase && !RelocBaseEnd)
return EFI_SUCCESS;
Expand Down Expand Up @@ -741,7 +741,7 @@ read_header(void *data, unsigned int datasize,
context->NumberOfSections = PEHdr->Pe32.FileHeader.NumberOfSections;

if (EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES < context->NumberOfRvaAndSizes) {
perror(L"Image header too small\n");
perror(L"Image header too large\n");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated really - apologies if I'm misreading this? (Could make separate tiny PR; or remove, if the wording change does not seem right. Tyvm!)

return EFI_UNSUPPORTED;
}

Expand Down Expand Up @@ -1277,8 +1277,11 @@ handle_image (void *data, unsigned int datasize,
Section->Misc.VirtualSize &&
base && end &&
RelocBase == base &&
RelocBaseEnd == end) {
RelocBaseEnd <= end) {
Copy link
Contributor Author

@mikebeaton mikebeaton Apr 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this change, the file loads but crashes (since shim will attempt to use the file as if there is no reloc section).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid crash, may I suggest we return error code to caller if RelocSection is not found finally. And let restore_loaded_image() to recover system.

image

Copy link
Contributor Author

@mikebeaton mikebeaton Jun 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Thanks for this input.

Is BS->FreePages correct, here? The various other (pre-existing) return EFI_UNSUPPORTED nearby (and in the same allocation context, I believe), do not do it.

Then on to the general idea, yes, in the current version of this PR the code continues if no reloc section is finally found (because this is what the code previously did), but the correct place to decide not to do so (which I tend to agree would be an improvement) would be around line 1331:

shim/pe.c

Line 1331 in 0640e13

if (context.RelocDir->Size && RelocSection) {

If you (possibly) meant the suggestion as an alternative fix - i.e. to avoid my crash - then I think that misunderstands the nature of the PR - the file should load and run (not abort, and also - ofc - not crash) and this PR allows it to load.

RelocSection = Section;
} else {
perror(L"Relocation section is invalid \n");
return EFI_UNSUPPORTED;
Comment on lines +1283 to +1284
Copy link
Contributor Author

@mikebeaton mikebeaton Apr 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest in the case where a reloc section is found but rejected, aborting with an error such as this is helpful (unless there are known cases where ignore and continue behaviour is required?).

}
}

Expand Down