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

["Michael D Kinney" <michael.d.kinney@intel.com>]

Ard,

I only see one potential issue.

The size of the buffer copied is based on the FwResourceCount field.
The actual size of based on the FwResourceCountMax field.  Your patch
does not set FwResourceCountMax to FwResourceCount, so this may confuse
the OS consumers that may think the buffer allocated for ESRT is larger
that is actually is.

  ///
  /// The number of firmware resources in the table, must not be zero.
  ///
  UINT32                     FwResourceCount;
  ///
  /// The maximum number of resource array entries that can be within the table
  /// without reallocating the table, must not be zero.
  ///
  UINT32                     FwResourceCountMax;

The simplest fix is to use FwResourceCountMax instead of FwResourceCount 
for the copy size.  The other option is to set FwResourceCountMax to
FwResourceCountMax after the copy.

The rest of the patch looks good.  With one of the two changes above:

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

Mike

> -----Original Message-----
> From: devel@edk2.groups.io
> [mailto:devel@edk2.groups.io] On Behalf Of Ard
> Biesheuvel
> Sent: Monday, April 22, 2019 3:03 PM
> To: Wu, Hao A <hao.a.wu@intel.com>
> Cc: devel@edk2.groups.io; Wang, Jian J
> <jian.j.wang@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2]
> MdeModulePkg/DxeCapsuleLibFmp: avoid ESRT accesses at
> runtime
> 
> On Mon, 22 Apr 2019 at 09:14, Wu, Hao A
> <hao.a.wu@intel.com> wrote:
> >
> > > -----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>
> >
> 
> Thanks Hao. I will modify the subject to 'clone ESRT
> for runtime access'
> 
> Mike: any comments?
> 
>