Re: [edk2-devel] [PATCH v2] MdeModulePkg/DxeCapsuleLibFmp: avoid ESRT accesses at runtime

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ard
> Biesheuvel
> Sent: Saturday, April 20, 2019 6:35 PM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A; Wang, Jian J; Kinney, Michael D; Ard Biesheuvel
> Subject: [edk2-devel] [PATCH v2] MdeModulePkg/DxeCapsuleLibFmp: avoid
> ESRT accesses at runtime

Hello Ard,

It seems to me v2 patch makes a copy of the ESRT for runtime usage (rather
than avoid using ESRT), so the title of the commit may need update as
well.

Could you help to update the patch subject when you push the change?

Other than that, the patch looks good to me. But I would like to see if
Mike has additional comments on this:

Acked-by: Hao Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu

> 
> The DxeCapsuleLibFmp code accesses the ESRT table to decide whether
> a certain capsule is an FMP capsule. Since the UEFI spec mandates
> that the ESRT resides in EfiBootServicesData memory, this results
> in problems at OS runtime, since the firmware implementation itself
> cannot access memory that has not been virtually remapped.
> 
> So let's take a private copy of the ESRT at ReadyToBoot, and store
> it in EfiRuntimeServicesData memory. The ESRT's size is order 10s
> of bytes so the memory footprint is going to be negligigble.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> v2: copy the whole table instead of just the list of GUIDs
> 
> Still build tested only.
> 
>  MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleRuntime.c      | 58
> ++++++++++++++++----
>  MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf |  1 +
>  2 files changed, 49 insertions(+), 10 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleRuntime.c
> b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleRuntime.c
> index 602921d13c06..9e0327afb53b 100644
> --- a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleRuntime.c
> +++ b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleRuntime.c
> @@ -23,6 +23,7 @@
>  extern EFI_SYSTEM_RESOURCE_TABLE *mEsrtTable;
>  extern BOOLEAN                   mIsVirtualAddrConverted;
>  EFI_EVENT                 mDxeRuntimeCapsuleLibVirtualAddressChangeEvent  =
> NULL;
> +EFI_EVENT                 mDxeRuntimeCapsuleLibReadyToBootEvent  = NULL;
> 
>  /**
>    Convert EsrtTable physical address to virtual address.
> @@ -38,8 +39,28 @@ DxeCapsuleLibVirtualAddressChangeEvent (
>    IN  VOID        *Context
>    )
>  {
> -  UINTN                    Index;
> -  EFI_CONFIGURATION_TABLE  *ConfigEntry;
> +  gRT->ConvertPointer (EFI_OPTIONAL_PTR, (VOID **)&mEsrtTable);
> +  mIsVirtualAddrConverted = TRUE;
> +}
> +
> +/**
> +  Notify function for event group EFI_EVENT_GROUP_READY_TO_BOOT.
> +
> +  @param[in]  Event   The Event that is being processed.
> +  @param[in]  Context The Event Context.
> +
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +DxeCapsuleLibReadyToBootEventNotify (
> +  IN EFI_EVENT        Event,
> +  IN VOID             *Context
> +  )
> +{
> +  UINTN                       Index;
> +  EFI_CONFIGURATION_TABLE     *ConfigEntry;
> +  EFI_SYSTEM_RESOURCE_TABLE   *EsrtTable;
> 
>    //
>    // Get Esrt table first
> @@ -59,16 +80,14 @@ DxeCapsuleLibVirtualAddressChangeEvent (
>      //
>      // Search Esrt to check given capsule is qualified
>      //
> -    mEsrtTable = (EFI_SYSTEM_RESOURCE_TABLE *) ConfigEntry->VendorTable;
> +    EsrtTable = (EFI_SYSTEM_RESOURCE_TABLE *) ConfigEntry->VendorTable;
> 
> -    //
> -    // Update protocol pointer to Esrt Table.
> -    //
> -    gRT->ConvertPointer (0x00, (VOID**) &(mEsrtTable));
> +    mEsrtTable = AllocateRuntimeCopyPool (
> +                   sizeof (EFI_SYSTEM_RESOURCE_TABLE) +
> +                   EsrtTable->FwResourceCount * sizeof
> (EFI_SYSTEM_RESOURCE_ENTRY),
> +                   EsrtTable);
> +    ASSERT (mEsrtTable != NULL);
>    }
> -
> -  mIsVirtualAddrConverted = TRUE;
> -
>  }
> 
>  /**
> @@ -101,6 +120,19 @@ DxeRuntimeCapsuleLibConstructor (
>                    );
>    ASSERT_EFI_ERROR (Status);
> 
> +  //
> +  // Register notify function to cache the FMP capsule GUIDs at ReadyToBoot.
> +  //
> +  Status = gBS->CreateEventEx (
> +                  EVT_NOTIFY_SIGNAL,
> +                  TPL_CALLBACK,
> +                  DxeCapsuleLibReadyToBootEventNotify,
> +                  NULL,
> +                  &gEfiEventReadyToBootGuid,
> +                  &mDxeRuntimeCapsuleLibReadyToBootEvent
> +                  );
> +  ASSERT_EFI_ERROR (Status);
> +
>    return EFI_SUCCESS;
>  }
> 
> @@ -127,5 +159,11 @@ DxeRuntimeCapsuleLibDestructor (
>    Status = gBS->CloseEvent
> (mDxeRuntimeCapsuleLibVirtualAddressChangeEvent);
>    ASSERT_EFI_ERROR (Status);
> 
> +  //
> +  // Close the ReadyToBoot event.
> +  //
> +  Status = gBS->CloseEvent (mDxeRuntimeCapsuleLibReadyToBootEvent);
> +  ASSERT_EFI_ERROR (Status);
> +
>    return EFI_SUCCESS;
>  }
> diff --git
> a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
> b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
> index 700d0d5dcddd..2c93e6870023 100644
> --- a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
> +++ b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
> @@ -66,6 +66,7 @@
>    gEfiCapsuleReportGuid
>    gEfiCapsuleVendorGuid                   ## SOMETIMES_CONSUMES ##
> Variable:L"CapsuleUpdateData"
>    gEfiEndOfDxeEventGroupGuid              ## CONSUMES ## Event
> +  gEfiEventReadyToBootGuid                ## CONSUMES ## Event
>    gEfiEventVirtualAddressChangeGuid       ## CONSUMES ## Event
> 
>  [Depex]
> --
> 2.20.1
> 
> 
>