Re: [edk2-devel] [Proposed Fix][PATCH v1 1/1] MdeModulePkg/DxeCapsuleLibFmp: Fix crash with VirtualAddressMap omitted

["Ard Biesheuvel" <ardb@kernel.org>]

On Tue, 28 Nov 2023 at 16:15, gaoliming via groups.io
<gaoliming=byosoft.com.cn@groups.io> wrote:
>
> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
>

Thanks I'll queue this up.



> > -----邮件原件-----
> > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Ard
> > Biesheuvel
> > 发送时间: 2023年11月23日 16:36
> > 收件人: Nhi Pham <nhi@os.amperecomputing.com>
> > 抄送: devel@edk2.groups.io; gaoliming@byosoft.com.cn;
> > ardb+tianocore@kernel.org
> > 主题: Re: [edk2-devel] [Proposed Fix][PATCH v1 1/1]
> > MdeModulePkg/DxeCapsuleLibFmp: Fix crash with VirtualAddressMap
> > omitted
> >
> > On Thu, 23 Nov 2023 at 05:49, Nhi Pham <nhi@os.amperecomputing.com>
> > wrote:
> > >
> > > If the SetVirtualAddressMap() is not called, mIsVirtualAddrConverted
> > > is FALSE and the kernel crash occurs in IsNestedFmpCapsule() when
> > > executing gBS->LocateProtocol () in the else case.
> > >
> > > To serve the omitted SetVirtualAddressMap() call, we could just check
> > > mEsrtTable presence instead of relying on mIsVirtualAddrConverted.
> > >
> > > Signed-off-by: Nhi Pham <nhi@os.amperecomputing.com>
> >
> > Agreed: SetVirtualAddressMap() is optional, so whether it has been
> > called or not should not be used as a proxy to decide whether or not
> > we are still running in the boot services context.
> >
> > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> >
> >
> > > ---
> > >  MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c     | 17
> > +++++++----------
> > >  MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleRuntime.c |  2 --
> > >  2 files changed, 7 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
> > b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
> > > index 197af267aff3..2433c76a8c6f 100644
> > > --- a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
> > > +++ b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
> > > @@ -41,8 +41,7 @@
> > >  #include <Protocol/FirmwareManagementProgress.h>
> > >  #include <Protocol/DevicePath.h>
> > >
> > > -EFI_SYSTEM_RESOURCE_TABLE  *mEsrtTable             = NULL;
> > > -BOOLEAN                    mIsVirtualAddrConverted = FALSE;
> > > +EFI_SYSTEM_RESOURCE_TABLE  *mEsrtTable = NULL;
> > >
> > >  BOOLEAN    mDxeCapsuleLibEndOfDxe      = FALSE;
> > >  EFI_EVENT  mDxeCapsuleLibEndOfDxeEvent = NULL;
> > > @@ -1394,14 +1393,12 @@ IsNestedFmpCapsule (
> > >    EFI_SYSTEM_RESOURCE_ENTRY  Entry;
> > >
> > >    EsrtGuidFound = FALSE;
> > > -  if (mIsVirtualAddrConverted) {
> > > -    if (mEsrtTable != NULL) {
> > > -      EsrtEntry = (EFI_SYSTEM_RESOURCE_ENTRY *)(mEsrtTable + 1);
> > > -      for (Index = 0; Index < mEsrtTable->FwResourceCount; Index++,
> > EsrtEntry++) {
> > > -        if (CompareGuid (&EsrtEntry->FwClass,
> > &CapsuleHeader->CapsuleGuid)) {
> > > -          EsrtGuidFound = TRUE;
> > > -          break;
> > > -        }
> > > +  if (mEsrtTable != NULL) {
> > > +    EsrtEntry = (EFI_SYSTEM_RESOURCE_ENTRY *)(mEsrtTable + 1);
> > > +    for (Index = 0; Index < mEsrtTable->FwResourceCount; Index++,
> > EsrtEntry++) {
> > > +      if (CompareGuid (&EsrtEntry->FwClass,
> > &CapsuleHeader->CapsuleGuid)) {
> > > +        EsrtGuidFound = TRUE;
> > > +        break;
> > >        }
> > >      }
> > >    } else {
> > > diff --git
> > a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleRuntime.c
> > b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleRuntime.c
> > > index 9ceb44fe4d79..44f30c16c284 100644
> > > --- a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleRuntime.c
> > > +++ b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleRuntime.c
> > > @@ -21,7 +21,6 @@
> > >  #include <Library/MemoryAllocationLib.h>
> > >
> > >  extern EFI_SYSTEM_RESOURCE_TABLE  *mEsrtTable;
> > > -extern BOOLEAN                    mIsVirtualAddrConverted;
> > >  EFI_EVENT
> > mDxeRuntimeCapsuleLibVirtualAddressChangeEvent = NULL;
> > >  EFI_EVENT
> > mDxeRuntimeCapsuleLibReadyToBootEvent          = NULL;
> > >
> > > @@ -40,7 +39,6 @@ DxeCapsuleLibVirtualAddressChangeEvent (
> > >    )
> > >  {
> > >    gRT->ConvertPointer (EFI_OPTIONAL_PTR, (VOID **)&mEsrtTable);
> > > -  mIsVirtualAddrConverted = TRUE;
> > >  }
> > >
> > >  /**
> > > --
> > > 2.25.1
> > >
> >
> >
> >
> >
>
>
>
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111822): https://edk2.groups.io/g/devel/message/111822
Mute This Topic: https://groups.io/mt/102855986/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-