[PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.

[PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.

[Neo Hsueh]

1.Skip runtime relocation for PE images that provide invalid relocation
  infomation (ex: RelocDir->Size = 0) to fix a hang observed while booting
  Windows.
2.Add a magic number check for PE32+ image.

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>
---
 MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
index 1bd079ad6a..6477ef0759 100644
--- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
+++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
@@ -1725,11 +1725,18 @@ PeCoffLoaderRelocateImageForRuntime (
     NumberOfRvaAndSizes = Hdr.Pe32->OptionalHeader.NumberOfRvaAndSizes;
     DataDirectory = (EFI_IMAGE_DATA_DIRECTORY *)&(Hdr.Pe32->OptionalHeader.DataDirectory[0]);
   } else {
+    if (Hdr.Pe32Plus->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
+      //
+      // Use PE32+ offset
+      //
+      NumberOfRvaAndSizes = Hdr.Pe32Plus->OptionalHeader.NumberOfRvaAndSizes;
+      DataDirectory = (EFI_IMAGE_DATA_DIRECTORY *)&(Hdr.Pe32Plus->OptionalHeader.DataDirectory[0]);
+    } else {
     //
-    // Use PE32+ offset
+    // Not a valid PE image so Exit
     //
-    NumberOfRvaAndSizes = Hdr.Pe32Plus->OptionalHeader.NumberOfRvaAndSizes;
-    DataDirectory = (EFI_IMAGE_DATA_DIRECTORY *)&(Hdr.Pe32Plus->OptionalHeader.DataDirectory[0]);
+    return;
+    }
   }
 
   //
@@ -1746,6 +1753,12 @@ PeCoffLoaderRelocateImageForRuntime (
                                                                             RelocDir->VirtualAddress + RelocDir->Size - 1,
                                                                             0
                                                                             );
+    if (RelocBase == NULL || RelocBaseEnd == NULL || RelocBaseEnd < RelocBase) {
+      //
+      // relocation block is not valid, just return
+      //
+      return;
+    }
   } else {
     //
     // Cannot find relocations, cannot continue to relocate the image, ASSERT for this invalid image.
-- 
2.16.2.windows.1

Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.

[Laszlo Ersek]

On 01/25/19 00:18, Neo Hsueh wrote:
> 1.Skip runtime relocation for PE images that provide invalid relocation
>   infomation (ex: RelocDir->Size = 0) to fix a hang observed while booting
>   Windows.
> 2.Add a magic number check for PE32+ image.
> 
> 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>
> ---
>  MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)

I can't comment on the technical details of the patch, but I have some
comments on the organization of the patch.

First, the two changes that it implements should be separate patches.

> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> index 1bd079ad6a..6477ef0759 100644
> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> @@ -1725,11 +1725,18 @@ PeCoffLoaderRelocateImageForRuntime (
>      NumberOfRvaAndSizes = Hdr.Pe32->OptionalHeader.NumberOfRvaAndSizes;
>      DataDirectory = (EFI_IMAGE_DATA_DIRECTORY *)&(Hdr.Pe32->OptionalHeader.DataDirectory[0]);
>    } else {
> +    if (Hdr.Pe32Plus->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
> +      //
> +      // Use PE32+ offset
> +      //
> +      NumberOfRvaAndSizes = Hdr.Pe32Plus->OptionalHeader.NumberOfRvaAndSizes;
> +      DataDirectory = (EFI_IMAGE_DATA_DIRECTORY *)&(Hdr.Pe32Plus->OptionalHeader.DataDirectory[0]);
> +    } else {
>      //
> -    // Use PE32+ offset
> +    // Not a valid PE image so Exit
>      //
> -    NumberOfRvaAndSizes = Hdr.Pe32Plus->OptionalHeader.NumberOfRvaAndSizes;
> -    DataDirectory = (EFI_IMAGE_DATA_DIRECTORY *)&(Hdr.Pe32Plus->OptionalHeader.DataDirectory[0]);
> +    return;
> +    }
>    }
>  
>    //

Second, my understanding is that in edk2, we don't do

  if (Condition1) {
  } else {
    if (Condition2) {
    } else {
    }
  }

Instead, we prefer

  if (Condition1) {
  } else if (Condition2) {
  } else {
  }

As far as I know, this is the only construct where we don't require
braces after an "else". See:

https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/57_c_programming.html#57342-when-an-else-is-used-it-may-start-on-the-same-line-as-the-close-brace-of-the-if-or-be-on-the-following-line-and-aligned-with-the-closing-brace


Third, the return statement and the comment are not properly indented in
the last branch.

Thanks
Laszlo

> @@ -1746,6 +1753,12 @@ PeCoffLoaderRelocateImageForRuntime (
>                                                                              RelocDir->VirtualAddress + RelocDir->Size - 1,
>                                                                              0
>                                                                              );
> +    if (RelocBase == NULL || RelocBaseEnd == NULL || RelocBaseEnd < RelocBase) {
> +      //
> +      // relocation block is not valid, just return
> +      //
> +      return;
> +    }
>    } else {
>      //
>      // Cannot find relocations, cannot continue to relocate the image, ASSERT for this invalid image.
> 

[PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.

[Neo Hsueh]

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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
index 1bd079ad6a..f01c691dea 100644
--- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
+++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
@@ -1746,6 +1746,12 @@ PeCoffLoaderRelocateImageForRuntime (
                                                                             RelocDir->VirtualAddress + RelocDir->Size - 1,
                                                                             0
                                                                             );
+    if (RelocBase == NULL || RelocBaseEnd == NULL || RelocBaseEnd < RelocBase) {
+      //
+      // relocation block is not valid, just return
+      //
+      return;
+    }
   } else {
     //
     // Cannot find relocations, cannot continue to relocate the image, ASSERT for this invalid image.
-- 
2.16.2.windows.1

Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.

[Hsueh, Hong-chihX]

Hi Laszlo,
Thank you for your comment.
I've sent an updated patch for review.
https://lists.01.org/pipermail/edk2-devel/2019-January/035806.html

Regards,
Neo

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, January 25, 2019 1:07 AM
> To: Hsueh, Hong-chihX <hong-chihx.hsueh@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2] [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if
> relocation info is invalid.
> 
> On 01/25/19 00:18, Neo Hsueh wrote:
> > 1.Skip runtime relocation for PE images that provide invalid relocation
> >   infomation (ex: RelocDir->Size = 0) to fix a hang observed while booting
> >   Windows.
> > 2.Add a magic number check for PE32+ image.
> >
> > 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>
> > ---
> >  MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 19 ++++++++++++++++---
> >  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> I can't comment on the technical details of the patch, but I have some
> comments on the organization of the patch.
> 
> First, the two changes that it implements should be separate patches.
> 
> > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > index 1bd079ad6a..6477ef0759 100644
> > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > @@ -1725,11 +1725,18 @@ PeCoffLoaderRelocateImageForRuntime (
> >      NumberOfRvaAndSizes = Hdr.Pe32-
> >OptionalHeader.NumberOfRvaAndSizes;
> >      DataDirectory = (EFI_IMAGE_DATA_DIRECTORY *)&(Hdr.Pe32-
> >OptionalHeader.DataDirectory[0]);
> >    } else {
> > +    if (Hdr.Pe32Plus->OptionalHeader.Magic ==
> EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
> > +      //
> > +      // Use PE32+ offset
> > +      //
> > +      NumberOfRvaAndSizes = Hdr.Pe32Plus-
> >OptionalHeader.NumberOfRvaAndSizes;
> > +      DataDirectory = (EFI_IMAGE_DATA_DIRECTORY *)&(Hdr.Pe32Plus-
> >OptionalHeader.DataDirectory[0]);
> > +    } else {
> >      //
> > -    // Use PE32+ offset
> > +    // Not a valid PE image so Exit
> >      //
> > -    NumberOfRvaAndSizes = Hdr.Pe32Plus-
> >OptionalHeader.NumberOfRvaAndSizes;
> > -    DataDirectory = (EFI_IMAGE_DATA_DIRECTORY *)&(Hdr.Pe32Plus-
> >OptionalHeader.DataDirectory[0]);
> > +    return;
> > +    }
> >    }
> >
> >    //
> 
> Second, my understanding is that in edk2, we don't do
> 
>   if (Condition1) {
>   } else {
>     if (Condition2) {
>     } else {
>     }
>   }
> 
> Instead, we prefer
> 
>   if (Condition1) {
>   } else if (Condition2) {
>   } else {
>   }
> 
> As far as I know, this is the only construct where we don't require braces after
> an "else". See:
> 
> https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-
> specification/content/5_source_files/57_c_programming.html#57342-when-
> an-else-is-used-it-may-start-on-the-same-line-as-the-close-brace-of-the-if-or-
> be-on-the-following-line-and-aligned-with-the-closing-brace
> 
> 
> Third, the return statement and the comment are not properly indented in the
> last branch.
> 
> Thanks
> Laszlo
> 
> > @@ -1746,6 +1753,12 @@ PeCoffLoaderRelocateImageForRuntime (
> >                                                                              RelocDir->VirtualAddress +
> RelocDir->Size - 1,
> >                                                                              0
> >
> > );
> > +    if (RelocBase == NULL || RelocBaseEnd == NULL || RelocBaseEnd <
> RelocBase) {
> > +      //
> > +      // relocation block is not valid, just return
> > +      //
> > +      return;
> > +    }
> >    } else {
> >      //
> >      // Cannot find relocations, cannot continue to relocate the image, ASSERT
> for this invalid image.
> >

Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.

[Laszlo Ersek]

On 01/28/19 19:40, 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 | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> index 1bd079ad6a..f01c691dea 100644
> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> @@ -1746,6 +1746,12 @@ PeCoffLoaderRelocateImageForRuntime (
>                                                                              RelocDir->VirtualAddress + RelocDir->Size - 1,
>                                                                              0
>                                                                              );
> +    if (RelocBase == NULL || RelocBaseEnd == NULL || RelocBaseEnd < RelocBase) {
> +      //
> +      // relocation block is not valid, just return
> +      //
> +      return;
> +    }
>    } else {
>      //
>      // Cannot find relocations, cannot continue to relocate the image, ASSERT for this invalid image.
> 

Thank you for the update.

... Originally I meant to respond with an Acked-by (purely from a formal
point-of-view); however I figured the patch wasn't large and I could
check it for a Reviewed-by as well.

I'm noticing the comparison (RelocBaseEnd < RelocBase) is supposed to
catch invalid relocation info. These variables are pointers, declared as
follows:

  EFI_IMAGE_BASE_RELOCATION             *RelocBase;
  EFI_IMAGE_BASE_RELOCATION             *RelocBaseEnd;

According to the C standard, the relational operators can only be
applied to a pair of pointers if each of those points into the same
array, or one past the last element. In this case, given that you intend
to catch invalid relocation info, that's exactly *not* the case. In
other words, in the only case when the relational operator would
evaluate to true, it would also invoke undefined behavior. Furthermore,
the byte distance between the pointed-to-objects might not even be a
whole multiple of sizeof (EFI_IMAGE_BASE_RELOCATION).

Normally I would suggest changing the return type of
PeCoffLoaderImageAddress() to UINTN -- that would be fitting because the
internal computation is already performed in UINTN, and only cast to
(CHAR8 *) as last step. This way we could move the cast to the callers,
and perform the sanity checks before the conversion to (VOID*) (or to
other pointer types).

I do see the function is called from many places, so this change might
be too costly. Can we at least write in this patch,

  if (RelocBase == NULL ||
      RelocBaseEnd == NULL ||
      (UINTN)RelocBaseEnd < (UINTN)RelocBase ||
      (((UINTN)RelocBaseEnd - (UINTN)RelocBase) %
       sizeof (EFI_IMAGE_BASE_RELOCATION) != 0)) {
    return;
  }

?

Perhaps we should even extract this logic to a helper function, because
I see another spot with the same condition. That's in
PeCoffLoaderRelocateImage(), from the top of commit a8d8d430510d
("Support load 64 bit image from 32 bit core. Add more enhancement to
check invalid PE format.", 2014-03-25).

I'm sorry that I didn't manage to make these suggestions under the v1
posting.

Thanks,
Laszlo

[PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.

[Neo Hsueh]

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 | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
index 1bd079ad6a..6d6c37bd61 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;
@@ -1746,6 +1746,12 @@ PeCoffLoaderRelocateImageForRuntime (
                                                                             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 +1775,7 @@ PeCoffLoaderRelocateImageForRuntime (
     //
     FixupData = RelocationData;
     RelocBaseOrig = RelocBase;
-    while (RelocBase < RelocBaseEnd) {
+    while ((UINTN) RelocBase < (UINTN) RelocBaseEnd) {
       //
       // Add check for RelocBase->SizeOfBlock field.
       //
@@ -1794,7 +1800,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

Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.

[Hsueh, Hong-chihX]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, January 28, 2019 2:17 PM
> 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>; Bi, Dandan <dandan.bi@intel.com>
> Subject: Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if
> relocation info is invalid.
> 
> On 01/28/19 19:40, 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 | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > index 1bd079ad6a..f01c691dea 100644
> > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > @@ -1746,6 +1746,12 @@ PeCoffLoaderRelocateImageForRuntime (
> >                                                                              RelocDir->VirtualAddress +
> RelocDir->Size - 1,
> >                                                                              0
> >
> > );
> > +    if (RelocBase == NULL || RelocBaseEnd == NULL || RelocBaseEnd <
> RelocBase) {
> > +      //
> > +      // relocation block is not valid, just return
> > +      //
> > +      return;
> > +    }
> >    } else {
> >      //
> >      // Cannot find relocations, cannot continue to relocate the image, ASSERT
> for this invalid image.
> >
> 
> Thank you for the update.
> 
> ... Originally I meant to respond with an Acked-by (purely from a formal point-
> of-view); however I figured the patch wasn't large and I could check it for a
> Reviewed-by as well.
> 
> I'm noticing the comparison (RelocBaseEnd < RelocBase) is supposed to catch
> invalid relocation info. These variables are pointers, declared as
> follows:
> 
>   EFI_IMAGE_BASE_RELOCATION             *RelocBase;
>   EFI_IMAGE_BASE_RELOCATION             *RelocBaseEnd;
> 
> According to the C standard, the relational operators can only be applied to a
> pair of pointers if each of those points into the same array, or one past the last
> element. In this case, given that you intend to catch invalid relocation info,
> that's exactly *not* the case. In other words, in the only case when the
> relational operator would evaluate to true, it would also invoke undefined
> behavior. Furthermore, the byte distance between the pointed-to-objects might
> not even be a whole multiple of sizeof (EFI_IMAGE_BASE_RELOCATION).
> 
> Normally I would suggest changing the return type of
> PeCoffLoaderImageAddress() to UINTN -- that would be fitting because the
> internal computation is already performed in UINTN, and only cast to
> (CHAR8 *) as last step. This way we could move the cast to the callers, and
> perform the sanity checks before the conversion to (VOID*) (or to other pointer
> types).
> 
> I do see the function is called from many places, so this change might be too
> costly. Can we at least write in this patch,
> 
>   if (RelocBase == NULL ||
>       RelocBaseEnd == NULL ||
>       (UINTN)RelocBaseEnd < (UINTN)RelocBase ||
>       (((UINTN)RelocBaseEnd - (UINTN)RelocBase) %
>        sizeof (EFI_IMAGE_BASE_RELOCATION) != 0)) {
>     return;
>   }
> 
> ?
> 
> Perhaps we should even extract this logic to a helper function, because I see
> another spot with the same condition. That's in PeCoffLoaderRelocateImage(),
> from the top of commit a8d8d430510d ("Support load 64 bit image from 32 bit
> core. Add more enhancement to check invalid PE format.", 2014-03-25).
> 
> I'm sorry that I didn't manage to make these suggestions under the v1 posting.
> 
> Thanks,
> Laszlo

Hi Laszlo,
Thank you. I agree the pointer comparison is not optimal especially in this case.
However I didn't add multiple of size (EFI_IMAGE_BASE_RELOCATION) check because from the commit eb76b762, we actually check the address range between Base to RelocDir->Size - 1.

Here is the updated patch :
https://lists.01.org/pipermail/edk2-devel/2019-January/035810.html

Regards,
Neo

Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.

[Bi, Dandan]

Hi Neo,

Thank you very much for the patch.

Some minor comments
1) Besides the skip check in this patch, could you help to add additional  check for RelocDir->Size before calling PeCoffLoaderImageAddress to calculate the RelocBase and RelocBaseEnd?
Since when RelocDir->Size==0, we can just return, don't need to do the calculation.

2) Please use the PatchCheck.py in edk2\BaseTools\Scripts to check the patch format before committing  the patch.
Can refer following link for more info:
https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format


Thanks,
Dandan

> -----Original Message-----
> From: Hsueh, Hong-chihX
> Sent: Tuesday, January 29, 2019 2:41 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
> relocation 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 | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> index 1bd079ad6a..f01c691dea 100644
> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> @@ -1746,6 +1746,12 @@ PeCoffLoaderRelocateImageForRuntime (
>                                                                              RelocDir->VirtualAddress + RelocDir-
> >Size - 1,
>                                                                              0
>                                                                              );
> +    if (RelocBase == NULL || RelocBaseEnd == NULL || RelocBaseEnd <
> RelocBase) {
> +      //
> +      // relocation block is not valid, just return
> +      //
> +      return;
> +    }
>    } else {
>      //
>      // Cannot find relocations, cannot continue to relocate the image, ASSERT
> for this invalid image.
> --
> 2.16.2.windows.1

Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.

[Laszlo Ersek]

On 01/29/19 00:40, Hsueh, Hong-chihX wrote:
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, January 28, 2019 2:17 PM
>> 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>; Bi, Dandan <dandan.bi@intel.com>
>> Subject: Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if
>> relocation info is invalid.
>>
>> On 01/28/19 19:40, 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 | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>>> b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>>> index 1bd079ad6a..f01c691dea 100644
>>> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>>> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>>> @@ -1746,6 +1746,12 @@ PeCoffLoaderRelocateImageForRuntime (
>>>                                                                              RelocDir->VirtualAddress +
>> RelocDir->Size - 1,
>>>                                                                              0
>>>
>>> );
>>> +    if (RelocBase == NULL || RelocBaseEnd == NULL || RelocBaseEnd <
>> RelocBase) {
>>> +      //
>>> +      // relocation block is not valid, just return
>>> +      //
>>> +      return;
>>> +    }
>>>    } else {
>>>      //
>>>      // Cannot find relocations, cannot continue to relocate the image, ASSERT
>> for this invalid image.
>>>
>>
>> Thank you for the update.
>>
>> ... Originally I meant to respond with an Acked-by (purely from a formal point-
>> of-view); however I figured the patch wasn't large and I could check it for a
>> Reviewed-by as well.
>>
>> I'm noticing the comparison (RelocBaseEnd < RelocBase) is supposed to catch
>> invalid relocation info. These variables are pointers, declared as
>> follows:
>>
>>   EFI_IMAGE_BASE_RELOCATION             *RelocBase;
>>   EFI_IMAGE_BASE_RELOCATION             *RelocBaseEnd;
>>
>> According to the C standard, the relational operators can only be applied to a
>> pair of pointers if each of those points into the same array, or one past the last
>> element. In this case, given that you intend to catch invalid relocation info,
>> that's exactly *not* the case. In other words, in the only case when the
>> relational operator would evaluate to true, it would also invoke undefined
>> behavior. Furthermore, the byte distance between the pointed-to-objects might
>> not even be a whole multiple of sizeof (EFI_IMAGE_BASE_RELOCATION).
>>
>> Normally I would suggest changing the return type of
>> PeCoffLoaderImageAddress() to UINTN -- that would be fitting because the
>> internal computation is already performed in UINTN, and only cast to
>> (CHAR8 *) as last step. This way we could move the cast to the callers, and
>> perform the sanity checks before the conversion to (VOID*) (or to other pointer
>> types).
>>
>> I do see the function is called from many places, so this change might be too
>> costly. Can we at least write in this patch,
>>
>>   if (RelocBase == NULL ||
>>       RelocBaseEnd == NULL ||
>>       (UINTN)RelocBaseEnd < (UINTN)RelocBase ||
>>       (((UINTN)RelocBaseEnd - (UINTN)RelocBase) %
>>        sizeof (EFI_IMAGE_BASE_RELOCATION) != 0)) {
>>     return;
>>   }
>>
>> ?
>>
>> Perhaps we should even extract this logic to a helper function, because I see
>> another spot with the same condition. That's in PeCoffLoaderRelocateImage(),
>> from the top of commit a8d8d430510d ("Support load 64 bit image from 32 bit
>> core. Add more enhancement to check invalid PE format.", 2014-03-25).
>>
>> I'm sorry that I didn't manage to make these suggestions under the v1 posting.
>>
>> Thanks,
>> Laszlo
> 
> Hi Laszlo,
> Thank you. I agree the pointer comparison is not optimal especially in this case.
> However I didn't add multiple of size (EFI_IMAGE_BASE_RELOCATION) check because from the commit eb76b762, we actually check the address range between Base to RelocDir->Size - 1.

Thank you for pointing that out.

I think that patch is not correct. We have:

  EFI_IMAGE_BASE_RELOCATION             *RelocBase;
  EFI_IMAGE_BASE_RELOCATION             *RelocBaseEnd;

and

    RelocBase = (EFI_IMAGE_BASE_RELOCATION *) PeCoffLoaderImageAddress (ImageContext, RelocDir->VirtualAddress, TeStrippedOffset);
    RelocBaseEnd = (EFI_IMAGE_BASE_RELOCATION *) PeCoffLoaderImageAddress (ImageContext,
                                                                            RelocDir->VirtualAddress + RelocDir->Size - 1,
                                                                            TeStrippedOffset
                                                                            );

It is fine to make RelocBaseEnd an *inclusive* end pointer (if that is our goal -- I'm not sure why though), but in that case, we should not cast the result to (EFI_IMAGE_BASE_RELOCATION*), and we certainly shouldn't compare (RelocBase < RelocBaseEnd), when we know that RelocBaseEnd can never point to an EFI_IMAGE_BASE_RELOCATION, or precisely one past it.

Thanks
Laszlo

> 
> Here is the updated patch :
> https://lists.01.org/pipermail/edk2-devel/2019-January/035810.html
> 
> Regards,
> Neo
> 

Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.

[Laszlo Ersek]

On 01/29/19 06:13, Bi, Dandan wrote:
> Hi Neo,
> 
> Thank you very much for the patch.
> 
> Some minor comments
> 1) Besides the skip check in this patch, could you help to add additional  check for RelocDir->Size before calling PeCoffLoaderImageAddress to calculate the RelocBase and RelocBaseEnd?
> Since when RelocDir->Size==0, we can just return, don't need to do the calculation.

I agree, checking Size against 0 before calling
PeCoffLoaderImageAddress() is best; we do the same in
PeCoffLoaderRelocateImage():

  if ((RelocDir != NULL) && (RelocDir->Size > 0)) {

Thanks,
Laszlo

> 
> 2) Please use the PatchCheck.py in edk2\BaseTools\Scripts to check the patch format before committing  the patch.
> Can refer following link for more info:
> https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format
> 
> 
> Thanks,
> Dandan
> 
>> -----Original Message-----
>> From: Hsueh, Hong-chihX
>> Sent: Tuesday, January 29, 2019 2:41 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
>> relocation 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 | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>> b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>> index 1bd079ad6a..f01c691dea 100644
>> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>> @@ -1746,6 +1746,12 @@ PeCoffLoaderRelocateImageForRuntime (
>>                                                                              RelocDir->VirtualAddress + RelocDir-
>>> Size - 1,
>>                                                                              0
>>                                                                              );
>> +    if (RelocBase == NULL || RelocBaseEnd == NULL || RelocBaseEnd <
>> RelocBase) {
>> +      //
>> +      // relocation block is not valid, just return
>> +      //
>> +      return;
>> +    }
>>    } else {
>>      //
>>      // Cannot find relocations, cannot continue to relocate the image, ASSERT
>> for this invalid image.
>> --
>> 2.16.2.windows.1
> 

Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.

[Laszlo Ersek]

On 01/29/19 00:22, 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 | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> index 1bd079ad6a..6d6c37bd61 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;
> @@ -1746,6 +1746,12 @@ PeCoffLoaderRelocateImageForRuntime (
>                                                                              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 +1775,7 @@ PeCoffLoaderRelocateImageForRuntime (
>      //
>      FixupData = RelocationData;
>      RelocBaseOrig = RelocBase;
> -    while (RelocBase < RelocBaseEnd) {
> +    while ((UINTN) RelocBase < (UINTN) RelocBaseEnd) {
>        //
>        // Add check for RelocBase->SizeOfBlock field.
>        //
> @@ -1794,7 +1800,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) {
> 

I think this patch is good enough, but it seems to miss the size==0
check that Dandan asked for.

Thanks,
laszlo

Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.

[Hsueh, Hong-chihX]

Hi Laszlo & Dandan,
Here is the updated patch for your review. Thank you!
https://lists.01.org/pipermail/edk2-devel/2019-January/035954.html

Regards,
Neo

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, January 29, 2019 2:57 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>; Bi, Dandan <dandan.bi@intel.com>
> Subject: Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if
> relocation info is invalid.
> 
> On 01/29/19 00:22, 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 | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > index 1bd079ad6a..6d6c37bd61 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;
> > @@ -1746,6 +1746,12 @@ PeCoffLoaderRelocateImageForRuntime (
> >                                                                              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 +1775,7 @@ PeCoffLoaderRelocateImageForRuntime (
> >      //
> >      FixupData = RelocationData;
> >      RelocBaseOrig = RelocBase;
> > -    while (RelocBase < RelocBaseEnd) {
> > +    while ((UINTN) RelocBase < (UINTN) RelocBaseEnd) {
> >        //
> >        // Add check for RelocBase->SizeOfBlock field.
> >        //
> > @@ -1794,7 +1800,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) {
> >
> 
> I think this patch is good enough, but it seems to miss the size==0 check that
> Dandan asked for.
> 
> Thanks,
> laszlo