MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4425
This change is to support the pre-built EFI image on the old Edk2 code.
Old Edk2 GenFw tool generates the wrong debug entry in EFI image.
Those pre-built images can be loaded before d6457b309.
Signed-off-by: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Michael Kubacki <michael.kubacki@microsoft.com>
---
MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
index 4b71176a0c..4636842eeb 100644
--- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
+++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
@@ -735,7 +735,7 @@ PeCoffLoaderGetImageInfo (
&Size,
&DebugEntry
);
- if (RETURN_ERROR (Status) || (Size != ReadSize)) {
+ if ((ImageContext->DebugDirectoryEntryRva == 0) && (RETURN_ERROR (Status) || (Size != ReadSize))) {
ImageContext->ImageError = IMAGE_ERROR_IMAGE_READ;
if (Size != ReadSize) {
Status = RETURN_UNSUPPORTED;
--
2.37.3.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#103698): https://edk2.groups.io/g/devel/message/103698
Mute This Topic: https://groups.io/mt/98531785/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Thu, 27 Apr 2023 at 07:17, gaoliming via groups.io <gaoliming=byosoft.com.cn@groups.io> wrote: > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4425 > > This change is to support the pre-built EFI image on the old Edk2 code. > Old Edk2 GenFw tool generates the wrong debug entry in EFI image. > Those pre-built images can be loaded before d6457b309. > > Signed-off-by: Liming Gao <gaoliming@byosoft.com.cn> > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: Michael Kubacki <michael.kubacki@microsoft.com> > --- > MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > index 4b71176a0c..4636842eeb 100644 > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > @@ -735,7 +735,7 @@ PeCoffLoaderGetImageInfo ( > &Size, > &DebugEntry > ); > - if (RETURN_ERROR (Status) || (Size != ReadSize)) { > + if ((ImageContext->DebugDirectoryEntryRva == 0) && (RETURN_ERROR (Status) || (Size != ReadSize))) { I'm not sure I understand how this fixes anything. Why is the condition 'RVA == 0' significant here? Would something like the below perhaps work as well? --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c @@ -755,6 +755,17 @@ PeCoffLoaderGetImageInfo ( ImageContext->ImageSize += DebugEntry.SizeOfData; } + // + // Implementations of GenFw before commit 60e85a39fe49071 will + // concatenate the debug directory entry and the codeview entry, + // and erroneously put the combined size into the debug directory + // entry's size field. If this is the case, no other relevant + // directory entries can exist, and we can terminate here. + // + if (DebugEntry.FileOffset == (DebugDirectoryEntryFileOffset + Index)) { + break; + } + continue; } -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#103717): https://edk2.groups.io/g/devel/message/103717 Mute This Topic: https://groups.io/mt/98531785/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Ard: > -----邮件原件----- > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Ard > Biesheuvel > 发送时间: 2023年4月27日 22:47 > 收件人: devel@edk2.groups.io; gaoliming@byosoft.com.cn > 抄送: Michael Kubacki <michael.kubacki@microsoft.com> > 主题: Re: [edk2-devel] [PATCH] MdePkg BasePeCoffLib: Ignore the debug > entry read error after it is found > > On Thu, 27 Apr 2023 at 07:17, gaoliming via groups.io > <gaoliming=byosoft.com.cn@groups.io> wrote: > > > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4425 > > > > This change is to support the pre-built EFI image on the old Edk2 code. > > Old Edk2 GenFw tool generates the wrong debug entry in EFI image. > > Those pre-built images can be loaded before d6457b309. > > > > Signed-off-by: Liming Gao <gaoliming@byosoft.com.cn> > > Cc: Ard Biesheuvel <ardb@kernel.org> > > Cc: Michael Kubacki <michael.kubacki@microsoft.com> > > --- > > MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > index 4b71176a0c..4636842eeb 100644 > > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > @@ -735,7 +735,7 @@ PeCoffLoaderGetImageInfo ( > > &Size, > > &DebugEntry > > ); > > - if (RETURN_ERROR (Status) || (Size != ReadSize)) { > > + if ((ImageContext->DebugDirectoryEntryRva == 0) && > (RETURN_ERROR (Status) || (Size != ReadSize))) { > > I'm not sure I understand how this fixes anything. Why is the > condition 'RVA == 0' significant here? > > Would something like the below perhaps work as well? > DebugDirectoryEntryRva will set to the real value if DEBUG_TYPE_CODEVIEW is found. If its value is not zero, it means DebugEntry has been found. This is not an error, and don't need to report error here. > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > @@ -755,6 +755,17 @@ PeCoffLoaderGetImageInfo ( > ImageContext->ImageSize += DebugEntry.SizeOfData; > } > > + // > + // Implementations of GenFw before commit > 60e85a39fe49071 will > + // concatenate the debug directory entry and the codeview > entry, > + // and erroneously put the combined size into the debug > directory > + // entry's size field. If this is the case, no other relevant > + // directory entries can exist, and we can terminate here. > + // > + if (DebugEntry.FileOffset == > (DebugDirectoryEntryFileOffset + Index)) { > + break; > + } > + d6457b309 adds DEBUG_TYPE_EX_DLLCHARACTERISTICS support. So, there may be two DEBUG entries in one EFI image. One is DEBUG_TYPE_CODEVIEW, another is DEBUG_TYPE_EX_DLLCHARACTERISTICS. Here break after DEBUG_TYPE_CODEVIEW is found. So, is there the chance to search DEBUG_TYPE_EX_DLLCHARACTERISTICS? Thanks Liming > continue; > } > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#103754): https://edk2.groups.io/g/devel/message/103754 Mute This Topic: https://groups.io/mt/98553124/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Fri, 28 Apr 2023 at 07:02, gaoliming via groups.io <gaoliming=byosoft.com.cn@groups.io> wrote: > > Ard: > > > -----邮件原件----- > > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Ard > > Biesheuvel > > 发送时间: 2023年4月27日 22:47 > > 收件人: devel@edk2.groups.io; gaoliming@byosoft.com.cn > > 抄送: Michael Kubacki <michael.kubacki@microsoft.com> > > 主题: Re: [edk2-devel] [PATCH] MdePkg BasePeCoffLib: Ignore the debug > > entry read error after it is found > > > > On Thu, 27 Apr 2023 at 07:17, gaoliming via groups.io > > <gaoliming=byosoft.com.cn@groups.io> wrote: > > > > > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4425 > > > > > > This change is to support the pre-built EFI image on the old Edk2 code. > > > Old Edk2 GenFw tool generates the wrong debug entry in EFI image. > > > Those pre-built images can be loaded before d6457b309. > > > > > > Signed-off-by: Liming Gao <gaoliming@byosoft.com.cn> > > > Cc: Ard Biesheuvel <ardb@kernel.org> > > > Cc: Michael Kubacki <michael.kubacki@microsoft.com> > > > --- > > > MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > > index 4b71176a0c..4636842eeb 100644 > > > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > > @@ -735,7 +735,7 @@ PeCoffLoaderGetImageInfo ( > > > &Size, > > > &DebugEntry > > > ); > > > - if (RETURN_ERROR (Status) || (Size != ReadSize)) { > > > + if ((ImageContext->DebugDirectoryEntryRva == 0) && > > (RETURN_ERROR (Status) || (Size != ReadSize))) { > > > > I'm not sure I understand how this fixes anything. Why is the > > condition 'RVA == 0' significant here? > > > > Would something like the below perhaps work as well? > > > > DebugDirectoryEntryRva will set to the real value if DEBUG_TYPE_CODEVIEW is found. > If its value is not zero, it means DebugEntry has been found. This is not an error, and don't need to report error here. > > > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > @@ -755,6 +755,17 @@ PeCoffLoaderGetImageInfo ( > > ImageContext->ImageSize += DebugEntry.SizeOfData; > > } > > > > + // > > + // Implementations of GenFw before commit > > 60e85a39fe49071 will > > + // concatenate the debug directory entry and the codeview > > entry, > > + // and erroneously put the combined size into the debug > > directory > > + // entry's size field. If this is the case, no other relevant > > + // directory entries can exist, and we can terminate here. > > + // > > + if (DebugEntry.FileOffset == > > (DebugDirectoryEntryFileOffset + Index)) { > > + break; > > + } > > + > d6457b309 adds DEBUG_TYPE_EX_DLLCHARACTERISTICS support. So, there may be two DEBUG entries in one EFI image. > One is DEBUG_TYPE_CODEVIEW, another is DEBUG_TYPE_EX_DLLCHARACTERISTICS. > > Here break after DEBUG_TYPE_CODEVIEW is found. So, is there the chance to search DEBUG_TYPE_EX_DLLCHARACTERISTICS? > No. But if the codeview payload follows the codeview entry directly, there cannot be any other entries, so there is no reason to proceed. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#103773): https://edk2.groups.io/g/devel/message/103773 Mute This Topic: https://groups.io/mt/98554964/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Ard: > -----邮件原件----- > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Ard > Biesheuvel > 发送时间: 2023年4月28日 17:21 > 收件人: devel@edk2.groups.io; gaoliming@byosoft.com.cn > 抄送: Michael Kubacki <michael.kubacki@microsoft.com> > 主题: Re: [edk2-devel] [PATCH] MdePkg BasePeCoffLib: Ignore the debug > entry read error after it is found > > On Fri, 28 Apr 2023 at 07:02, gaoliming via groups.io > <gaoliming=byosoft.com.cn@groups.io> wrote: > > > > Ard: > > > > > -----邮件原件----- > > > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Ard > > > Biesheuvel > > > 发送时间: 2023年4月27日 22:47 > > > 收件人: devel@edk2.groups.io; gaoliming@byosoft.com.cn > > > 抄送: Michael Kubacki <michael.kubacki@microsoft.com> > > > 主题: Re: [edk2-devel] [PATCH] MdePkg BasePeCoffLib: Ignore the debug > > > entry read error after it is found > > > > > > On Thu, 27 Apr 2023 at 07:17, gaoliming via groups.io > > > <gaoliming=byosoft.com.cn@groups.io> wrote: > > > > > > > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4425 > > > > > > > > This change is to support the pre-built EFI image on the old Edk2 code. > > > > Old Edk2 GenFw tool generates the wrong debug entry in EFI image. > > > > Those pre-built images can be loaded before d6457b309. > > > > > > > > Signed-off-by: Liming Gao <gaoliming@byosoft.com.cn> > > > > Cc: Ard Biesheuvel <ardb@kernel.org> > > > > Cc: Michael Kubacki <michael.kubacki@microsoft.com> > > > > --- > > > > MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > > b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > > > index 4b71176a0c..4636842eeb 100644 > > > > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > > > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > > > @@ -735,7 +735,7 @@ PeCoffLoaderGetImageInfo ( > > > > &Size, > > > > &DebugEntry > > > > ); > > > > - if (RETURN_ERROR (Status) || (Size != ReadSize)) { > > > > + if ((ImageContext->DebugDirectoryEntryRva == 0) && > > > (RETURN_ERROR (Status) || (Size != ReadSize))) { > > > > > > I'm not sure I understand how this fixes anything. Why is the > > > condition 'RVA == 0' significant here? > > > > > > Would something like the below perhaps work as well? > > > > > > > DebugDirectoryEntryRva will set to the real value if > DEBUG_TYPE_CODEVIEW is found. > > If its value is not zero, it means DebugEntry has been found. This is not an > error, and don't need to report error here. > > > > > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > > @@ -755,6 +755,17 @@ PeCoffLoaderGetImageInfo ( > > > ImageContext->ImageSize += DebugEntry.SizeOfData; > > > } > > > > > > + // > > > + // Implementations of GenFw before commit > > > 60e85a39fe49071 will > > > + // concatenate the debug directory entry and the > codeview > > > entry, > > > + // and erroneously put the combined size into the debug > > > directory > > > + // entry's size field. If this is the case, no other relevant > > > + // directory entries can exist, and we can terminate here. > > > + // > > > + if (DebugEntry.FileOffset == > > > (DebugDirectoryEntryFileOffset + Index)) { > > > + break; > > > + } > > > + > > d6457b309 adds DEBUG_TYPE_EX_DLLCHARACTERISTICS support. So, > there may be two DEBUG entries in one EFI image. > > One is DEBUG_TYPE_CODEVIEW, another is > DEBUG_TYPE_EX_DLLCHARACTERISTICS. > > > > Here break after DEBUG_TYPE_CODEVIEW is found. So, is there the chance > to search DEBUG_TYPE_EX_DLLCHARACTERISTICS? > > > > No. But if the codeview payload follows the codeview entry directly, > there cannot be any other entries, so there is no reason to proceed. > > I verified this change. It doesn't work, because DebugEntry.FileOffset value is DebugDirectoryEntryFileOffset + Index + sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY) I review the change 60e85a39fe49071 in GenFw. It places DebugEntry as the last debug directory. So, I think the simple fix is to directly break once DebugEntry is found. Thanks Liming > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#103973): https://edk2.groups.io/g/devel/message/103973 Mute This Topic: https://groups.io/mt/98679630/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Thu, 4 May 2023 at 09:49, gaoliming via groups.io <gaoliming=byosoft.com.cn@groups.io> wrote: > > Ard: > > > -----邮件原件----- > > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Ard > > Biesheuvel > > 发送时间: 2023年4月28日 17:21 > > 收件人: devel@edk2.groups.io; gaoliming@byosoft.com.cn > > 抄送: Michael Kubacki <michael.kubacki@microsoft.com> > > 主题: Re: [edk2-devel] [PATCH] MdePkg BasePeCoffLib: Ignore the debug > > entry read error after it is found > > > > On Fri, 28 Apr 2023 at 07:02, gaoliming via groups.io > > <gaoliming=byosoft.com.cn@groups.io> wrote: > > > > > > Ard: > > > > > > > -----邮件原件----- > > > > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Ard > > > > Biesheuvel > > > > 发送时间: 2023年4月27日 22:47 > > > > 收件人: devel@edk2.groups.io; gaoliming@byosoft.com.cn > > > > 抄送: Michael Kubacki <michael.kubacki@microsoft.com> > > > > 主题: Re: [edk2-devel] [PATCH] MdePkg BasePeCoffLib: Ignore the debug > > > > entry read error after it is found > > > > > > > > On Thu, 27 Apr 2023 at 07:17, gaoliming via groups.io > > > > <gaoliming=byosoft.com.cn@groups.io> wrote: > > > > > > > > > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4425 > > > > > > > > > > This change is to support the pre-built EFI image on the old Edk2 code. > > > > > Old Edk2 GenFw tool generates the wrong debug entry in EFI image. > > > > > Those pre-built images can be loaded before d6457b309. > > > > > > > > > > Signed-off-by: Liming Gao <gaoliming@byosoft.com.cn> > > > > > Cc: Ard Biesheuvel <ardb@kernel.org> > > > > > Cc: Michael Kubacki <michael.kubacki@microsoft.com> > > > > > --- > > > > > MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > > > b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > > > > index 4b71176a0c..4636842eeb 100644 > > > > > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > > > > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > > > > @@ -735,7 +735,7 @@ PeCoffLoaderGetImageInfo ( > > > > > &Size, > > > > > &DebugEntry > > > > > ); > > > > > - if (RETURN_ERROR (Status) || (Size != ReadSize)) { > > > > > + if ((ImageContext->DebugDirectoryEntryRva == 0) && > > > > (RETURN_ERROR (Status) || (Size != ReadSize))) { > > > > > > > > I'm not sure I understand how this fixes anything. Why is the > > > > condition 'RVA == 0' significant here? > > > > > > > > Would something like the below perhaps work as well? > > > > > > > > > > DebugDirectoryEntryRva will set to the real value if > > DEBUG_TYPE_CODEVIEW is found. > > > If its value is not zero, it means DebugEntry has been found. This is not an > > error, and don't need to report error here. > > > > > > > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > > > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > > > @@ -755,6 +755,17 @@ PeCoffLoaderGetImageInfo ( > > > > ImageContext->ImageSize += DebugEntry.SizeOfData; > > > > } > > > > > > > > + // > > > > + // Implementations of GenFw before commit > > > > 60e85a39fe49071 will > > > > + // concatenate the debug directory entry and the > > codeview > > > > entry, > > > > + // and erroneously put the combined size into the debug > > > > directory > > > > + // entry's size field. If this is the case, no other relevant > > > > + // directory entries can exist, and we can terminate here. > > > > + // > > > > + if (DebugEntry.FileOffset == > > > > (DebugDirectoryEntryFileOffset + Index)) { > > > > + break; > > > > + } > > > > + > > > d6457b309 adds DEBUG_TYPE_EX_DLLCHARACTERISTICS support. So, > > there may be two DEBUG entries in one EFI image. > > > One is DEBUG_TYPE_CODEVIEW, another is > > DEBUG_TYPE_EX_DLLCHARACTERISTICS. > > > > > > Here break after DEBUG_TYPE_CODEVIEW is found. So, is there the chance > > to search DEBUG_TYPE_EX_DLLCHARACTERISTICS? > > > > > > > No. But if the codeview payload follows the codeview entry directly, > > there cannot be any other entries, so there is no reason to proceed. > > > > > I verified this change. It doesn't work, because > DebugEntry.FileOffset value is DebugDirectoryEntryFileOffset + Index + sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY) > > I review the change 60e85a39fe49071 in GenFw. It places DebugEntry as the last debug directory. > So, I think the simple fix is to directly break once DebugEntry is found. > I'm confused. Doesn't that mean we will ignore the DEBUG_TYPE_EX_DLLCHARACTERISTICS entry? Given that the old, broken GenFw puts the NB10 directory entry and the NB10 data structure contiguous in memory, we should be able to detect that, right? And in this case, because the NB10 directory entry is followed by something we /know/ is not another directory entry, we can exit the loop early. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#104008): https://edk2.groups.io/g/devel/message/104008 Mute This Topic: https://groups.io/mt/98684582/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.