Agree with Laszlo. I prefer to move "Status = EFI_SUCCESS;" before the EDKII_TCG_PRE_HASH check.
With that moving, reviewed-by: Jiewen Yao <jiewen....@intel.com> Thank you Yao Jiewen > -----Original Message----- > From: Laszlo Ersek <ler...@redhat.com> > Sent: Monday, August 31, 2020 11:46 PM > To: devel@edk2.groups.io; Liu, Zhiguang <zhiguang....@intel.com> > Cc: Yao, Jiewen <jiewen....@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; > Zhang, Qi1 <qi1.zh...@intel.com>; Kumar, Rahul1 <rahul1.ku...@intel.com> > Subject: Re: [edk2-devel] [PATCH] SecurityPkg: Initailize variable Status > before it > is consumed. > > On 08/31/20 10:15, Zhiguang Liu wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2945 > > > > Cc: Jiewen Yao <jiewen....@intel.com> > > Cc: Jian J Wang <jian.j.w...@intel.com> > > Cc: Qi Zhang <qi1.zh...@intel.com> > > Cc: Rahul Kumar <rahul1.ku...@intel.com> > > Signed-off-by: Zhiguang Liu <zhiguang....@intel.com> > > --- > > SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c > b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c > > index 0e770f4485..5e883f0cc5 100644 > > --- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c > > +++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c > > @@ -449,6 +449,7 @@ HashLogExtendEvent ( > > EFI_STATUS Status; > > TPML_DIGEST_VALUES DigestList; > > > > + Status = EFI_SUCCESS; > > if (GetFirstGuidHob (&gTpmErrorHobGuid) != NULL) { > > return EFI_DEVICE_ERROR; > > } > > > > I agree that there is a path in the code where Status is read without > having been set. > > It seems that using EFI_SUCCESS as default value makes sense (assuming > the LogHashEvent() call is the important one). I'll let SecurityPkg > maintainers decide about this. > > I think it would be nicer to set Status to EFI_SUCCESS either on the > (currently missing) "else" branch, or else just before the > EDKII_TCG_PRE_HASH check. In particular, setting Status so early that we > may still exit with EFI_DEVICE_ERROR is wasteful. So at least I'd move > the assignment past the "return EFI_DEVICE_ERROR" statement. > > But... it's late. I agree that this patch qualifies for the stable tag. > So I'm not asking for a repost. I just wish more thought had been given > to the placement of the assignment. > > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64843): https://edk2.groups.io/g/devel/message/64843 Mute This Topic: https://groups.io/mt/76530112/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
