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.biesheu...@linaro.org] > Sent: Monday, April 22, 2019 3:40 PM > To: edk2-devel-groups-io <devel@edk2.groups.io>; Kinney, > Michael D <michael.d.kin...@intel.com> > Cc: Wu, Hao A <hao.a...@intel.com>; Wang, Jian J > <jian.j.w...@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.kin...@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.kin...@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...@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 (#39378): https://edk2.groups.io/g/devel/message/39378 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] -=-=-=-=-=-=-=-=-=-=-=-
