* [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info is invalid
@ 2019-01-30 1:19 Neo Hsueh
2019-01-30 14:45 ` Laszlo Ersek
2019-01-31 0:27 ` Bi, Dandan
0 siblings, 2 replies; 7+ messages in thread
From: Neo Hsueh @ 2019-01-30 1:19 UTC (permalink / raw)
To: edk2-devel; +Cc: Michael D Kinney, Liming Gao, Dandan Bi, Laszlo Ersek
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 | 30 ++++++++++++++++++++----------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
index 1bd079ad6a..e2c62e1932 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;
@@ -1739,13 +1739,23 @@ PeCoffLoaderRelocateImageForRuntime (
// is present in the image. You have to check the NumberOfRvaAndSizes in
// the optional header to verify a desired directory entry is there.
//
+ RelocBase = NULL;
+ RelocBaseEnd = NULL;
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 (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 +1779,7 @@ PeCoffLoaderRelocateImageForRuntime (
//
FixupData = RelocationData;
RelocBaseOrig = RelocBase;
- while (RelocBase < RelocBaseEnd) {
+ while ((UINTN) RelocBase < (UINTN) RelocBaseEnd) {
//
// Add check for RelocBase->SizeOfBlock field.
//
@@ -1794,7 +1804,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) {
--
2.16.2.windows.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info is invalid
2019-01-30 1:19 [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info is invalid Neo Hsueh
@ 2019-01-30 14:45 ` Laszlo Ersek
2019-01-31 0:27 ` Bi, Dandan
1 sibling, 0 replies; 7+ messages in thread
From: Laszlo Ersek @ 2019-01-30 14:45 UTC (permalink / raw)
To: Neo Hsueh, edk2-devel; +Cc: Michael D Kinney, Liming Gao, Dandan Bi
Hi,
I think this is maybe the 3rd or 4th iteration of the patch. That's OK,
but if you don't add v2, v3, v4 identifies to the subject prefix, such
as [PATCH v2], [PATCH v3] etc, then it's too difficult to follow what
one should review vs. what is obsolete.
Anyway, I think this version is good:
On 01/30/19 02:19, 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 | 30 ++++++++++++++++++++----------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> index 1bd079ad6a..e2c62e1932 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;
> @@ -1739,13 +1739,23 @@ PeCoffLoaderRelocateImageForRuntime (
> // is present in the image. You have to check the NumberOfRvaAndSizes in
> // the optional header to verify a desired directory entry is there.
> //
> + RelocBase = NULL;
> + RelocBaseEnd = NULL;
> 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 (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 +1779,7 @@ PeCoffLoaderRelocateImageForRuntime (
> //
> FixupData = RelocationData;
> RelocBaseOrig = RelocBase;
> - while (RelocBase < RelocBaseEnd) {
> + while ((UINTN) RelocBase < (UINTN) RelocBaseEnd) {
> //
> // Add check for RelocBase->SizeOfBlock field.
> //
> @@ -1794,7 +1804,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) {
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info is invalid
2019-01-30 1:19 [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info is invalid Neo Hsueh
2019-01-30 14:45 ` Laszlo Ersek
@ 2019-01-31 0:27 ` Bi, Dandan
2019-01-31 3:30 ` Gao, Liming
1 sibling, 1 reply; 7+ messages in thread
From: Bi, Dandan @ 2019-01-31 0:27 UTC (permalink / raw)
To: Hsueh, Hong-chihX, edk2-devel@lists.01.org
Cc: Kinney, Michael D, Gao, Liming, Laszlo Ersek
Reviewed-by: Bi Dandan <dandan.bi@intel.com>
Thanks,
Dandan
> -----Original Message-----
> From: Hsueh, Hong-chihX
> Sent: Wednesday, January 30, 2019 9:20 AM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info
> is invalid
>
> 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 | 30
> ++++++++++++++++++++----------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> index 1bd079ad6a..e2c62e1932 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;
> @@ -1739,13 +1739,23 @@ PeCoffLoaderRelocateImageForRuntime (
> // is present in the image. You have to check the NumberOfRvaAndSizes in
> // the optional header to verify a desired directory entry is there.
> //
> + RelocBase = NULL;
> + RelocBaseEnd = NULL;
> 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 (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 +1779,7 @@ PeCoffLoaderRelocateImageForRuntime (
> //
> FixupData = RelocationData;
> RelocBaseOrig = RelocBase;
> - while (RelocBase < RelocBaseEnd) {
> + while ((UINTN) RelocBase < (UINTN) RelocBaseEnd) {
> //
> // Add check for RelocBase->SizeOfBlock field.
> //
> @@ -1794,7 +1804,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) {
> --
> 2.16.2.windows.1
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info is invalid
2019-01-31 0:27 ` Bi, Dandan
@ 2019-01-31 3:30 ` Gao, Liming
2019-01-31 3:38 ` Gao, Liming
0 siblings, 1 reply; 7+ messages in thread
From: Gao, Liming @ 2019-01-31 3:30 UTC (permalink / raw)
To: Bi, Dandan, Hsueh, Hong-chihX, edk2-devel@lists.01.org
Cc: Kinney, Michael D, Laszlo Ersek
Reviewed-by: Liming Gao <liming.gao@intel.com>
> -----Original Message-----
> From: Bi, Dandan
> Sent: Thursday, January 31, 2019 8:27 AM
> To: Hsueh, Hong-chihX <hong-chihx.hsueh@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info is invalid
>
> Reviewed-by: Bi Dandan <dandan.bi@intel.com>
>
> Thanks,
> Dandan
> > -----Original Message-----
> > From: Hsueh, Hong-chihX
> > Sent: Wednesday, January 30, 2019 9:20 AM
> > To: edk2-devel@lists.01.org
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > <liming.gao@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>
> > Subject: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info
> > is invalid
> >
> > 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 | 30
> > ++++++++++++++++++++----------
> > 1 file changed, 20 insertions(+), 10 deletions(-)
> >
> > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > index 1bd079ad6a..e2c62e1932 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;
> > @@ -1739,13 +1739,23 @@ PeCoffLoaderRelocateImageForRuntime (
> > // is present in the image. You have to check the NumberOfRvaAndSizes in
> > // the optional header to verify a desired directory entry is there.
> > //
> > + RelocBase = NULL;
> > + RelocBaseEnd = NULL;
> > 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 (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 +1779,7 @@ PeCoffLoaderRelocateImageForRuntime (
> > //
> > FixupData = RelocationData;
> > RelocBaseOrig = RelocBase;
> > - while (RelocBase < RelocBaseEnd) {
> > + while ((UINTN) RelocBase < (UINTN) RelocBaseEnd) {
> > //
> > // Add check for RelocBase->SizeOfBlock field.
> > //
> > @@ -1794,7 +1804,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) {
> > --
> > 2.16.2.windows.1
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info is invalid
2019-01-31 3:30 ` Gao, Liming
@ 2019-01-31 3:38 ` Gao, Liming
0 siblings, 0 replies; 7+ messages in thread
From: Gao, Liming @ 2019-01-31 3:38 UTC (permalink / raw)
To: Gao, Liming, Bi, Dandan, Hsueh, Hong-chihX,
edk2-devel@lists.01.org
Cc: Kinney, Michael D, Laszlo Ersek
Push at a824c7ebde0a431413329049252b8c1d3770de82
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Gao, Liming
> Sent: Thursday, January 31, 2019 11:31 AM
> To: Bi, Dandan <dandan.bi@intel.com>; Hsueh, Hong-chihX <hong-chihx.hsueh@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [edk2] [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info is invalid
>
> Reviewed-by: Liming Gao <liming.gao@intel.com>
>
> > -----Original Message-----
> > From: Bi, Dandan
> > Sent: Thursday, January 31, 2019 8:27 AM
> > To: Hsueh, Hong-chihX <hong-chihx.hsueh@intel.com>; edk2-devel@lists.01.org
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Laszlo Ersek <lersek@redhat.com>
> > Subject: RE: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info is invalid
> >
> > Reviewed-by: Bi Dandan <dandan.bi@intel.com>
> >
> > Thanks,
> > Dandan
> > > -----Original Message-----
> > > From: Hsueh, Hong-chihX
> > > Sent: Wednesday, January 30, 2019 9:20 AM
> > > To: edk2-devel@lists.01.org
> > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > > <liming.gao@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Laszlo Ersek
> > > <lersek@redhat.com>
> > > Subject: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info
> > > is invalid
> > >
> > > 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 | 30
> > > ++++++++++++++++++++----------
> > > 1 file changed, 20 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > > b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > > index 1bd079ad6a..e2c62e1932 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;
> > > @@ -1739,13 +1739,23 @@ PeCoffLoaderRelocateImageForRuntime (
> > > // is present in the image. You have to check the NumberOfRvaAndSizes in
> > > // the optional header to verify a desired directory entry is there.
> > > //
> > > + RelocBase = NULL;
> > > + RelocBaseEnd = NULL;
> > > 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 (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 +1779,7 @@ PeCoffLoaderRelocateImageForRuntime (
> > > //
> > > FixupData = RelocationData;
> > > RelocBaseOrig = RelocBase;
> > > - while (RelocBase < RelocBaseEnd) {
> > > + while ((UINTN) RelocBase < (UINTN) RelocBaseEnd) {
> > > //
> > > // Add check for RelocBase->SizeOfBlock field.
> > > //
> > > @@ -1794,7 +1804,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) {
> > > --
> > > 2.16.2.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info is invalid
@ 2019-01-29 18:50 Neo Hsueh
2019-01-30 14:42 ` Laszlo Ersek
0 siblings, 1 reply; 7+ messages in thread
From: Neo Hsueh @ 2019-01-29 18:50 UTC (permalink / raw)
To: edk2-devel; +Cc: Michael D Kinney, Liming Gao, Dandan Bi, Laszlo Ersek
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 (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) {
--
2.16.2.windows.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info is invalid
2019-01-29 18:50 Neo Hsueh
@ 2019-01-30 14:42 ` Laszlo Ersek
0 siblings, 0 replies; 7+ messages in thread
From: Laszlo Ersek @ 2019-01-30 14:42 UTC (permalink / raw)
To: Neo Hsueh, edk2-devel; +Cc: Michael D Kinney, Liming Gao, Dandan Bi
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) {
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-01-31 3:38 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-01-30 1:19 [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info is invalid 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
-- strict thread matches above, loose matches on Subject: below --
2019-01-29 18:50 Neo Hsueh
2019-01-30 14:42 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox