.../SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
From: Terry Lee <terry.lee@hpe.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2697
Tcg2PhysicalPresenceLibConstructor set the module variable
mIsTcg2PPVerLowerThan_1_3 with incorrect TCG version comparision.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
.../SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c
index 1c46d5e69d..8afaa0a785 100644
--- a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c
+++ b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c
@@ -387,7 +387,7 @@ Tcg2PhysicalPresenceLibConstructor (
{
EFI_STATUS Status;
- if (AsciiStrnCmp(PP_INF_VERSION_1_2, (CHAR8 *)PcdGetPtr(PcdTcgPhysicalPresenceInterfaceVer), sizeof(PP_INF_VERSION_1_2) - 1) <= 0) {
+ if (AsciiStrnCmp(PP_INF_VERSION_1_2, (CHAR8 *)PcdGetPtr(PcdTcgPhysicalPresenceInterfaceVer), sizeof(PP_INF_VERSION_1_2) - 1) >= 0) {
mIsTcg2PPVerLowerThan_1_3 = TRUE;
}
--
2.21.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#62276): https://edk2.groups.io/g/devel/message/62276
Mute This Topic: https://groups.io/mt/75390754/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 07/09/20 04:46, Gao, Zhichao wrote: > From: Terry Lee <terry.lee@hpe.com> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2697 > > Tcg2PhysicalPresenceLibConstructor set the module variable > mIsTcg2PPVerLowerThan_1_3 with incorrect TCG version comparision. > > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Chao Zhang <chao.b.zhang@intel.com> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > --- > .../SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c > index 1c46d5e69d..8afaa0a785 100644 > --- a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c > +++ b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c > @@ -387,7 +387,7 @@ Tcg2PhysicalPresenceLibConstructor ( > { > EFI_STATUS Status; > > - if (AsciiStrnCmp(PP_INF_VERSION_1_2, (CHAR8 *)PcdGetPtr(PcdTcgPhysicalPresenceInterfaceVer), sizeof(PP_INF_VERSION_1_2) - 1) <= 0) { > + if (AsciiStrnCmp(PP_INF_VERSION_1_2, (CHAR8 *)PcdGetPtr(PcdTcgPhysicalPresenceInterfaceVer), sizeof(PP_INF_VERSION_1_2) - 1) >= 0) { > mIsTcg2PPVerLowerThan_1_3 = TRUE; > } > > What is the practical impact of this bug / fix? Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#62296): https://edk2.groups.io/g/devel/message/62296 Mute This Topic: https://groups.io/mt/75390754/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
This bug is not obeserved by me. But I view the code. The condition is incorrect and it would affect the TCG operation: if (!mIsTcg2PPVerLowerThan_1_3) { if (OperationRequest < TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION) { // // TCG2 PP1.3 spec defined operations that are reserved or un-implemented // return TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED; } } else { // // TCG PP lower than 1.3. (1.0, 1.1, 1.2) // if (OperationRequest <= TCG2_PHYSICAL_PRESENCE_NO_ACTION_MAX) { RequestConfirmed = TRUE; } else if (OperationRequest < TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION) { return TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED; } } So I think it should be fixed. Thanks, Zhichao > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek > Sent: Thursday, July 9, 2020 6:02 PM > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com> > Cc: Terry Lee <terry.lee@hpe.com>; Yao, Jiewen <jiewen.yao@intel.com>; Wang, > Jian J <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com> > Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix > incorrect TCG VER comparision > > On 07/09/20 04:46, Gao, Zhichao wrote: > > From: Terry Lee <terry.lee@hpe.com> > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2697 > > > > Tcg2PhysicalPresenceLibConstructor set the module variable > > mIsTcg2PPVerLowerThan_1_3 with incorrect TCG version comparision. > > > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > Cc: Jian J Wang <jian.j.wang@intel.com> > > Cc: Chao Zhang <chao.b.zhang@intel.com> > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > > --- > > .../SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git > > a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresen > > ceLib.c > > b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresen > > ceLib.c > > index 1c46d5e69d..8afaa0a785 100644 > > --- > > a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresen > > ceLib.c > > +++ b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr > > +++ esenceLib.c > > @@ -387,7 +387,7 @@ Tcg2PhysicalPresenceLibConstructor ( { > > EFI_STATUS Status; > > > > - if (AsciiStrnCmp(PP_INF_VERSION_1_2, (CHAR8 > > *)PcdGetPtr(PcdTcgPhysicalPresenceInterfaceVer), > > sizeof(PP_INF_VERSION_1_2) - 1) <= 0) { > > + if (AsciiStrnCmp(PP_INF_VERSION_1_2, (CHAR8 > > + *)PcdGetPtr(PcdTcgPhysicalPresenceInterfaceVer), > > + sizeof(PP_INF_VERSION_1_2) - 1) >= 0) { > > mIsTcg2PPVerLowerThan_1_3 = TRUE; > > } > > > > > > What is the practical impact of this bug / fix? > > Thanks > Laszlo > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#62324): https://edk2.groups.io/g/devel/message/62324 Mute This Topic: https://groups.io/mt/75390754/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
(+Marc-André, Stefan) On 07/10/20 02:44, Gao, Zhichao wrote: > This bug is not obeserved by me. But I view the code. The condition is incorrect and it would affect the TCG operation: > if (!mIsTcg2PPVerLowerThan_1_3) { > if (OperationRequest < TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION) { > // > // TCG2 PP1.3 spec defined operations that are reserved or un-implemented > // > return TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED; > } > } else { > // > // TCG PP lower than 1.3. (1.0, 1.1, 1.2) > // > if (OperationRequest <= TCG2_PHYSICAL_PRESENCE_NO_ACTION_MAX) { > RequestConfirmed = TRUE; > } else if (OperationRequest < TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION) { > return TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED; > } > } > I've found that code myself, but I'm not familiar enough with TPM PPI stuff to understand immediately the effects of this change. I can see that where we used to return TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED before, we could now assign "RequestConfirmed = TRUE", and vice versa, due to "mIsTcg2PPVerLowerThan_1_3" being potentially inverted. But what does that *mean*? What is the behavioral change that human end-users, or software components, will experience? Thanks Laszlo > So I think it should be fixed. > > Thanks, > Zhichao > >> -----Original Message----- >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek >> Sent: Thursday, July 9, 2020 6:02 PM >> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com> >> Cc: Terry Lee <terry.lee@hpe.com>; Yao, Jiewen <jiewen.yao@intel.com>; Wang, >> Jian J <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com> >> Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix >> incorrect TCG VER comparision >> >> On 07/09/20 04:46, Gao, Zhichao wrote: >>> From: Terry Lee <terry.lee@hpe.com> >>> >>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2697 >>> >>> Tcg2PhysicalPresenceLibConstructor set the module variable >>> mIsTcg2PPVerLowerThan_1_3 with incorrect TCG version comparision. >>> >>> Cc: Jiewen Yao <jiewen.yao@intel.com> >>> Cc: Jian J Wang <jian.j.wang@intel.com> >>> Cc: Chao Zhang <chao.b.zhang@intel.com> >>> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> >>> --- >>> .../SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git >>> a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresen >>> ceLib.c >>> b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresen >>> ceLib.c >>> index 1c46d5e69d..8afaa0a785 100644 >>> --- >>> a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresen >>> ceLib.c >>> +++ b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr >>> +++ esenceLib.c >>> @@ -387,7 +387,7 @@ Tcg2PhysicalPresenceLibConstructor ( { >>> EFI_STATUS Status; >>> >>> - if (AsciiStrnCmp(PP_INF_VERSION_1_2, (CHAR8 >>> *)PcdGetPtr(PcdTcgPhysicalPresenceInterfaceVer), >>> sizeof(PP_INF_VERSION_1_2) - 1) <= 0) { >>> + if (AsciiStrnCmp(PP_INF_VERSION_1_2, (CHAR8 >>> + *)PcdGetPtr(PcdTcgPhysicalPresenceInterfaceVer), >>> + sizeof(PP_INF_VERSION_1_2) - 1) >= 0) { >>> mIsTcg2PPVerLowerThan_1_3 = TRUE; >>> } >>> >>> >> >> What is the practical impact of this bug / fix? >> >> Thanks >> Laszlo >> >> >> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#62329): https://edk2.groups.io/g/devel/message/62329 Mute This Topic: https://groups.io/mt/75390754/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 7/10/20 1:43 AM, Laszlo Ersek wrote: > (+Marc-André, Stefan) > > On 07/10/20 02:44, Gao, Zhichao wrote: >> This bug is not obeserved by me. But I view the code. The condition is incorrect and it would affect the TCG operation: >> if (!mIsTcg2PPVerLowerThan_1_3) { >> if (OperationRequest < TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION) { >> // >> // TCG2 PP1.3 spec defined operations that are reserved or un-implemented >> // >> return TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED; >> } >> } else { >> // >> // TCG PP lower than 1.3. (1.0, 1.1, 1.2) >> // >> if (OperationRequest <= TCG2_PHYSICAL_PRESENCE_NO_ACTION_MAX) { >> RequestConfirmed = TRUE; >> } else if (OperationRequest < TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION) { >> return TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED; >> } >> } >> > I've found that code myself, but I'm not familiar enough with TPM PPI > stuff to understand immediately the effects of this change. I can see > that where we used to return > TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED before, we could now assign > "RequestConfirmed = TRUE", and vice versa, due to > "mIsTcg2PPVerLowerThan_1_3" being potentially inverted. > > But what does that *mean*? What is the behavioral change that human > end-users, or software components, will experience? The above code snipped is located in a default branch of a large switch statement that handles most of the common PPI operations independent of this change, so that at least is good. I would say that in the worst case some of the operations not otherwise handled may have mistakenly failed or could have been executed without user confirmation/interaction. On Linux at least PPI requests can only be sent by root. > > Thanks > Laszlo > >> So I think it should be fixed. >> >> Thanks, >> Zhichao >> >>> -----Original Message----- >>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek >>> Sent: Thursday, July 9, 2020 6:02 PM >>> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com> >>> Cc: Terry Lee <terry.lee@hpe.com>; Yao, Jiewen <jiewen.yao@intel.com>; Wang, >>> Jian J <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com> >>> Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix >>> incorrect TCG VER comparision >>> >>> On 07/09/20 04:46, Gao, Zhichao wrote: >>>> From: Terry Lee <terry.lee@hpe.com> >>>> >>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2697 >>>> >>>> Tcg2PhysicalPresenceLibConstructor set the module variable >>>> mIsTcg2PPVerLowerThan_1_3 with incorrect TCG version comparision. >>>> >>>> Cc: Jiewen Yao <jiewen.yao@intel.com> >>>> Cc: Jian J Wang <jian.j.wang@intel.com> >>>> Cc: Chao Zhang <chao.b.zhang@intel.com> >>>> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> >>>> --- >>>> .../SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git >>>> a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresen >>>> ceLib.c >>>> b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresen >>>> ceLib.c >>>> index 1c46d5e69d..8afaa0a785 100644 >>>> --- >>>> a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresen >>>> ceLib.c >>>> +++ b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr >>>> +++ esenceLib.c >>>> @@ -387,7 +387,7 @@ Tcg2PhysicalPresenceLibConstructor ( { >>>> EFI_STATUS Status; >>>> >>>> - if (AsciiStrnCmp(PP_INF_VERSION_1_2, (CHAR8 >>>> *)PcdGetPtr(PcdTcgPhysicalPresenceInterfaceVer), >>>> sizeof(PP_INF_VERSION_1_2) - 1) <= 0) { >>>> + if (AsciiStrnCmp(PP_INF_VERSION_1_2, (CHAR8 >>>> + *)PcdGetPtr(PcdTcgPhysicalPresenceInterfaceVer), >>>> + sizeof(PP_INF_VERSION_1_2) - 1) >= 0) { >>>> mIsTcg2PPVerLowerThan_1_3 = TRUE; >>>> } >>>> >>>> >>> What is the practical impact of this bug / fix? >>> >>> Thanks >>> Laszlo >>> >>> >>> > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#62347): https://edk2.groups.io/g/devel/message/62347 Mute This Topic: https://groups.io/mt/75390754/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.