[edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER comparision

Gao, Zhichao posted 1 patch 3 years, 8 months ago
Failed in applying to current master (apply log)
.../SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c     | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER comparision
Posted by Gao, Zhichao 3 years, 8 months ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER comparision
Posted by Laszlo Ersek 3 years, 8 months ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER comparision
Posted by Gao, Zhichao 3 years, 8 months ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER comparision
Posted by Laszlo Ersek 3 years, 8 months ago
(+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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER comparision
Posted by Stefan Berger 3 years, 8 months ago
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]
-=-=-=-=-=-=-=-=-=-=-=-