From: Laszlo Ersek <lersek@redhat.com>
To: Neo Hsueh <hong-chihx.hsueh@intel.com>, edk2-devel@lists.01.org
Cc: Michael D Kinney <michael.d.kinney@intel.com>,
Liming Gao <liming.gao@intel.com>,
Dandan Bi <dandan.bi@intel.com>
Subject: Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info is invalid
Date: Wed, 30 Jan 2019 15:42:09 +0100 [thread overview]
Message-ID: <fa042de2-2bbf-9971-5eac-0abf6755dc51@redhat.com> (raw)
In-Reply-To: <20190129185048.17500-1-hong-chihx.hsueh@intel.com>
On 01/29/19 19:50, Neo Hsueh wrote:
> Skip runtime relocation for PE images that provide invalid relocation
> infomation (ex: RelocDir->Size = 0) to fix a hang observed while booting
> Windows.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Neo Hsueh <hong-chihx.hsueh@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
> MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 28 ++++++++++++++++++----------
> 1 file changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> index 1bd079ad6a..3a46e626e4 100644
> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> @@ -1002,7 +1002,7 @@ PeCoffLoaderRelocateImage (
> RelocDir->VirtualAddress + RelocDir->Size - 1,
> TeStrippedOffset
> );
> - if (RelocBase == NULL || RelocBaseEnd == NULL || RelocBaseEnd < RelocBase) {
> + if (RelocBase == NULL || RelocBaseEnd == NULL || (UINTN) RelocBaseEnd < (UINTN) RelocBase) {
> ImageContext->ImageError = IMAGE_ERROR_FAILED_RELOCATION;
> return RETURN_LOAD_ERROR;
> }
> @@ -1022,7 +1022,7 @@ PeCoffLoaderRelocateImage (
> // Run the relocation information and apply the fixups
> //
> FixupData = ImageContext->FixupData;
> - while (RelocBase < RelocBaseEnd) {
> + while ((UINTN) RelocBase < (UINTN) RelocBaseEnd) {
>
> Reloc = (UINT16 *) ((CHAR8 *) RelocBase + sizeof (EFI_IMAGE_BASE_RELOCATION));
> //
> @@ -1051,7 +1051,7 @@ PeCoffLoaderRelocateImage (
> //
> // Run this relocation record
> //
> - while (Reloc < RelocEnd) {
> + while ((UINTN) Reloc < (UINTN) RelocEnd) {
> Fixup = PeCoffLoaderImageAddress (ImageContext, RelocBase->VirtualAddress + (*Reloc & 0xFFF), TeStrippedOffset);
> if (Fixup == NULL) {
> ImageContext->ImageError = IMAGE_ERROR_FAILED_RELOCATION;
> @@ -1741,11 +1741,19 @@ PeCoffLoaderRelocateImageForRuntime (
> //
> if (NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC) {
> RelocDir = DataDirectory + EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC;
> - RelocBase = (EFI_IMAGE_BASE_RELOCATION *) PeCoffLoaderImageAddress (&ImageContext, RelocDir->VirtualAddress, 0);
> - RelocBaseEnd = (EFI_IMAGE_BASE_RELOCATION *) PeCoffLoaderImageAddress (&ImageContext,
> - RelocDir->VirtualAddress + RelocDir->Size - 1,
> - 0
> - );
> + if ((RelocDir != NULL) && (RelocDir->Size > 0)) {
> + RelocBase = (EFI_IMAGE_BASE_RELOCATION *) PeCoffLoaderImageAddress (&ImageContext, RelocDir->VirtualAddress, 0);
> + RelocBaseEnd = (EFI_IMAGE_BASE_RELOCATION *) PeCoffLoaderImageAddress (&ImageContext,
> + RelocDir->VirtualAddress + RelocDir->Size - 1,
> + 0
> + );
> + }
If the above check fails, then RelocBase and RelocBaseEnd will not be
assigned, and the expressions below will check the previous values of
those variables. What guarantees that those values are valid / not
indeterminate?
Thanks
Laszlo
> + if (RelocBase == NULL || RelocBaseEnd == NULL || (UINTN) RelocBaseEnd < (UINTN) RelocBase) {
> + //
> + // relocation block is not valid, just return
> + //
> + return;
> + }
> } else {
> //
> // Cannot find relocations, cannot continue to relocate the image, ASSERT for this invalid image.
> @@ -1769,7 +1777,7 @@ PeCoffLoaderRelocateImageForRuntime (
> //
> FixupData = RelocationData;
> RelocBaseOrig = RelocBase;
> - while (RelocBase < RelocBaseEnd) {
> + while ((UINTN) RelocBase < (UINTN) RelocBaseEnd) {
> //
> // Add check for RelocBase->SizeOfBlock field.
> //
> @@ -1794,7 +1802,7 @@ PeCoffLoaderRelocateImageForRuntime (
> //
> // Run this relocation record
> //
> - while (Reloc < RelocEnd) {
> + while ((UINTN) Reloc < (UINTN) RelocEnd) {
>
> Fixup = PeCoffLoaderImageAddress (&ImageContext, RelocBase->VirtualAddress + (*Reloc & 0xFFF), 0);
> if (Fixup == NULL) {
>
next prev parent reply other threads:[~2019-01-30 14:42 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-29 18:50 [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info is invalid Neo Hsueh
2019-01-30 14:42 ` Laszlo Ersek [this message]
-- strict thread matches above, loose matches on Subject: below --
2019-01-30 1:19 Neo Hsueh
2019-01-30 14:45 ` Laszlo Ersek
2019-01-31 0:27 ` Bi, Dandan
2019-01-31 3:30 ` Gao, Liming
2019-01-31 3:38 ` Gao, Liming
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fa042de2-2bbf-9971-5eac-0abf6755dc51@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox