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.kin...@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...@intel.com> > Cc: devel@edk2.groups.io; Wang, Jian J > <jian.j.w...@intel.com>; Kinney, Michael D > <michael.d.kin...@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...@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...@intel.com> > > > > Thanks Hao. I will modify the subject to 'clone ESRT > for runtime access' > > Mike: any comments? > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#39376): https://edk2.groups.io/g/devel/message/39376 Mute This Topic: https://groups.io/mt/31254157/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
