[edk2-devel] [PATCH] MdePkg BasePeCoffLib: Ignore the debug entry read error after it is found

[Ard Biesheuvel]

On Thu, 4 May 2023 at 09:49, gaoliming via groups.io
<gaoliming=byosoft.com.cn at groups.io> wrote:
>
> Ard:
>
> > -----邮件原件-----
> > 发件人: devel at edk2.groups.io <devel at edk2.groups.io> 代表 Ard
> > Biesheuvel
> > 发送时间: 2023年4月28日 17:21
> > 收件人: devel at edk2.groups.io; gaoliming at byosoft.com.cn
> > 抄送: Michael Kubacki <michael.kubacki at 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 at groups.io> wrote:
> > >
> > > Ard:
> > >
> > > > -----邮件原件-----
> > > > 发件人: devel at edk2.groups.io <devel at edk2.groups.io> 代表 Ard
> > > > Biesheuvel
> > > > 发送时间: 2023年4月27日 22:47
> > > > 收件人: devel at edk2.groups.io; gaoliming at byosoft.com.cn
> > > > 抄送: Michael Kubacki <michael.kubacki at 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 at 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 at byosoft.com.cn>
> > > > > Cc: Ard Biesheuvel <ardb at kernel.org>
> > > > > Cc: Michael Kubacki <michael.kubacki at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-