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

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

Ard,

I agree. Only consumer is only the DxeCapsuleLibFmp
itself.

I see I had a typo on the second option.  The
statement to add after the copy and ASSERT() 
would be:

  mEsrtTable->FwResourceCountMax = FwResourceCount;

Mike

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, April 22, 2019 3:40 PM
> To: edk2-devel-groups-io <devel@edk2.groups.io>; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2]
> MdeModulePkg/DxeCapsuleLibFmp: avoid ESRT accesses at
> runtime
> 
> On Tue, 23 Apr 2019 at 00:37, Michael D Kinney
> <michael.d.kinney@intel.com> wrote:
> >
> > 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 OS never sees our copy of the table. That copy is
> for internal use
> only by the DxeCapsuleLibFmp implementation.
> 
> > 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.
> >
> 
> Given the above, I'll go with the latter. It is unlikely
> to matter,
> but it is more correct, and prevents any potential
> confusion in the
> future.
> 
> > The rest of the patch looks good.  With one of the two
> changes above:
> >
> > Reviewed-by: Michael D Kinney
> <michael.d.kinney@intel.com>
> >
> 
> Thanks.
> 
> >
> > > -----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?
> > >
> > >
> >
> >
> > 
> >