SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c | 1 + 1 file changed, 1 insertion(+)
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2945
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Qi Zhang <qi1.zhang@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@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;
}
--
2.25.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#64819): https://edk2.groups.io/g/devel/message/64819
Mute This Topic: https://groups.io/mt/76530112/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 08/31/20 10:15, Zhiguang Liu wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2945 > > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Qi Zhang <qi1.zhang@intel.com> > Cc: Rahul Kumar <rahul1.kumar@intel.com> > Signed-off-by: Zhiguang Liu <zhiguang.liu@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 (#64840): https://edk2.groups.io/g/devel/message/64840 Mute This Topic: https://groups.io/mt/76530112/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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.yao@intel.com> Thank you Yao Jiewen > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: Monday, August 31, 2020 11:46 PM > To: devel@edk2.groups.io; Liu, Zhiguang <zhiguang.liu@intel.com> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; > Zhang, Qi1 <qi1.zhang@intel.com>; Kumar, Rahul1 <rahul1.kumar@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.yao@intel.com> > > Cc: Jian J Wang <jian.j.wang@intel.com> > > Cc: Qi Zhang <qi1.zhang@intel.com> > > Cc: Rahul Kumar <rahul1.kumar@intel.com> > > Signed-off-by: Zhiguang Liu <zhiguang.liu@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/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.