* [PATCH] MdePkg/BasePeCoffLib: Deal with broken debug directories
@ 2023-05-04 14:47 Ard Biesheuvel
2023-05-05 1:54 ` 回复: " gaoliming
2023-05-05 1:56 ` [edk2-devel] " Michael Kubacki
0 siblings, 2 replies; 4+ messages in thread
From: Ard Biesheuvel @ 2023-05-04 14:47 UTC (permalink / raw)
To: devel; +Cc: gaoliming, michael.kubacki, Ard Biesheuvel
Older versions of GenFw put the wrong value in the debug directory size
field in the PE/COFF header: instead of putting the combined size of all
the entries, it puts the size of the only entry it creates, but adds the
size of the NB10 payload that the entry points to. This confuses the
loader now that we started using additional debug directory entries to
describe DLL characteristics.
GenFw was fixed in commit 60e85a39fe49071, but the binaries that were
generated with it still need to be supported.
So let's detect this condition, and check whether the size of the debug
directory is consistent with the NB10 payload: if we should expect
additional directory entries where we observe the NB10 payload, the size
field is clearly wrong, and we can break from the loop.
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4425
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
index 4b71176a0c7c2ed0..27f8526370fa3859 100644
--- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
+++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
@@ -585,6 +585,7 @@ PeCoffLoaderGetImageInfo (
UINTN Size;
UINTN ReadSize;
UINTN Index;
+ UINTN NextIndex;
UINTN DebugDirectoryEntryRva;
UINTN DebugDirectoryEntryFileOffset;
UINTN SectionHeaderOffset;
@@ -755,6 +756,19 @@ 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.
+ //
+ NextIndex = Index + sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
+ if ((NextIndex < DebugDirectoryEntry->Size) &&
+ (DebugEntry.FileOffset == (DebugDirectoryEntryFileOffset + NextIndex))) {
+ break;
+ }
+
continue;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* 回复: [PATCH] MdePkg/BasePeCoffLib: Deal with broken debug directories
2023-05-04 14:47 [PATCH] MdePkg/BasePeCoffLib: Deal with broken debug directories Ard Biesheuvel
@ 2023-05-05 1:54 ` gaoliming
2023-05-05 1:56 ` [edk2-devel] " Michael Kubacki
1 sibling, 0 replies; 4+ messages in thread
From: gaoliming @ 2023-05-05 1:54 UTC (permalink / raw)
To: 'Ard Biesheuvel', devel; +Cc: michael.kubacki
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
Tested-by: Liming Gao <gaoliming@byosoft.com.cn>
> -----邮件原件-----
> 发件人: Ard Biesheuvel <ardb@kernel.org>
> 发送时间: 2023年5月4日 22:48
> 收件人: devel@edk2.groups.io
> 抄送: gaoliming@byosoft.com.cn; michael.kubacki@microsoft.com; Ard
> Biesheuvel <ardb@kernel.org>
> 主题: [PATCH] MdePkg/BasePeCoffLib: Deal with broken debug directories
>
> Older versions of GenFw put the wrong value in the debug directory size
> field in the PE/COFF header: instead of putting the combined size of all
> the entries, it puts the size of the only entry it creates, but adds the
> size of the NB10 payload that the entry points to. This confuses the
> loader now that we started using additional debug directory entries to
> describe DLL characteristics.
>
> GenFw was fixed in commit 60e85a39fe49071, but the binaries that were
> generated with it still need to be supported.
>
> So let's detect this condition, and check whether the size of the debug
> directory is consistent with the NB10 payload: if we should expect
> additional directory entries where we observe the NB10 payload, the size
> field is clearly wrong, and we can break from the loop.
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4425
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> index 4b71176a0c7c2ed0..27f8526370fa3859 100644
> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> @@ -585,6 +585,7 @@ PeCoffLoaderGetImageInfo (
> UINTN Size;
>
> UINTN ReadSize;
>
> UINTN Index;
>
> + UINTN NextIndex;
>
> UINTN DebugDirectoryEntryRva;
>
> UINTN
> DebugDirectoryEntryFileOffset;
>
> UINTN SectionHeaderOffset;
>
> @@ -755,6 +756,19 @@ 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.
>
> + //
>
> + NextIndex = Index + sizeof
> (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>
> + if ((NextIndex < DebugDirectoryEntry->Size) &&
>
> + (DebugEntry.FileOffset ==
> (DebugDirectoryEntryFileOffset + NextIndex))) {
>
> + break;
>
> + }
>
> +
>
> continue;
>
> }
>
>
>
> --
> 2.39.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH] MdePkg/BasePeCoffLib: Deal with broken debug directories
2023-05-04 14:47 [PATCH] MdePkg/BasePeCoffLib: Deal with broken debug directories Ard Biesheuvel
2023-05-05 1:54 ` 回复: " gaoliming
@ 2023-05-05 1:56 ` Michael Kubacki
2023-05-05 8:12 ` Ard Biesheuvel
1 sibling, 1 reply; 4+ messages in thread
From: Michael Kubacki @ 2023-05-05 1:56 UTC (permalink / raw)
To: devel, ardb; +Cc: gaoliming, michael.kubacki
Acked-by: Michael Kubacki <michael.kubacki@microsoft.com>
On 5/4/2023 10:47 AM, Ard Biesheuvel wrote:
> Older versions of GenFw put the wrong value in the debug directory size
> field in the PE/COFF header: instead of putting the combined size of all
> the entries, it puts the size of the only entry it creates, but adds the
> size of the NB10 payload that the entry points to. This confuses the
> loader now that we started using additional debug directory entries to
> describe DLL characteristics.
>
> GenFw was fixed in commit 60e85a39fe49071, but the binaries that were
> generated with it still need to be supported.
>
> So let's detect this condition, and check whether the size of the debug
> directory is consistent with the NB10 payload: if we should expect
> additional directory entries where we observe the NB10 payload, the size
> field is clearly wrong, and we can break from the loop.
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4425
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> index 4b71176a0c7c2ed0..27f8526370fa3859 100644
> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> @@ -585,6 +585,7 @@ PeCoffLoaderGetImageInfo (
> UINTN Size;
>
> UINTN ReadSize;
>
> UINTN Index;
>
> + UINTN NextIndex;
>
> UINTN DebugDirectoryEntryRva;
>
> UINTN DebugDirectoryEntryFileOffset;
>
> UINTN SectionHeaderOffset;
>
> @@ -755,6 +756,19 @@ 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.
>
> + //
>
> + NextIndex = Index + sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>
> + if ((NextIndex < DebugDirectoryEntry->Size) &&
>
> + (DebugEntry.FileOffset == (DebugDirectoryEntryFileOffset + NextIndex))) {
>
> + break;
>
> + }
>
> +
>
> continue;
>
> }
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH] MdePkg/BasePeCoffLib: Deal with broken debug directories
2023-05-05 1:56 ` [edk2-devel] " Michael Kubacki
@ 2023-05-05 8:12 ` Ard Biesheuvel
0 siblings, 0 replies; 4+ messages in thread
From: Ard Biesheuvel @ 2023-05-05 8:12 UTC (permalink / raw)
To: devel, mikuback; +Cc: gaoliming, michael.kubacki
On Fri, 5 May 2023 at 03:56, Michael Kubacki
<mikuback@linux.microsoft.com> wrote:
>
> Acked-by: Michael Kubacki <michael.kubacki@microsoft.com>
>
Merged as #4340
Thanks all
> On 5/4/2023 10:47 AM, Ard Biesheuvel wrote:
> > Older versions of GenFw put the wrong value in the debug directory size
> > field in the PE/COFF header: instead of putting the combined size of all
> > the entries, it puts the size of the only entry it creates, but adds the
> > size of the NB10 payload that the entry points to. This confuses the
> > loader now that we started using additional debug directory entries to
> > describe DLL characteristics.
> >
> > GenFw was fixed in commit 60e85a39fe49071, but the binaries that were
> > generated with it still need to be supported.
> >
> > So let's detect this condition, and check whether the size of the debug
> > directory is consistent with the NB10 payload: if we should expect
> > additional directory entries where we observe the NB10 payload, the size
> > field is clearly wrong, and we can break from the loop.
> >
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4425
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> > MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > index 4b71176a0c7c2ed0..27f8526370fa3859 100644
> > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > @@ -585,6 +585,7 @@ PeCoffLoaderGetImageInfo (
> > UINTN Size;
> >
> > UINTN ReadSize;
> >
> > UINTN Index;
> >
> > + UINTN NextIndex;
> >
> > UINTN DebugDirectoryEntryRva;
> >
> > UINTN DebugDirectoryEntryFileOffset;
> >
> > UINTN SectionHeaderOffset;
> >
> > @@ -755,6 +756,19 @@ 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.
> >
> > + //
> >
> > + NextIndex = Index + sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
> >
> > + if ((NextIndex < DebugDirectoryEntry->Size) &&
> >
> > + (DebugEntry.FileOffset == (DebugDirectoryEntryFileOffset + NextIndex))) {
> >
> > + break;
> >
> > + }
> >
> > +
> >
> > continue;
> >
> > }
> >
> >
> >
>
>
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-05-05 8:12 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-04 14:47 [PATCH] MdePkg/BasePeCoffLib: Deal with broken debug directories Ard Biesheuvel
2023-05-05 1:54 ` 回复: " gaoliming
2023-05-05 1:56 ` [edk2-devel] " Michael Kubacki
2023-05-05 8:12 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox