* [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.
@ 2019-01-28 18:40 Neo Hsueh
2019-01-28 22:16 ` Laszlo Ersek
2019-01-29 5:13 ` Bi, Dandan
0 siblings, 2 replies; 12+ messages in thread
From: Neo Hsueh @ 2019-01-28 18:40 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 | 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.
2019-01-28 18:40 [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid Neo Hsueh
@ 2019-01-28 22:16 ` Laszlo Ersek
2019-01-28 23:40 ` Hsueh, Hong-chihX
2019-01-29 5:13 ` Bi, Dandan
1 sibling, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2019-01-28 22:16 UTC (permalink / raw)
To: Neo Hsueh, edk2-devel; +Cc: Michael D Kinney, Liming Gao, Dandan Bi
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.
2019-01-28 22:16 ` Laszlo Ersek
@ 2019-01-28 23:40 ` Hsueh, Hong-chihX
2019-01-29 10:52 ` Laszlo Ersek
0 siblings, 1 reply; 12+ messages in thread
From: Hsueh, Hong-chihX @ 2019-01-28 23:40 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Cc: Kinney, Michael D, Gao, Liming, Bi, Dandan
> -----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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.
2019-01-28 23:40 ` Hsueh, Hong-chihX
@ 2019-01-29 10:52 ` Laszlo Ersek
0 siblings, 0 replies; 12+ messages in thread
From: Laszlo Ersek @ 2019-01-29 10:52 UTC (permalink / raw)
To: Hsueh, Hong-chihX, edk2-devel@lists.01.org
Cc: Kinney, Michael D, Gao, Liming, Bi, Dandan
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.
2019-01-28 18:40 [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid Neo Hsueh
2019-01-28 22:16 ` Laszlo Ersek
@ 2019-01-29 5:13 ` Bi, Dandan
2019-01-29 10:55 ` Laszlo Ersek
1 sibling, 1 reply; 12+ messages in thread
From: Bi, Dandan @ 2019-01-29 5:13 UTC (permalink / raw)
To: Hsueh, Hong-chihX, edk2-devel@lists.01.org
Cc: Kinney, Michael D, Gao, Liming, Laszlo Ersek, 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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.
2019-01-29 5:13 ` Bi, Dandan
@ 2019-01-29 10:55 ` Laszlo Ersek
0 siblings, 0 replies; 12+ messages in thread
From: Laszlo Ersek @ 2019-01-29 10:55 UTC (permalink / raw)
To: Bi, Dandan, Hsueh, Hong-chihX, edk2-devel@lists.01.org
Cc: Kinney, Michael D, Gao, Liming
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.
@ 2019-01-28 23:22 Neo Hsueh
2019-01-29 10:57 ` Laszlo Ersek
0 siblings, 1 reply; 12+ messages in thread
From: Neo Hsueh @ 2019-01-28 23:22 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 | 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.
2019-01-28 23:22 Neo Hsueh
@ 2019-01-29 10:57 ` Laszlo Ersek
2019-01-30 1:05 ` Hsueh, Hong-chihX
0 siblings, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2019-01-29 10:57 UTC (permalink / raw)
To: Neo Hsueh, edk2-devel; +Cc: Michael D Kinney, Liming Gao, Dandan Bi
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.
2019-01-29 10:57 ` Laszlo Ersek
@ 2019-01-30 1:05 ` Hsueh, Hong-chihX
0 siblings, 0 replies; 12+ messages in thread
From: Hsueh, Hong-chihX @ 2019-01-30 1:05 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org, Bi, Dandan
Cc: Kinney, Michael D, Gao, Liming
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.
@ 2019-01-24 23:18 Neo Hsueh
2019-01-25 9:07 ` Laszlo Ersek
0 siblings, 1 reply; 12+ messages in thread
From: Neo Hsueh @ 2019-01-24 23:18 UTC (permalink / raw)
To: edk2-devel; +Cc: Michael D Kinney, Liming Gao, Dandan Bi
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.
2019-01-24 23:18 Neo Hsueh
@ 2019-01-25 9:07 ` Laszlo Ersek
2019-01-28 18:46 ` Hsueh, Hong-chihX
0 siblings, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2019-01-25 9:07 UTC (permalink / raw)
To: Neo Hsueh, edk2-devel; +Cc: Michael D Kinney, Dandan Bi, Liming Gao
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.
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.
2019-01-25 9:07 ` Laszlo Ersek
@ 2019-01-28 18:46 ` Hsueh, Hong-chihX
0 siblings, 0 replies; 12+ messages in thread
From: Hsueh, Hong-chihX @ 2019-01-28 18:46 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Cc: Kinney, Michael D, Bi, Dandan, Gao, Liming
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.
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-01-30 1:05 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-01-28 18:40 [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid Neo Hsueh
2019-01-28 22:16 ` Laszlo Ersek
2019-01-28 23:40 ` Hsueh, Hong-chihX
2019-01-29 10:52 ` Laszlo Ersek
2019-01-29 5:13 ` Bi, Dandan
2019-01-29 10:55 ` Laszlo Ersek
-- strict thread matches above, loose matches on Subject: below --
2019-01-28 23:22 Neo Hsueh
2019-01-29 10:57 ` Laszlo Ersek
2019-01-30 1:05 ` Hsueh, Hong-chihX
2019-01-24 23:18 Neo Hsueh
2019-01-25 9:07 ` Laszlo Ersek
2019-01-28 18:46 ` Hsueh, Hong-chihX
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox