[PATCH resend] MdeModulePkg/EsrtDxe: allocate ESRT table from RtServicesData memory

[PATCH resend] MdeModulePkg/EsrtDxe: allocate ESRT table from RtServicesData memory

[Ard Biesheuvel]

Given that the firmware itself may access the ESRT table when the OS
invokes the UpdateCapsule () boot service, it requires a virtual mapping
and so it needs to be allocated from RtServicesData memory.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Universal/EsrtDxe/EsrtDxe.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/EsrtDxe/EsrtDxe.c b/MdeModulePkg/Universal/EsrtDxe/EsrtDxe.c
index a386a9770583..1741cbe8f2b5 100644
--- a/MdeModulePkg/Universal/EsrtDxe/EsrtDxe.c
+++ b/MdeModulePkg/Universal/EsrtDxe/EsrtDxe.c
@@ -571,7 +571,8 @@ EsrtReadyToBootEventNotify (
     goto EXIT;
   }
 
-  EsrtTable = AllocatePool(sizeof(EFI_SYSTEM_RESOURCE_TABLE) + NonFmpRepositorySize + FmpRepositorySize);
+  EsrtTable = AllocateRuntimePool (sizeof(EFI_SYSTEM_RESOURCE_TABLE) +
+                                   NonFmpRepositorySize + FmpRepositorySize);
   if (EsrtTable == NULL) {
     DEBUG ((EFI_D_ERROR, "Esrt table memory allocation failure\n"));
     goto EXIT;
-- 
2.20.1

Re: [edk2-devel] [PATCH resend] MdeModulePkg/EsrtDxe: allocate ESRT table from RtServicesData memory

[Michael D Kinney]

Hi Ard,

The UEFI Specification Section 23.3 says:

	"The ESRT shall be stored in memory of type EfiBootServicesData."

If an RT driver needs ESRT info after ExitBootServices(), then the
RT driver should collect that information before ExitBootServices().

Best regards,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io
> [mailto:devel@edk2.groups.io] On Behalf Of Ard
> Biesheuvel
> Sent: Friday, April 19, 2019 7:13 AM
> To: devel@edk2.groups.io
> Cc: ming.huang@linaro.org; Wu, Hao A
> <hao.a.wu@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: [edk2-devel] [PATCH resend]
> MdeModulePkg/EsrtDxe: allocate ESRT table from
> RtServicesData memory
> 
> Given that the firmware itself may access the ESRT
> table when the OS
> invokes the UpdateCapsule () boot service, it requires
> a virtual mapping
> and so it needs to be allocated from RtServicesData
> memory.
> 
> Signed-off-by: Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> ---
>  MdeModulePkg/Universal/EsrtDxe/EsrtDxe.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Universal/EsrtDxe/EsrtDxe.c
> b/MdeModulePkg/Universal/EsrtDxe/EsrtDxe.c
> index a386a9770583..1741cbe8f2b5 100644
> --- a/MdeModulePkg/Universal/EsrtDxe/EsrtDxe.c
> +++ b/MdeModulePkg/Universal/EsrtDxe/EsrtDxe.c
> @@ -571,7 +571,8 @@ EsrtReadyToBootEventNotify (
>      goto EXIT;
>    }
> 
> -  EsrtTable =
> AllocatePool(sizeof(EFI_SYSTEM_RESOURCE_TABLE) +
> NonFmpRepositorySize + FmpRepositorySize);
> +  EsrtTable = AllocateRuntimePool
> (sizeof(EFI_SYSTEM_RESOURCE_TABLE) +
> +
> NonFmpRepositorySize + FmpRepositorySize);
>    if (EsrtTable == NULL) {
>      DEBUG ((EFI_D_ERROR, "Esrt table memory allocation
> failure\n"));
>      goto EXIT;
> --
> 2.20.1
> 
> 
> 

Re: [edk2-devel] [PATCH resend] MdeModulePkg/EsrtDxe: allocate ESRT table from RtServicesData memory

[Ard Biesheuvel]

On Fri, 19 Apr 2019 at 19:36, Michael D Kinney
<michael.d.kinney@intel.com> wrote:
>
> Hi Ard,
>
> The UEFI Specification Section 23.3 says:
>
>         "The ESRT shall be stored in memory of type EfiBootServicesData."
>
> If an RT driver needs ESRT info after ExitBootServices(), then the
> RT driver should collect that information before ExitBootServices().
>

But that means that before EBS(), we will have to make a copy of the
ESRT into runtime services data, and we will end up using twice as
much memory - one copy in boot services data for the OS, and one copy
in runtime services data for the firmware itself. I understand the
reasoning behind preferring boot over runtime, so that the OS is not
burdened with it if it does not care. But it this case, the RT
services data will be allocated regardless, and so we end up using
less memory if we expose that same copy to the OS.




>
> > -----Original Message-----
> > From: devel@edk2.groups.io
> > [mailto:devel@edk2.groups.io] On Behalf Of Ard
> > Biesheuvel
> > Sent: Friday, April 19, 2019 7:13 AM
> > To: devel@edk2.groups.io
> > Cc: ming.huang@linaro.org; Wu, Hao A
> > <hao.a.wu@intel.com>; Wang, Jian J
> > <jian.j.wang@intel.com>; Ard Biesheuvel
> > <ard.biesheuvel@linaro.org>
> > Subject: [edk2-devel] [PATCH resend]
> > MdeModulePkg/EsrtDxe: allocate ESRT table from
> > RtServicesData memory
> >
> > Given that the firmware itself may access the ESRT
> > table when the OS
> > invokes the UpdateCapsule () boot service, it requires
> > a virtual mapping
> > and so it needs to be allocated from RtServicesData
> > memory.
> >
> > Signed-off-by: Ard Biesheuvel
> > <ard.biesheuvel@linaro.org>
> > ---
> >  MdeModulePkg/Universal/EsrtDxe/EsrtDxe.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Universal/EsrtDxe/EsrtDxe.c
> > b/MdeModulePkg/Universal/EsrtDxe/EsrtDxe.c
> > index a386a9770583..1741cbe8f2b5 100644
> > --- a/MdeModulePkg/Universal/EsrtDxe/EsrtDxe.c
> > +++ b/MdeModulePkg/Universal/EsrtDxe/EsrtDxe.c
> > @@ -571,7 +571,8 @@ EsrtReadyToBootEventNotify (
> >      goto EXIT;
> >    }
> >
> > -  EsrtTable =
> > AllocatePool(sizeof(EFI_SYSTEM_RESOURCE_TABLE) +
> > NonFmpRepositorySize + FmpRepositorySize);
> > +  EsrtTable = AllocateRuntimePool
> > (sizeof(EFI_SYSTEM_RESOURCE_TABLE) +
> > +
> > NonFmpRepositorySize + FmpRepositorySize);
> >    if (EsrtTable == NULL) {
> >      DEBUG ((EFI_D_ERROR, "Esrt table memory allocation
> > failure\n"));
> >      goto EXIT;
> > --
> > 2.20.1
> >
> >
> >
>
>
> 
>

Re: [edk2-devel] [PATCH resend] MdeModulePkg/EsrtDxe: allocate ESRT table from RtServicesData memory

[Ard Biesheuvel]

On Fri, 19 Apr 2019 at 19:42, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Fri, 19 Apr 2019 at 19:36, Michael D Kinney
> <michael.d.kinney@intel.com> wrote:
> >
> > Hi Ard,
> >
> > The UEFI Specification Section 23.3 says:
> >
> >         "The ESRT shall be stored in memory of type EfiBootServicesData."
> >
> > If an RT driver needs ESRT info after ExitBootServices(), then the
> > RT driver should collect that information before ExitBootServices().
> >
>
> But that means that before EBS(), we will have to make a copy of the
> ESRT into runtime services data, and we will end up using twice as
> much memory - one copy in boot services data for the OS, and one copy
> in runtime services data for the firmware itself. I understand the
> reasoning behind preferring boot over runtime, so that the OS is not
> burdened with it if it does not care. But it this case, the RT
> services data will be allocated regardless, and so we end up using
> less memory if we expose that same copy to the OS.
>

Actually, DxeCapsuleLibVirtualAddressChangeEvent() in DxeCapsuleLibFmp
updates the ESRT pointer to virtual, so this code is clearly based on
the assumption that the ESRT resides in memory that is covered by a
runtime VA mapping.

Could we instead clarify the UEFI spec to something like

"The ESRT shall be stored in memory of type EfiBootServicesData unless
the runtime services implementations themselves need to access it at
OS runtime, in which case it may be stored in EfiRuntimeServicesData."

Re: [edk2-devel] [PATCH resend] MdeModulePkg/EsrtDxe: allocate ESRT table from RtServicesData memory

[Michael D Kinney]

Ard,

Where is the use of ESRT at runtime?

The SetVa conversion of the ESRT table may be an old artifact
that we should consider removing.

Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io
> [mailto:devel@edk2.groups.io] On Behalf Of Ard
> Biesheuvel
> Sent: Friday, April 19, 2019 10:54 AM
> To: edk2-devel-groups-io <devel@edk2.groups.io>;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: ming.huang@linaro.org; Wu, Hao A
> <hao.a.wu@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>
> Subject: Re: [edk2-devel] [PATCH resend]
> MdeModulePkg/EsrtDxe: allocate ESRT table from
> RtServicesData memory
> 
> On Fri, 19 Apr 2019 at 19:42, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> >
> > On Fri, 19 Apr 2019 at 19:36, Michael D Kinney
> > <michael.d.kinney@intel.com> wrote:
> > >
> > > Hi Ard,
> > >
> > > The UEFI Specification Section 23.3 says:
> > >
> > >         "The ESRT shall be stored in memory of type
> EfiBootServicesData."
> > >
> > > If an RT driver needs ESRT info after
> ExitBootServices(), then the
> > > RT driver should collect that information before
> ExitBootServices().
> > >
> >
> > But that means that before EBS(), we will have to
> make a copy of the
> > ESRT into runtime services data, and we will end up
> using twice as
> > much memory - one copy in boot services data for the
> OS, and one copy
> > in runtime services data for the firmware itself. I
> understand the
> > reasoning behind preferring boot over runtime, so
> that the OS is not
> > burdened with it if it does not care. But it this
> case, the RT
> > services data will be allocated regardless, and so we
> end up using
> > less memory if we expose that same copy to the OS.
> >
> 
> Actually, DxeCapsuleLibVirtualAddressChangeEvent() in
> DxeCapsuleLibFmp
> updates the ESRT pointer to virtual, so this code is
> clearly based on
> the assumption that the ESRT resides in memory that is
> covered by a
> runtime VA mapping.
> 
> Could we instead clarify the UEFI spec to something
> like
> 
> "The ESRT shall be stored in memory of type
> EfiBootServicesData unless
> the runtime services implementations themselves need to
> access it at
> OS runtime, in which case it may be stored in
> EfiRuntimeServicesData."
> 
> 

Re: [edk2-devel] [PATCH resend] MdeModulePkg/EsrtDxe: allocate ESRT table from RtServicesData memory

[Ard Biesheuvel]

On Fri, 19 Apr 2019 at 20:08, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> Ard,
>
> Where is the use of ESRT at runtime?
>

DxeCapsuleLibFmp is invoked at runtime by the UpdateCapsule() and
QueryCapsuleCapabilities() code.

The Canonical FWTS crashes on this runtime service on some ARM and x86
systems due to this issue, hence the patch.


> The SetVa conversion of the ESRT table may be an old artifact
> that we should consider removing.
>

No, it is definitely live code.


>
> > -----Original Message-----
> > From: devel@edk2.groups.io
> > [mailto:devel@edk2.groups.io] On Behalf Of Ard
> > Biesheuvel
> > Sent: Friday, April 19, 2019 10:54 AM
> > To: edk2-devel-groups-io <devel@edk2.groups.io>;
> > Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: ming.huang@linaro.org; Wu, Hao A
> > <hao.a.wu@intel.com>; Wang, Jian J
> > <jian.j.wang@intel.com>
> > Subject: Re: [edk2-devel] [PATCH resend]
> > MdeModulePkg/EsrtDxe: allocate ESRT table from
> > RtServicesData memory
> >
> > On Fri, 19 Apr 2019 at 19:42, Ard Biesheuvel
> > <ard.biesheuvel@linaro.org> wrote:
> > >
> > > On Fri, 19 Apr 2019 at 19:36, Michael D Kinney
> > > <michael.d.kinney@intel.com> wrote:
> > > >
> > > > Hi Ard,
> > > >
> > > > The UEFI Specification Section 23.3 says:
> > > >
> > > >         "The ESRT shall be stored in memory of type
> > EfiBootServicesData."
> > > >
> > > > If an RT driver needs ESRT info after
> > ExitBootServices(), then the
> > > > RT driver should collect that information before
> > ExitBootServices().
> > > >
> > >
> > > But that means that before EBS(), we will have to
> > make a copy of the
> > > ESRT into runtime services data, and we will end up
> > using twice as
> > > much memory - one copy in boot services data for the
> > OS, and one copy
> > > in runtime services data for the firmware itself. I
> > understand the
> > > reasoning behind preferring boot over runtime, so
> > that the OS is not
> > > burdened with it if it does not care. But it this
> > case, the RT
> > > services data will be allocated regardless, and so we
> > end up using
> > > less memory if we expose that same copy to the OS.
> > >
> >
> > Actually, DxeCapsuleLibVirtualAddressChangeEvent() in
> > DxeCapsuleLibFmp
> > updates the ESRT pointer to virtual, so this code is
> > clearly based on
> > the assumption that the ESRT resides in memory that is
> > covered by a
> > runtime VA mapping.
> >
> > Could we instead clarify the UEFI spec to something
> > like
> >
> > "The ESRT shall be stored in memory of type
> > EfiBootServicesData unless
> > the runtime services implementations themselves need to
> > access it at
> > OS runtime, in which case it may be stored in
> > EfiRuntimeServicesData."
> >
> > 
>

Re: [edk2-devel] [PATCH resend] MdeModulePkg/EsrtDxe: allocate ESRT table from RtServicesData memory

[Michael D Kinney]

Ard,

Let's see if we can remove the ESRT access from those
paths.  That would be the better fix.

Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io
> [mailto:devel@edk2.groups.io] On Behalf Of Ard
> Biesheuvel
> Sent: Friday, April 19, 2019 11:11 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io; ming.huang@linaro.org; Wu,
> Hao A <hao.a.wu@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>
> Subject: Re: [edk2-devel] [PATCH resend]
> MdeModulePkg/EsrtDxe: allocate ESRT table from
> RtServicesData memory
> 
> On Fri, 19 Apr 2019 at 20:08, Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
> >
> > Ard,
> >
> > Where is the use of ESRT at runtime?
> >
> 
> DxeCapsuleLibFmp is invoked at runtime by the
> UpdateCapsule() and
> QueryCapsuleCapabilities() code.
> 
> The Canonical FWTS crashes on this runtime service on
> some ARM and x86
> systems due to this issue, hence the patch.
> 
> 
> > The SetVa conversion of the ESRT table may be an old
> artifact
> > that we should consider removing.
> >
> 
> No, it is definitely live code.
> 
> 
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io
> > > [mailto:devel@edk2.groups.io] On Behalf Of Ard
> > > Biesheuvel
> > > Sent: Friday, April 19, 2019 10:54 AM
> > > To: edk2-devel-groups-io <devel@edk2.groups.io>;
> > > Kinney, Michael D <michael.d.kinney@intel.com>
> > > Cc: ming.huang@linaro.org; Wu, Hao A
> > > <hao.a.wu@intel.com>; Wang, Jian J
> > > <jian.j.wang@intel.com>
> > > Subject: Re: [edk2-devel] [PATCH resend]
> > > MdeModulePkg/EsrtDxe: allocate ESRT table from
> > > RtServicesData memory
> > >
> > > On Fri, 19 Apr 2019 at 19:42, Ard Biesheuvel
> > > <ard.biesheuvel@linaro.org> wrote:
> > > >
> > > > On Fri, 19 Apr 2019 at 19:36, Michael D Kinney
> > > > <michael.d.kinney@intel.com> wrote:
> > > > >
> > > > > Hi Ard,
> > > > >
> > > > > The UEFI Specification Section 23.3 says:
> > > > >
> > > > >         "The ESRT shall be stored in memory of
> type
> > > EfiBootServicesData."
> > > > >
> > > > > If an RT driver needs ESRT info after
> > > ExitBootServices(), then the
> > > > > RT driver should collect that information
> before
> > > ExitBootServices().
> > > > >
> > > >
> > > > But that means that before EBS(), we will have to
> > > make a copy of the
> > > > ESRT into runtime services data, and we will end
> up
> > > using twice as
> > > > much memory - one copy in boot services data for
> the
> > > OS, and one copy
> > > > in runtime services data for the firmware itself.
> I
> > > understand the
> > > > reasoning behind preferring boot over runtime, so
> > > that the OS is not
> > > > burdened with it if it does not care. But it this
> > > case, the RT
> > > > services data will be allocated regardless, and
> so we
> > > end up using
> > > > less memory if we expose that same copy to the
> OS.
> > > >
> > >
> > > Actually, DxeCapsuleLibVirtualAddressChangeEvent()
> in
> > > DxeCapsuleLibFmp
> > > updates the ESRT pointer to virtual, so this code
> is
> > > clearly based on
> > > the assumption that the ESRT resides in memory that
> is
> > > covered by a
> > > runtime VA mapping.
> > >
> > > Could we instead clarify the UEFI spec to something
> > > like
> > >
> > > "The ESRT shall be stored in memory of type
> > > EfiBootServicesData unless
> > > the runtime services implementations themselves
> need to
> > > access it at
> > > OS runtime, in which case it may be stored in
> > > EfiRuntimeServicesData."
> > >
> > >
> >
> 
> 

Re: [edk2-devel] [PATCH resend] MdeModulePkg/EsrtDxe: allocate ESRT table from RtServicesData memory

[Ard Biesheuvel]

On Fri, 19 Apr 2019 at 20:23, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> Ard,
>
> Let's see if we can remove the ESRT access from those
> paths.  That would be the better fix.
>

I am not that familiar with this code, but it seems that the only
reason we access the ESRT at runtime is to check whether a capsule is
a nested FMP capsule, where the outer GUID is cross referenced against
the ESRT before checking whether the inner GUID is the FMP guid.

Could we relax this check? FMP capsule can only be dispatched across a
reboot anyway, and so the runtime capsule handling could accept any
capsule that has an inner FMP guid.

If not, it means we do need to read the ESRT at runtime, in which case
my patch is the simplest solution. We would have to clarify the spec
though.


> > -----Original Message-----
> > From: devel@edk2.groups.io
> > [mailto:devel@edk2.groups.io] On Behalf Of Ard
> > Biesheuvel
> > Sent: Friday, April 19, 2019 11:11 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: devel@edk2.groups.io; ming.huang@linaro.org; Wu,
> > Hao A <hao.a.wu@intel.com>; Wang, Jian J
> > <jian.j.wang@intel.com>
> > Subject: Re: [edk2-devel] [PATCH resend]
> > MdeModulePkg/EsrtDxe: allocate ESRT table from
> > RtServicesData memory
> >
> > On Fri, 19 Apr 2019 at 20:08, Kinney, Michael D
> > <michael.d.kinney@intel.com> wrote:
> > >
> > > Ard,
> > >
> > > Where is the use of ESRT at runtime?
> > >
> >
> > DxeCapsuleLibFmp is invoked at runtime by the
> > UpdateCapsule() and
> > QueryCapsuleCapabilities() code.
> >
> > The Canonical FWTS crashes on this runtime service on
> > some ARM and x86
> > systems due to this issue, hence the patch.
> >
> >
> > > The SetVa conversion of the ESRT table may be an old
> > artifact
> > > that we should consider removing.
> > >
> >
> > No, it is definitely live code.
> >
> >
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io
> > > > [mailto:devel@edk2.groups.io] On Behalf Of Ard
> > > > Biesheuvel
> > > > Sent: Friday, April 19, 2019 10:54 AM
> > > > To: edk2-devel-groups-io <devel@edk2.groups.io>;
> > > > Kinney, Michael D <michael.d.kinney@intel.com>
> > > > Cc: ming.huang@linaro.org; Wu, Hao A
> > > > <hao.a.wu@intel.com>; Wang, Jian J
> > > > <jian.j.wang@intel.com>
> > > > Subject: Re: [edk2-devel] [PATCH resend]
> > > > MdeModulePkg/EsrtDxe: allocate ESRT table from
> > > > RtServicesData memory
> > > >
> > > > On Fri, 19 Apr 2019 at 19:42, Ard Biesheuvel
> > > > <ard.biesheuvel@linaro.org> wrote:
> > > > >
> > > > > On Fri, 19 Apr 2019 at 19:36, Michael D Kinney
> > > > > <michael.d.kinney@intel.com> wrote:
> > > > > >
> > > > > > Hi Ard,
> > > > > >
> > > > > > The UEFI Specification Section 23.3 says:
> > > > > >
> > > > > >         "The ESRT shall be stored in memory of
> > type
> > > > EfiBootServicesData."
> > > > > >
> > > > > > If an RT driver needs ESRT info after
> > > > ExitBootServices(), then the
> > > > > > RT driver should collect that information
> > before
> > > > ExitBootServices().
> > > > > >
> > > > >
> > > > > But that means that before EBS(), we will have to
> > > > make a copy of the
> > > > > ESRT into runtime services data, and we will end
> > up
> > > > using twice as
> > > > > much memory - one copy in boot services data for
> > the
> > > > OS, and one copy
> > > > > in runtime services data for the firmware itself.
> > I
> > > > understand the
> > > > > reasoning behind preferring boot over runtime, so
> > > > that the OS is not
> > > > > burdened with it if it does not care. But it this
> > > > case, the RT
> > > > > services data will be allocated regardless, and
> > so we
> > > > end up using
> > > > > less memory if we expose that same copy to the
> > OS.
> > > > >
> > > >
> > > > Actually, DxeCapsuleLibVirtualAddressChangeEvent()
> > in
> > > > DxeCapsuleLibFmp
> > > > updates the ESRT pointer to virtual, so this code
> > is
> > > > clearly based on
> > > > the assumption that the ESRT resides in memory that
> > is
> > > > covered by a
> > > > runtime VA mapping.
> > > >
> > > > Could we instead clarify the UEFI spec to something
> > > > like
> > > >
> > > > "The ESRT shall be stored in memory of type
> > > > EfiBootServicesData unless
> > > > the runtime services implementations themselves
> > need to
> > > > access it at
> > > > OS runtime, in which case it may be stored in
> > > > EfiRuntimeServicesData."
> > > >
> > > >
> > >
> >
> > 
>

Re: [edk2-devel] [PATCH resend] MdeModulePkg/EsrtDxe: allocate ESRT table from RtServicesData memory

[Ard Biesheuvel]

On Fri, 19 Apr 2019 at 20:38, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Fri, 19 Apr 2019 at 20:23, Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
> >
> > Ard,
> >
> > Let's see if we can remove the ESRT access from those
> > paths.  That would be the better fix.
> >
>
> I am not that familiar with this code, but it seems that the only
> reason we access the ESRT at runtime is to check whether a capsule is
> a nested FMP capsule, where the outer GUID is cross referenced against
> the ESRT before checking whether the inner GUID is the FMP guid.
>
> Could we relax this check? FMP capsule can only be dispatched across a
> reboot anyway, and so the runtime capsule handling could accept any
> capsule that has an inner FMP guid.
>
> If not, it means we do need to read the ESRT at runtime, in which case
> my patch is the simplest solution. We would have to clarify the spec
> though.
>

Actually, I might be able to cache just the list of GUIDs at
ReadyToBoot, so we can do the comparisons at runtime.

Re: [edk2-devel] [PATCH resend] MdeModulePkg/EsrtDxe: allocate ESRT table from RtServicesData memory

[Michael D Kinney]

Ard,

I think skipping the nested check at runtime is a good
idea.  No need for ESRT at RT and no need to cache GUIDs
at EBS.

Mike

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Friday, April 19, 2019 11:43 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io; ming.huang@linaro.org; Wu,
> Hao A <hao.a.wu@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>
> Subject: Re: [edk2-devel] [PATCH resend]
> MdeModulePkg/EsrtDxe: allocate ESRT table from
> RtServicesData memory
> 
> On Fri, 19 Apr 2019 at 20:38, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> >
> > On Fri, 19 Apr 2019 at 20:23, Kinney, Michael D
> > <michael.d.kinney@intel.com> wrote:
> > >
> > > Ard,
> > >
> > > Let's see if we can remove the ESRT access from
> those
> > > paths.  That would be the better fix.
> > >
> >
> > I am not that familiar with this code, but it seems
> that the only
> > reason we access the ESRT at runtime is to check
> whether a capsule is
> > a nested FMP capsule, where the outer GUID is cross
> referenced against
> > the ESRT before checking whether the inner GUID is
> the FMP guid.
> >
> > Could we relax this check? FMP capsule can only be
> dispatched across a
> > reboot anyway, and so the runtime capsule handling
> could accept any
> > capsule that has an inner FMP guid.
> >
> > If not, it means we do need to read the ESRT at
> runtime, in which case
> > my patch is the simplest solution. We would have to
> clarify the spec
> > though.
> >
> 
> Actually, I might be able to cache just the list of
> GUIDs at
> ReadyToBoot, so we can do the comparisons at runtime.

Re: [edk2-devel] [PATCH resend] MdeModulePkg/EsrtDxe: allocate ESRT table from RtServicesData memory

[Michael D Kinney]

Ard,

Saw your patch to cache the GUID table.  The ESRT Table
is not that much bigger, so the algorithm may be simpler
if you just make a copy of the ESRT table with the 
active entries. 

* 16+24 bytes per ESRT entry.
* 16 bytes/entry for just the GUID.

The only advantage of checking against the ESRT GUID at
RT is to reject capsules that will be rejected when the
capsule is processed later.  This can prevent extra reboots
if an OS agent is sending capsules that do not really 
apply to the current system.  I expect OS agent to only 
send capsules that are in the ESRT.

Mike

> -----Original Message-----
> From: Kinney, Michael D
> Sent: Friday, April 19, 2019 12:46 PM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io; ming.huang@linaro.org; Wu,
> Hao A <hao.a.wu@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>
> Subject: RE: [edk2-devel] [PATCH resend]
> MdeModulePkg/EsrtDxe: allocate ESRT table from
> RtServicesData memory
> 
> Ard,
> 
> I think skipping the nested check at runtime is a good
> idea.  No need for ESRT at RT and no need to cache
> GUIDs
> at EBS.
> 
> Mike
> 
> > -----Original Message-----
> > From: Ard Biesheuvel
> [mailto:ard.biesheuvel@linaro.org]
> > Sent: Friday, April 19, 2019 11:43 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: devel@edk2.groups.io; ming.huang@linaro.org; Wu,
> > Hao A <hao.a.wu@intel.com>; Wang, Jian J
> > <jian.j.wang@intel.com>
> > Subject: Re: [edk2-devel] [PATCH resend]
> > MdeModulePkg/EsrtDxe: allocate ESRT table from
> > RtServicesData memory
> >
> > On Fri, 19 Apr 2019 at 20:38, Ard Biesheuvel
> > <ard.biesheuvel@linaro.org> wrote:
> > >
> > > On Fri, 19 Apr 2019 at 20:23, Kinney, Michael D
> > > <michael.d.kinney@intel.com> wrote:
> > > >
> > > > Ard,
> > > >
> > > > Let's see if we can remove the ESRT access from
> > those
> > > > paths.  That would be the better fix.
> > > >
> > >
> > > I am not that familiar with this code, but it seems
> > that the only
> > > reason we access the ESRT at runtime is to check
> > whether a capsule is
> > > a nested FMP capsule, where the outer GUID is cross
> > referenced against
> > > the ESRT before checking whether the inner GUID is
> > the FMP guid.
> > >
> > > Could we relax this check? FMP capsule can only be
> > dispatched across a
> > > reboot anyway, and so the runtime capsule handling
> > could accept any
> > > capsule that has an inner FMP guid.
> > >
> > > If not, it means we do need to read the ESRT at
> > runtime, in which case
> > > my patch is the simplest solution. We would have to
> > clarify the spec
> > > though.
> > >
> >
> > Actually, I might be able to cache just the list of
> > GUIDs at
> > ReadyToBoot, so we can do the comparisons at runtime.

Re: [edk2-devel] [PATCH resend] MdeModulePkg/EsrtDxe: allocate ESRT table from RtServicesData memory

[Ard Biesheuvel]

On Fri, 19 Apr 2019 at 22:04, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> Ard,
>
> Saw your patch to cache the GUID table.  The ESRT Table
> is not that much bigger, so the algorithm may be simpler
> if you just make a copy of the ESRT table with the
> active entries.
>
> * 16+24 bytes per ESRT entry.
> * 16 bytes/entry for just the GUID.
>
> The only advantage of checking against the ESRT GUID at
> RT is to reject capsules that will be rejected when the
> capsule is processed later.  This can prevent extra reboots
> if an OS agent is sending capsules that do not really
> apply to the current system.  I expect OS agent to only
> send capsules that are in the ESRT.
>

Yeah. The only problem is that some capsules have no header at all,
and so without a GUID check, we might end up interpreting random data
as the capsule's HeaderSize field, which would be bad.

In general, I think we should be robust against random junk being
passed into the QueryCapsuleCapabilities() runtime service.