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

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

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?
> >
> >
>
>
> 
>