[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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Laszlo Ersek
>>> Sent: Thursday, July 9, 2020 6:02 PM
>>> To: devel at edk2.groups.io; Gao, Zhichao <zhichao.gao at intel.com>
>>> Cc: Terry Lee <terry.lee at hpe.com>; Yao, Jiewen <jiewen.yao at intel.com>; Wang,
>>> Jian J <jian.j.wang at intel.com>; Zhang, Chao B <chao.b.zhang at 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 at 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 at intel.com>
>>>> Cc: Jian J Wang <jian.j.wang at intel.com>
>>>> Cc: Chao Zhang <chao.b.zhang at intel.com>
>>>> Signed-off-by: Zhichao Gao <zhichao.gao at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-