From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web10.51375.1683209771022723897 for ; Thu, 04 May 2023 07:16:11 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=lk7jLO6/; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 769D06347E for ; Thu, 4 May 2023 14:16:10 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id DDE85C433D2 for ; Thu, 4 May 2023 14:16:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1683209769; bh=jQhdDj1QrZijw42+LlgNR78laR6IDQTBgE0ev0/1rTo=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=lk7jLO6/aa31Xlm73hlZcxK4+NwgopLbMHL7AbS6PcQD9dUyMe9P+ufVRjdpqfx0+ g8R9EUm6KRHF5ZJ+RFJWblGgRNhuCI6Mr8pwzGEGZ9K46MWu9LYOKiM4EoGcKUrI/S YsA22gJ2B0jrmwa77nsMZ5b3C+SS1AptCKn3pjJNdhfrTdGzG7BGyk39ZNs9hg2y7K cIviz4VR9mKnmhYIrmeE/rfYp3iJ1ijW4k74x6E8PRR93FzWsohOyQ4WxWWKkbRda9 6PckBP5P/TkMiIbdPsRS6cuYHknZABjVcI2O7eecd0AIqhuLl/74A98GHq0Jm8ZMn0 pUYpTokQNhI/g== Received: by mail-lf1-f54.google.com with SMTP id 2adb3069b0e04-4f13bfe257aso635247e87.3 for ; Thu, 04 May 2023 07:16:09 -0700 (PDT) X-Gm-Message-State: AC+VfDxHddumjo3RF8g34FXcAeK1OJjW3+65JE1AlMfEN7mG4SA3pS6V w7e3MfwedZbLF0TVrXOs8QVKeG/yX2/n70/rS1U= X-Google-Smtp-Source: ACHHUZ5rtHTagWgbnui2UA7H1Oxn4D5VBaDpmIZjQQM2IrMBDhqZofKM/xj47uV9CCPzBbXaHuHxCOyaeHLvamKUL7E= X-Received: by 2002:ac2:548f:0:b0:4dd:afd7:8f1 with SMTP id t15-20020ac2548f000000b004ddafd708f1mr1784986lfk.52.1683209767912; Thu, 04 May 2023 07:16:07 -0700 (PDT) MIME-Version: 1.0 References: <20230427061714.510-1-gaoliming@byosoft.com.cn> <001501d97997$07241c90$156c55b0$@byosoft.com.cn> <033f01d97e5c$fb81f770$f285e650$@byosoft.com.cn> In-Reply-To: <033f01d97e5c$fb81f770$f285e650$@byosoft.com.cn> From: "Ard Biesheuvel" Date: Thu, 4 May 2023 16:15:56 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH] MdePkg BasePeCoffLib: Ignore the debug entry read error after it is found To: devel@edk2.groups.io, gaoliming@byosoft.com.cn Cc: Michael Kubacki Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 4 May 2023 at 09:49, gaoliming via groups.io wrote: > > Ard: > > > -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- > > =E5=8F=91=E4=BB=B6=E4=BA=BA: devel@edk2.groups.io =E4=BB=A3=E8=A1=A8 Ard > > Biesheuvel > > =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2023=E5=B9=B44=E6=9C=8828=E6=97= =A5 17:21 > > =E6=94=B6=E4=BB=B6=E4=BA=BA: devel@edk2.groups.io; gaoliming@byosoft.co= m.cn > > =E6=8A=84=E9=80=81: Michael Kubacki > > =E4=B8=BB=E9=A2=98: Re: [edk2-devel] [PATCH] MdePkg BasePeCoffLib: Igno= re the debug > > entry read error after it is found > > > > On Fri, 28 Apr 2023 at 07:02, gaoliming via groups.io > > wrote: > > > > > > Ard: > > > > > > > -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- > > > > =E5=8F=91=E4=BB=B6=E4=BA=BA: devel@edk2.groups.io =E4=BB=A3=E8=A1=A8 Ard > > > > Biesheuvel > > > > =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2023=E5=B9=B44=E6=9C=8827=E6= =97=A5 22:47 > > > > =E6=94=B6=E4=BB=B6=E4=BA=BA: devel@edk2.groups.io; gaoliming@byosof= t.com.cn > > > > =E6=8A=84=E9=80=81: Michael Kubacki > > > > =E4=B8=BB=E9=A2=98: 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 > > > > wrote: > > > > > > > > > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4425 > > > > > > > > > > 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 > > > > > Cc: Ard Biesheuvel > > > > > Cc: Michael Kubacki > > > > > --- > > > > > 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 !=3D ReadSize)) { > > > > > + if ((ImageContext->DebugDirectoryEntryRva =3D=3D 0) && > > > > (RETURN_ERROR (Status) || (Size !=3D ReadSize))) { > > > > > > > > I'm not sure I understand how this fixes anything. Why is the > > > > condition 'RVA =3D=3D 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 +=3D 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 debu= g > > > > directory > > > > + // entry's size field. If this is the case, no other r= elevant > > > > + // directory entries can exist, and we can terminate h= ere. > > > > + // > > > > + if (DebugEntry.FileOffset =3D=3D > > > > (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 chanc= e > > 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 + si= zeof (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.