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

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

[Gao, Zhichao]

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

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

[Laszlo Ersek]

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

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

[Gao, Zhichao]

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
> 
> 
> 

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

[Laszlo Ersek]

(+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
>>
>>
>> 
> 

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

[Stefan Berger]

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
>>>
>>>
>>>
>
> 
>

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

[Stefan Berger]

On 7/10/20 9:53 AM, Stefan Berger wrote:
> 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.


I am running a somewhat dated version of edk2 (Fedora 31). The 
operations advertised are: 0,5,14,21,22,23,24,33,96,97. All of these are 
individually handled in the switch statement, so there should no be any 
impact. I am currently not aware of whether this list can be extended 
with some sort of module.


>
>
>>
>> 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
>>>>
>>>>
>>>>
>>
>>
>>
>
>
> 
>

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

[Laszlo Ersek]

On 07/10/20 16:27, Stefan Berger wrote:
> On 7/10/20 9:53 AM, Stefan Berger wrote:
>> 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.
> 
> 
> I am running a somewhat dated version of edk2 (Fedora 31). The
> operations advertised are: 0,5,14,21,22,23,24,33,96,97. All of these are
> individually handled in the switch statement, so there should no be any
> impact. I am currently not aware of whether this list can be extended
> with some sort of module.

Thank you!
Laszlo

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

[Lee, Terry]

Could the package maintainer merge this patch?  Thanks.

Terry

-----Original Message-----
From: Stefan Berger [mailto:stefanb@linux.ibm.com] 
Sent: Friday, July 10, 2020 7:27 AM
To: devel@edk2.groups.io; lersek@redhat.com; Gao, Zhichao <zhichao.gao@intel.com>
Cc: Lee, Terry <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>; Marc-André Lureau <marcandre.lureau@redhat.com>
Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER comparision

On 7/10/20 9:53 AM, Stefan Berger wrote:
> 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.


I am running a somewhat dated version of edk2 (Fedora 31). The operations advertised are: 0,5,14,21,22,23,24,33,96,97. All of these are individually handled in the switch statement, so there should no be any impact. I am currently not aware of whether this list can be extended with some sort of module.


>
>
>>
>> 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: 
>>>>> INVALID URI REMOVED
>>>>> ocore.org_show-5Fbug.cgi-3Fid-3D2697&d=DwIDaQ&c=C5b8zRQO1miGmBeVZ2
>>>>> LFWg&r=Jlc0Jxr620EZ-CppyrjGotnxH9DrT0KvwcLjekZ9Dow&m=WPv3vn5VEelRC
>>>>> s-W8pfNM00wMOfpKBesXnAhRfylF7g&s=iFUYthUCfHLeeQAvr_OhTPHTiA9hZvw48
>>>>> Bj8YhXhQAI&e=
>>>>>
>>>>> 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/SmmTcg2PhysicalPr
>>>>> esen
>>>>>
>>>>> ceLib.c
>>>>> b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
>>>>> esen
>>>>>
>>>>> ceLib.c
>>>>> index 1c46d5e69d..8afaa0a785 100644
>>>>> ---
>>>>> a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
>>>>> esen
>>>>>
>>>>> 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
>>>>
>>>>
>>>>
>>
>>
>>
>
>
> 
>

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

[Yao, Jiewen]

Hello
Is there any one can share the information on what test has been done for this ?

Thank you
Yao Jiewen

> -----Original Message-----
> From: Lee, Terry <terry.lee@hpe.com>
> Sent: Friday, October 16, 2020 12:59 AM
> To: devel@edk2.groups.io; stefanb@linux.ibm.com; lersek@redhat.com; Gao,
> Zhichao <zhichao.gao@intel.com>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Marc-
> André Lureau <marcandre.lureau@redhat.com>
> Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix
> incorrect TCG VER comparision
> 
> Could the package maintainer merge this patch?  Thanks.
> 
> Terry
> 
> -----Original Message-----
> From: Stefan Berger [mailto:stefanb@linux.ibm.com]
> Sent: Friday, July 10, 2020 7:27 AM
> To: devel@edk2.groups.io; lersek@redhat.com; Gao, Zhichao
> <zhichao.gao@intel.com>
> Cc: Lee, Terry <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>; Marc-André Lureau
> <marcandre.lureau@redhat.com>
> Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix
> incorrect TCG VER comparision
> 
> On 7/10/20 9:53 AM, Stefan Berger wrote:
> > 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.
> 
> 
> I am running a somewhat dated version of edk2 (Fedora 31). The operations
> advertised are: 0,5,14,21,22,23,24,33,96,97. All of these are individually
> handled in the switch statement, so there should no be any impact. I am
> currently not aware of whether this list can be extended with some sort of
> module.
> 
> 
> >
> >
> >>
> >> 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://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.tian
> >>>>> ocore.org_show-5Fbug.cgi-3Fid-
> 3D2697&d=DwIDaQ&c=C5b8zRQO1miGmBeVZ2
> >>>>> LFWg&r=Jlc0Jxr620EZ-
> CppyrjGotnxH9DrT0KvwcLjekZ9Dow&m=WPv3vn5VEelRC
> >>>>> s-
> W8pfNM00wMOfpKBesXnAhRfylF7g&s=iFUYthUCfHLeeQAvr_OhTPHTiA9hZvw
> 48
> >>>>> Bj8YhXhQAI&e=
> >>>>>
> >>>>> 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/SmmTcg2PhysicalPr
> >>>>> esen
> >>>>>
> >>>>> ceLib.c
> >>>>>
> b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
> >>>>> esen
> >>>>>
> >>>>> ceLib.c
> >>>>> index 1c46d5e69d..8afaa0a785 100644
> >>>>> ---
> >>>>>
> a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
> >>>>> esen
> >>>>>
> >>>>> 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
> >>>>
> >>>>
> >>>>
> >>
> >>
> >>
> >
> >
> > 
> >
> 

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

[Lee, Terry]

Jiewen,

	I tested this patch on HPE Superdome Flex with both Linux and Windows.

				Terry

-----Original Message-----
From: Yao, Jiewen [mailto:jiewen.yao@intel.com] 
Sent: Thursday, October 15, 2020 6:09 PM
To: Lee, Terry <terry.lee@hpe.com>; devel@edk2.groups.io; stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao <zhichao.gao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Marc-André Lureau <marcandre.lureau@redhat.com>
Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER comparision

Hello
Is there any one can share the information on what test has been done for this ?

Thank you
Yao Jiewen

> -----Original Message-----
> From: Lee, Terry <terry.lee@hpe.com>
> Sent: Friday, October 16, 2020 12:59 AM
> To: devel@edk2.groups.io; stefanb@linux.ibm.com; lersek@redhat.com; 
> Gao, Zhichao <zhichao.gao@intel.com>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J 
> <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Marc- 
> André Lureau <marcandre.lureau@redhat.com>
> Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: 
> Fix incorrect TCG VER comparision
> 
> Could the package maintainer merge this patch?  Thanks.
> 
> Terry
> 
> -----Original Message-----
> From: Stefan Berger [mailto:stefanb@linux.ibm.com]
> Sent: Friday, July 10, 2020 7:27 AM
> To: devel@edk2.groups.io; lersek@redhat.com; Gao, Zhichao 
> <zhichao.gao@intel.com>
> Cc: Lee, Terry <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>; Marc-André Lureau 
> <marcandre.lureau@redhat.com>
> Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: 
> Fix incorrect TCG VER comparision
> 
> On 7/10/20 9:53 AM, Stefan Berger wrote:
> > 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.
> 
> 
> I am running a somewhat dated version of edk2 (Fedora 31). The 
> operations advertised are: 0,5,14,21,22,23,24,33,96,97. All of these 
> are individually handled in the switch statement, so there should no 
> be any impact. I am currently not aware of whether this list can be 
> extended with some sort of module.
> 
> 
> >
> >
> >>
> >> 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:
> >>>>> INVALID URI REMOVED
> >>>>> an
> >>>>> ocore.org_show-5Fbug.cgi-3Fid-
> 3D2697&d=DwIDaQ&c=C5b8zRQO1miGmBeVZ2
> >>>>> LFWg&r=Jlc0Jxr620EZ-
> CppyrjGotnxH9DrT0KvwcLjekZ9Dow&m=WPv3vn5VEelRC
> >>>>> s-
> W8pfNM00wMOfpKBesXnAhRfylF7g&s=iFUYthUCfHLeeQAvr_OhTPHTiA9hZvw
> 48
> >>>>> Bj8YhXhQAI&e=
> >>>>>
> >>>>> 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/SmmTcg2PhysicalPr
> >>>>> esen
> >>>>>
> >>>>> ceLib.c
> >>>>>
> b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
> >>>>> esen
> >>>>>
> >>>>> ceLib.c
> >>>>> index 1c46d5e69d..8afaa0a785 100644
> >>>>> ---
> >>>>>
> a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
> >>>>> esen
> >>>>>
> >>>>> 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
> >>>>
> >>>>
> >>>>
> >>
> >>
> >>
> >
> >
> > 
> >
> 

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

[Yao, Jiewen]

Thanks Terry.
I tend to give R-B. I read the code it seems no impact. 

Would you please confirm you have tested both PP1.2 and PP1.3 configuration, with windows WHCK test pass?

Thank you
Yao Jiewen

> -----Original Message-----
> From: Lee, Terry <terry.lee@hpe.com>
> Sent: Friday, October 16, 2020 10:25 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
> stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
> <zhichao.gao@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>; Marc-André Lureau
> <marcandre.lureau@redhat.com>
> Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix
> incorrect TCG VER comparision
> 
> Jiewen,
> 
> I tested this patch on HPE Superdome Flex with both Linux and Windows.
> 
> Terry
> 
> -----Original Message-----
> From: Yao, Jiewen [mailto:jiewen.yao@intel.com]
> Sent: Thursday, October 15, 2020 6:09 PM
> To: Lee, Terry <terry.lee@hpe.com>; devel@edk2.groups.io;
> stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
> <zhichao.gao@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>; Marc-André Lureau
> <marcandre.lureau@redhat.com>
> Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix
> incorrect TCG VER comparision
> 
> Hello
> Is there any one can share the information on what test has been done for
> this ?
> 
> Thank you
> Yao Jiewen
> 
> > -----Original Message-----
> > From: Lee, Terry <terry.lee@hpe.com>
> > Sent: Friday, October 16, 2020 12:59 AM
> > To: devel@edk2.groups.io; stefanb@linux.ibm.com; lersek@redhat.com;
> > Gao, Zhichao <zhichao.gao@intel.com>
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Marc-
> > André Lureau <marcandre.lureau@redhat.com>
> > Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
> > Fix incorrect TCG VER comparision
> >
> > Could the package maintainer merge this patch?  Thanks.
> >
> > Terry
> >
> > -----Original Message-----
> > From: Stefan Berger [mailto:stefanb@linux.ibm.com]
> > Sent: Friday, July 10, 2020 7:27 AM
> > To: devel@edk2.groups.io; lersek@redhat.com; Gao, Zhichao
> > <zhichao.gao@intel.com>
> > Cc: Lee, Terry <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>; Marc-André Lureau
> > <marcandre.lureau@redhat.com>
> > Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
> > Fix incorrect TCG VER comparision
> >
> > On 7/10/20 9:53 AM, Stefan Berger wrote:
> > > 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.
> >
> >
> > I am running a somewhat dated version of edk2 (Fedora 31). The
> > operations advertised are: 0,5,14,21,22,23,24,33,96,97. All of these
> > are individually handled in the switch statement, so there should no
> > be any impact. I am currently not aware of whether this list can be
> > extended with some sort of module.
> >
> >
> > >
> > >
> > >>
> > >> 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://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.ti
> > >>>>> an
> > >>>>> ocore.org_show-5Fbug.cgi-3Fid-
> > 3D2697&d=DwIDaQ&c=C5b8zRQO1miGmBeVZ2
> > >>>>> LFWg&r=Jlc0Jxr620EZ-
> > CppyrjGotnxH9DrT0KvwcLjekZ9Dow&m=WPv3vn5VEelRC
> > >>>>> s-
> >
> W8pfNM00wMOfpKBesXnAhRfylF7g&s=iFUYthUCfHLeeQAvr_OhTPHTiA9hZvw
> > 48
> > >>>>> Bj8YhXhQAI&e=
> > >>>>>
> > >>>>> 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/SmmTcg2PhysicalPr
> > >>>>> esen
> > >>>>>
> > >>>>> ceLib.c
> > >>>>>
> > b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
> > >>>>> esen
> > >>>>>
> > >>>>> ceLib.c
> > >>>>> index 1c46d5e69d..8afaa0a785 100644
> > >>>>> ---
> > >>>>>
> > a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
> > >>>>> esen
> > >>>>>
> > >>>>> 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
> > >>>>
> > >>>>
> > >>>>
> > >>
> > >>
> > >>
> > >
> > >
> > > 
> > >
> >
> 

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

[Lee, Terry]

Jiewen,

	I have only PP1.3 configuration.  The only WHCK test failure is a known Windows issue that I believe is unrelated to PP.

				Terry

-----Original Message-----
From: Yao, Jiewen [mailto:jiewen.yao@intel.com] 
Sent: Thursday, October 15, 2020 7:31 PM
To: Lee, Terry <terry.lee@hpe.com>; devel@edk2.groups.io; stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao <zhichao.gao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Marc-André Lureau <marcandre.lureau@redhat.com>
Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER comparision

Thanks Terry.
I tend to give R-B. I read the code it seems no impact. 

Would you please confirm you have tested both PP1.2 and PP1.3 configuration, with windows WHCK test pass?

Thank you
Yao Jiewen

> -----Original Message-----
> From: Lee, Terry <terry.lee@hpe.com>
> Sent: Friday, October 16, 2020 10:25 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; 
> stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao 
> <zhichao.gao@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B 
> <chao.b.zhang@intel.com>; Marc-André Lureau 
> <marcandre.lureau@redhat.com>
> Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: 
> Fix incorrect TCG VER comparision
> 
> Jiewen,
> 
> I tested this patch on HPE Superdome Flex with both Linux and Windows.
> 
> Terry
> 
> -----Original Message-----
> From: Yao, Jiewen [mailto:jiewen.yao@intel.com]
> Sent: Thursday, October 15, 2020 6:09 PM
> To: Lee, Terry <terry.lee@hpe.com>; devel@edk2.groups.io; 
> stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao 
> <zhichao.gao@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B 
> <chao.b.zhang@intel.com>; Marc-André Lureau 
> <marcandre.lureau@redhat.com>
> Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: 
> Fix incorrect TCG VER comparision
> 
> Hello
> Is there any one can share the information on what test has been done 
> for this ?
> 
> Thank you
> Yao Jiewen
> 
> > -----Original Message-----
> > From: Lee, Terry <terry.lee@hpe.com>
> > Sent: Friday, October 16, 2020 12:59 AM
> > To: devel@edk2.groups.io; stefanb@linux.ibm.com; lersek@redhat.com; 
> > Gao, Zhichao <zhichao.gao@intel.com>
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J 
> > <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; 
> > Marc- André Lureau <marcandre.lureau@redhat.com>
> > Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
> > Fix incorrect TCG VER comparision
> >
> > Could the package maintainer merge this patch?  Thanks.
> >
> > Terry
> >
> > -----Original Message-----
> > From: Stefan Berger [mailto:stefanb@linux.ibm.com]
> > Sent: Friday, July 10, 2020 7:27 AM
> > To: devel@edk2.groups.io; lersek@redhat.com; Gao, Zhichao 
> > <zhichao.gao@intel.com>
> > Cc: Lee, Terry <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>; Marc-André Lureau 
> > <marcandre.lureau@redhat.com>
> > Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
> > Fix incorrect TCG VER comparision
> >
> > On 7/10/20 9:53 AM, Stefan Berger wrote:
> > > 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.
> >
> >
> > I am running a somewhat dated version of edk2 (Fedora 31). The 
> > operations advertised are: 0,5,14,21,22,23,24,33,96,97. All of these 
> > are individually handled in the switch statement, so there should no 
> > be any impact. I am currently not aware of whether this list can be 
> > extended with some sort of module.
> >
> >
> > >
> > >
> > >>
> > >> 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:
> > >>>>> INVALID URI REMOVED.
> > >>>>> ti
> > >>>>> an
> > >>>>> ocore.org_show-5Fbug.cgi-3Fid-
> > 3D2697&d=DwIDaQ&c=C5b8zRQO1miGmBeVZ2
> > >>>>> LFWg&r=Jlc0Jxr620EZ-
> > CppyrjGotnxH9DrT0KvwcLjekZ9Dow&m=WPv3vn5VEelRC
> > >>>>> s-
> >
> W8pfNM00wMOfpKBesXnAhRfylF7g&s=iFUYthUCfHLeeQAvr_OhTPHTiA9hZvw
> > 48
> > >>>>> Bj8YhXhQAI&e=
> > >>>>>
> > >>>>> 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/SmmTcg2PhysicalPr
> > >>>>> esen
> > >>>>>
> > >>>>> ceLib.c
> > >>>>>
> > b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
> > >>>>> esen
> > >>>>>
> > >>>>> ceLib.c
> > >>>>> index 1c46d5e69d..8afaa0a785 100644
> > >>>>> ---
> > >>>>>
> > a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
> > >>>>> esen
> > >>>>>
> > >>>>> 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
> > >>>>
> > >>>>
> > >>>>
> > >>
> > >>
> > >>
> > >
> > >
> > > 
> > >
> >
> 

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

[Yao, Jiewen]

Fair enough.

Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

I will wait for a few more days to see if there is any feedback from Laszlo or Marc-Andre.




> -----Original Message-----
> From: Lee, Terry <terry.lee@hpe.com>
> Sent: Friday, October 16, 2020 1:32 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
> stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
> <zhichao.gao@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>; Marc-André Lureau
> <marcandre.lureau@redhat.com>
> Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix
> incorrect TCG VER comparision
> 
> Jiewen,
> 
> I have only PP1.3 configuration.  The only WHCK test failure is a known
> Windows issue that I believe is unrelated to PP.
> 
> Terry
> 
> -----Original Message-----
> From: Yao, Jiewen [mailto:jiewen.yao@intel.com]
> Sent: Thursday, October 15, 2020 7:31 PM
> To: Lee, Terry <terry.lee@hpe.com>; devel@edk2.groups.io;
> stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
> <zhichao.gao@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>; Marc-André Lureau
> <marcandre.lureau@redhat.com>
> Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix
> incorrect TCG VER comparision
> 
> Thanks Terry.
> I tend to give R-B. I read the code it seems no impact.
> 
> Would you please confirm you have tested both PP1.2 and PP1.3
> configuration, with windows WHCK test pass?
> 
> Thank you
> Yao Jiewen
> 
> > -----Original Message-----
> > From: Lee, Terry <terry.lee@hpe.com>
> > Sent: Friday, October 16, 2020 10:25 AM
> > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
> > stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
> > <zhichao.gao@intel.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > <chao.b.zhang@intel.com>; Marc-André Lureau
> > <marcandre.lureau@redhat.com>
> > Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
> > Fix incorrect TCG VER comparision
> >
> > Jiewen,
> >
> > I tested this patch on HPE Superdome Flex with both Linux and Windows.
> >
> > Terry
> >
> > -----Original Message-----
> > From: Yao, Jiewen [mailto:jiewen.yao@intel.com]
> > Sent: Thursday, October 15, 2020 6:09 PM
> > To: Lee, Terry <terry.lee@hpe.com>; devel@edk2.groups.io;
> > stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
> > <zhichao.gao@intel.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > <chao.b.zhang@intel.com>; Marc-André Lureau
> > <marcandre.lureau@redhat.com>
> > Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
> > Fix incorrect TCG VER comparision
> >
> > Hello
> > Is there any one can share the information on what test has been done
> > for this ?
> >
> > Thank you
> > Yao Jiewen
> >
> > > -----Original Message-----
> > > From: Lee, Terry <terry.lee@hpe.com>
> > > Sent: Friday, October 16, 2020 12:59 AM
> > > To: devel@edk2.groups.io; stefanb@linux.ibm.com; lersek@redhat.com;
> > > Gao, Zhichao <zhichao.gao@intel.com>
> > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > > <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>;
> > > Marc- André Lureau <marcandre.lureau@redhat.com>
> > > Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
> > > Fix incorrect TCG VER comparision
> > >
> > > Could the package maintainer merge this patch?  Thanks.
> > >
> > > Terry
> > >
> > > -----Original Message-----
> > > From: Stefan Berger [mailto:stefanb@linux.ibm.com]
> > > Sent: Friday, July 10, 2020 7:27 AM
> > > To: devel@edk2.groups.io; lersek@redhat.com; Gao, Zhichao
> > > <zhichao.gao@intel.com>
> > > Cc: Lee, Terry <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>; Marc-André Lureau
> > > <marcandre.lureau@redhat.com>
> > > Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
> > > Fix incorrect TCG VER comparision
> > >
> > > On 7/10/20 9:53 AM, Stefan Berger wrote:
> > > > 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.
> > >
> > >
> > > I am running a somewhat dated version of edk2 (Fedora 31). The
> > > operations advertised are: 0,5,14,21,22,23,24,33,96,97. All of these
> > > are individually handled in the switch statement, so there should no
> > > be any impact. I am currently not aware of whether this list can be
> > > extended with some sort of module.
> > >
> > >
> > > >
> > > >
> > > >>
> > > >> 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://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.
> > > >>>>> ti
> > > >>>>> an
> > > >>>>> ocore.org_show-5Fbug.cgi-3Fid-
> > > 3D2697&d=DwIDaQ&c=C5b8zRQO1miGmBeVZ2
> > > >>>>> LFWg&r=Jlc0Jxr620EZ-
> > > CppyrjGotnxH9DrT0KvwcLjekZ9Dow&m=WPv3vn5VEelRC
> > > >>>>> s-
> > >
> >
> W8pfNM00wMOfpKBesXnAhRfylF7g&s=iFUYthUCfHLeeQAvr_OhTPHTiA9hZvw
> > > 48
> > > >>>>> Bj8YhXhQAI&e=
> > > >>>>>
> > > >>>>> 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/SmmTcg2PhysicalPr
> > > >>>>> esen
> > > >>>>>
> > > >>>>> ceLib.c
> > > >>>>>
> > > b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
> > > >>>>> esen
> > > >>>>>
> > > >>>>> ceLib.c
> > > >>>>> index 1c46d5e69d..8afaa0a785 100644
> > > >>>>> ---
> > > >>>>>
> > > a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
> > > >>>>> esen
> > > >>>>>
> > > >>>>> 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
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>
> > > >>
> > > >>
> > > >
> > > >
> > > > 
> > > >
> > >
> >
> 

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

[Yao, Jiewen]

Pushed
https://github.com/tianocore/edk2/pull/1027
git-hash: 709b163940c55604b983400eb49dad144a2aa091

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao,
> Jiewen
> Sent: Friday, October 16, 2020 1:55 PM
> To: Lee, Terry <terry.lee@hpe.com>; devel@edk2.groups.io;
> stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
> <zhichao.gao@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>; Marc-André Lureau
> <marcandre.lureau@redhat.com>
> Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix
> incorrect TCG VER comparision
> 
> Fair enough.
> 
> Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> I will wait for a few more days to see if there is any feedback from Laszlo or
> Marc-Andre.
> 
> 
> 
> 
> > -----Original Message-----
> > From: Lee, Terry <terry.lee@hpe.com>
> > Sent: Friday, October 16, 2020 1:32 PM
> > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
> > stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
> > <zhichao.gao@intel.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > <chao.b.zhang@intel.com>; Marc-André Lureau
> > <marcandre.lureau@redhat.com>
> > Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix
> > incorrect TCG VER comparision
> >
> > Jiewen,
> >
> > I have only PP1.3 configuration.  The only WHCK test failure is a known
> > Windows issue that I believe is unrelated to PP.
> >
> > Terry
> >
> > -----Original Message-----
> > From: Yao, Jiewen [mailto:jiewen.yao@intel.com]
> > Sent: Thursday, October 15, 2020 7:31 PM
> > To: Lee, Terry <terry.lee@hpe.com>; devel@edk2.groups.io;
> > stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
> > <zhichao.gao@intel.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > <chao.b.zhang@intel.com>; Marc-André Lureau
> > <marcandre.lureau@redhat.com>
> > Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix
> > incorrect TCG VER comparision
> >
> > Thanks Terry.
> > I tend to give R-B. I read the code it seems no impact.
> >
> > Would you please confirm you have tested both PP1.2 and PP1.3
> > configuration, with windows WHCK test pass?
> >
> > Thank you
> > Yao Jiewen
> >
> > > -----Original Message-----
> > > From: Lee, Terry <terry.lee@hpe.com>
> > > Sent: Friday, October 16, 2020 10:25 AM
> > > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
> > > stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
> > > <zhichao.gao@intel.com>
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > > <chao.b.zhang@intel.com>; Marc-André Lureau
> > > <marcandre.lureau@redhat.com>
> > > Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
> > > Fix incorrect TCG VER comparision
> > >
> > > Jiewen,
> > >
> > > I tested this patch on HPE Superdome Flex with both Linux and Windows.
> > >
> > > Terry
> > >
> > > -----Original Message-----
> > > From: Yao, Jiewen [mailto:jiewen.yao@intel.com]
> > > Sent: Thursday, October 15, 2020 6:09 PM
> > > To: Lee, Terry <terry.lee@hpe.com>; devel@edk2.groups.io;
> > > stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
> > > <zhichao.gao@intel.com>
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > > <chao.b.zhang@intel.com>; Marc-André Lureau
> > > <marcandre.lureau@redhat.com>
> > > Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
> > > Fix incorrect TCG VER comparision
> > >
> > > Hello
> > > Is there any one can share the information on what test has been done
> > > for this ?
> > >
> > > Thank you
> > > Yao Jiewen
> > >
> > > > -----Original Message-----
> > > > From: Lee, Terry <terry.lee@hpe.com>
> > > > Sent: Friday, October 16, 2020 12:59 AM
> > > > To: devel@edk2.groups.io; stefanb@linux.ibm.com; lersek@redhat.com;
> > > > Gao, Zhichao <zhichao.gao@intel.com>
> > > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > > > <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>;
> > > > Marc- André Lureau <marcandre.lureau@redhat.com>
> > > > Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
> > > > Fix incorrect TCG VER comparision
> > > >
> > > > Could the package maintainer merge this patch?  Thanks.
> > > >
> > > > Terry
> > > >
> > > > -----Original Message-----
> > > > From: Stefan Berger [mailto:stefanb@linux.ibm.com]
> > > > Sent: Friday, July 10, 2020 7:27 AM
> > > > To: devel@edk2.groups.io; lersek@redhat.com; Gao, Zhichao
> > > > <zhichao.gao@intel.com>
> > > > Cc: Lee, Terry <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>; Marc-André Lureau
> > > > <marcandre.lureau@redhat.com>
> > > > Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
> > > > Fix incorrect TCG VER comparision
> > > >
> > > > On 7/10/20 9:53 AM, Stefan Berger wrote:
> > > > > 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.
> > > >
> > > >
> > > > I am running a somewhat dated version of edk2 (Fedora 31). The
> > > > operations advertised are: 0,5,14,21,22,23,24,33,96,97. All of these
> > > > are individually handled in the switch statement, so there should no
> > > > be any impact. I am currently not aware of whether this list can be
> > > > extended with some sort of module.
> > > >
> > > >
> > > > >
> > > > >
> > > > >>
> > > > >> 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://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.
> > > > >>>>> ti
> > > > >>>>> an
> > > > >>>>> ocore.org_show-5Fbug.cgi-3Fid-
> > > > 3D2697&d=DwIDaQ&c=C5b8zRQO1miGmBeVZ2
> > > > >>>>> LFWg&r=Jlc0Jxr620EZ-
> > > > CppyrjGotnxH9DrT0KvwcLjekZ9Dow&m=WPv3vn5VEelRC
> > > > >>>>> s-
> > > >
> > >
> >
> W8pfNM00wMOfpKBesXnAhRfylF7g&s=iFUYthUCfHLeeQAvr_OhTPHTiA9hZvw
> > > > 48
> > > > >>>>> Bj8YhXhQAI&e=
> > > > >>>>>
> > > > >>>>> 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/SmmTcg2PhysicalPr
> > > > >>>>> esen
> > > > >>>>>
> > > > >>>>> ceLib.c
> > > > >>>>>
> > > > b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
> > > > >>>>> esen
> > > > >>>>>
> > > > >>>>> ceLib.c
> > > > >>>>> index 1c46d5e69d..8afaa0a785 100644
> > > > >>>>> ---
> > > > >>>>>
> > > > a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
> > > > >>>>> esen
> > > > >>>>>
> > > > >>>>> 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
> > > > >>>>
> > > > >>>>
> > > > >>>>
> > > > >>
> > > > >>
> > > > >>
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >
> 
> 
> 
> 
> 

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

[Laszlo Ersek]

(sorry about the late response)

On 10/18/20 03:18, Yao, Jiewen wrote:
> Pushed
> https://github.com/tianocore/edk2/pull/1027
> git-hash: 709b163940c55604b983400eb49dad144a2aa091

Thanks, Jiewen!

I didn't have anything to add, after this:

https://edk2.groups.io/g/devel/message/62424

So I have no objection.

Thanks,
Laszlo

> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao,
>> Jiewen
>> Sent: Friday, October 16, 2020 1:55 PM
>> To: Lee, Terry <terry.lee@hpe.com>; devel@edk2.groups.io;
>> stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
>> <zhichao.gao@intel.com>
>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
>> <chao.b.zhang@intel.com>; Marc-André Lureau
>> <marcandre.lureau@redhat.com>
>> Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix
>> incorrect TCG VER comparision
>>
>> Fair enough.
>>
>> Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
>>
>> I will wait for a few more days to see if there is any feedback from Laszlo or
>> Marc-Andre.
>>
>>
>>
>>
>>> -----Original Message-----
>>> From: Lee, Terry <terry.lee@hpe.com>
>>> Sent: Friday, October 16, 2020 1:32 PM
>>> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
>>> stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
>>> <zhichao.gao@intel.com>
>>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
>>> <chao.b.zhang@intel.com>; Marc-André Lureau
>>> <marcandre.lureau@redhat.com>
>>> Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix
>>> incorrect TCG VER comparision
>>>
>>> Jiewen,
>>>
>>> I have only PP1.3 configuration.  The only WHCK test failure is a known
>>> Windows issue that I believe is unrelated to PP.
>>>
>>> Terry
>>>
>>> -----Original Message-----
>>> From: Yao, Jiewen [mailto:jiewen.yao@intel.com]
>>> Sent: Thursday, October 15, 2020 7:31 PM
>>> To: Lee, Terry <terry.lee@hpe.com>; devel@edk2.groups.io;
>>> stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
>>> <zhichao.gao@intel.com>
>>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
>>> <chao.b.zhang@intel.com>; Marc-André Lureau
>>> <marcandre.lureau@redhat.com>
>>> Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix
>>> incorrect TCG VER comparision
>>>
>>> Thanks Terry.
>>> I tend to give R-B. I read the code it seems no impact.
>>>
>>> Would you please confirm you have tested both PP1.2 and PP1.3
>>> configuration, with windows WHCK test pass?
>>>
>>> Thank you
>>> Yao Jiewen
>>>
>>>> -----Original Message-----
>>>> From: Lee, Terry <terry.lee@hpe.com>
>>>> Sent: Friday, October 16, 2020 10:25 AM
>>>> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
>>>> stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
>>>> <zhichao.gao@intel.com>
>>>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
>>>> <chao.b.zhang@intel.com>; Marc-André Lureau
>>>> <marcandre.lureau@redhat.com>
>>>> Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
>>>> Fix incorrect TCG VER comparision
>>>>
>>>> Jiewen,
>>>>
>>>> I tested this patch on HPE Superdome Flex with both Linux and Windows.
>>>>
>>>> Terry
>>>>
>>>> -----Original Message-----
>>>> From: Yao, Jiewen [mailto:jiewen.yao@intel.com]
>>>> Sent: Thursday, October 15, 2020 6:09 PM
>>>> To: Lee, Terry <terry.lee@hpe.com>; devel@edk2.groups.io;
>>>> stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
>>>> <zhichao.gao@intel.com>
>>>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
>>>> <chao.b.zhang@intel.com>; Marc-André Lureau
>>>> <marcandre.lureau@redhat.com>
>>>> Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
>>>> Fix incorrect TCG VER comparision
>>>>
>>>> Hello
>>>> Is there any one can share the information on what test has been done
>>>> for this ?
>>>>
>>>> Thank you
>>>> Yao Jiewen
>>>>
>>>>> -----Original Message-----
>>>>> From: Lee, Terry <terry.lee@hpe.com>
>>>>> Sent: Friday, October 16, 2020 12:59 AM
>>>>> To: devel@edk2.groups.io; stefanb@linux.ibm.com; lersek@redhat.com;
>>>>> Gao, Zhichao <zhichao.gao@intel.com>
>>>>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
>>>>> <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>;
>>>>> Marc- André Lureau <marcandre.lureau@redhat.com>
>>>>> Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
>>>>> Fix incorrect TCG VER comparision
>>>>>
>>>>> Could the package maintainer merge this patch?  Thanks.
>>>>>
>>>>> Terry
>>>>>
>>>>> -----Original Message-----
>>>>> From: Stefan Berger [mailto:stefanb@linux.ibm.com]
>>>>> Sent: Friday, July 10, 2020 7:27 AM
>>>>> To: devel@edk2.groups.io; lersek@redhat.com; Gao, Zhichao
>>>>> <zhichao.gao@intel.com>
>>>>> Cc: Lee, Terry <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>; Marc-André Lureau
>>>>> <marcandre.lureau@redhat.com>
>>>>> Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
>>>>> Fix incorrect TCG VER comparision
>>>>>
>>>>> On 7/10/20 9:53 AM, Stefan Berger wrote:
>>>>>> 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.
>>>>>
>>>>>
>>>>> I am running a somewhat dated version of edk2 (Fedora 31). The
>>>>> operations advertised are: 0,5,14,21,22,23,24,33,96,97. All of these
>>>>> are individually handled in the switch statement, so there should no
>>>>> be any impact. I am currently not aware of whether this list can be
>>>>> extended with some sort of module.
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> 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://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.
>>>>>>>>>> ti
>>>>>>>>>> an
>>>>>>>>>> ocore.org_show-5Fbug.cgi-3Fid-
>>>>> 3D2697&d=DwIDaQ&c=C5b8zRQO1miGmBeVZ2
>>>>>>>>>> LFWg&r=Jlc0Jxr620EZ-
>>>>> CppyrjGotnxH9DrT0KvwcLjekZ9Dow&m=WPv3vn5VEelRC
>>>>>>>>>> s-
>>>>>
>>>>
>>>
>> W8pfNM00wMOfpKBesXnAhRfylF7g&s=iFUYthUCfHLeeQAvr_OhTPHTiA9hZvw
>>>>> 48
>>>>>>>>>> Bj8YhXhQAI&e=
>>>>>>>>>>
>>>>>>>>>> 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/SmmTcg2PhysicalPr
>>>>>>>>>> esen
>>>>>>>>>>
>>>>>>>>>> ceLib.c
>>>>>>>>>>
>>>>> b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
>>>>>>>>>> esen
>>>>>>>>>>
>>>>>>>>>> ceLib.c
>>>>>>>>>> index 1c46d5e69d..8afaa0a785 100644
>>>>>>>>>> ---
>>>>>>>>>>
>>>>> a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
>>>>>>>>>> esen
>>>>>>>>>>
>>>>>>>>>> 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>>
>> 
>>
>