Topics

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


Gao, Zhichao
 

From: Terry Lee <terry.lee@...>

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@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Chao Zhang <chao.b.zhang@...>
Signed-off-by: Zhichao Gao <zhichao.gao@...>
---
.../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


Laszlo Ersek
 

On 07/09/20 04:46, Gao, Zhichao wrote:
From: Terry Lee <terry.lee@...>

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@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Chao Zhang <chao.b.zhang@...>
Signed-off-by: Zhichao Gao <zhichao.gao@...>
---
.../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


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@...>
Cc: Terry Lee <terry.lee@...>; Yao, Jiewen <jiewen.yao@...>; Wang,
Jian J <jian.j.wang@...>; Zhang, Chao B <chao.b.zhang@...>
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@...>

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@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Chao Zhang <chao.b.zhang@...>
Signed-off-by: Zhichao Gao <zhichao.gao@...>
---
.../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



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@...>
Cc: Terry Lee <terry.lee@...>; Yao, Jiewen <jiewen.yao@...>; Wang,
Jian J <jian.j.wang@...>; Zhang, Chao B <chao.b.zhang@...>
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@...>

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@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Chao Zhang <chao.b.zhang@...>
Signed-off-by: Zhichao Gao <zhichao.gao@...>
---
.../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



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@...>
Cc: Terry Lee <terry.lee@...>; Yao, Jiewen <jiewen.yao@...>; Wang,
Jian J <jian.j.wang@...>; Zhang, Chao B <chao.b.zhang@...>
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@...>

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@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Chao Zhang <chao.b.zhang@...>
Signed-off-by: Zhichao Gao <zhichao.gao@...>
---
.../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



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@...>
Cc: Terry Lee <terry.lee@...>; Yao, Jiewen <jiewen.yao@...>; Wang,
Jian J <jian.j.wang@...>; Zhang, Chao B <chao.b.zhang@...>
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@...>

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@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Chao Zhang <chao.b.zhang@...>
Signed-off-by: Zhichao Gao <zhichao.gao@...>
---
.../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





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


Lee, Terry <terry.lee@...>
 

Could the package maintainer merge this patch? Thanks.

Terry

-----Original Message-----
From: Stefan Berger [mailto:stefanb@...]
Sent: Friday, July 10, 2020 7:27 AM
To: devel@edk2.groups.io; lersek@...; Gao, Zhichao <zhichao.gao@...>
Cc: Lee, Terry <terry.lee@...>; Yao, Jiewen <jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>; Zhang, Chao B <chao.b.zhang@...>; Marc-André Lureau <marcandre.lureau@...>
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@...>
Cc: Terry Lee <terry.lee@...>; Yao, Jiewen
<jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>;
Zhang, Chao B <chao.b.zhang@...>
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@...>

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@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Chao Zhang <chao.b.zhang@...>
Signed-off-by: Zhichao Gao <zhichao.gao@...>
---
.../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






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@...>
Sent: Friday, October 16, 2020 12:59 AM
To: devel@edk2.groups.io; stefanb@...; lersek@...; Gao,
Zhichao <zhichao.gao@...>
Cc: Yao, Jiewen <jiewen.yao@...>; Wang, Jian J
<jian.j.wang@...>; Zhang, Chao B <chao.b.zhang@...>; Marc-
André Lureau <marcandre.lureau@...>
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@...]
Sent: Friday, July 10, 2020 7:27 AM
To: devel@edk2.groups.io; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Lee, Terry <terry.lee@...>; Yao, Jiewen <jiewen.yao@...>;
Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...>
Cc: Terry Lee <terry.lee@...>; Yao, Jiewen
<jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>;
Zhang, Chao B <chao.b.zhang@...>
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@...>

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@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Chao Zhang <chao.b.zhang@...>
Signed-off-by: Zhichao Gao <zhichao.gao@...>
---
.../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






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@...>
Sent: Friday, October 16, 2020 10:25 AM
To: Yao, Jiewen <jiewen.yao@...>; devel@edk2.groups.io;
stefanb@...; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...]
Sent: Thursday, October 15, 2020 6:09 PM
To: Lee, Terry <terry.lee@...>; devel@edk2.groups.io;
stefanb@...; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...>
Sent: Friday, October 16, 2020 12:59 AM
To: devel@edk2.groups.io; stefanb@...; lersek@...;
Gao, Zhichao <zhichao.gao@...>
Cc: Yao, Jiewen <jiewen.yao@...>; Wang, Jian J
<jian.j.wang@...>; Zhang, Chao B <chao.b.zhang@...>; Marc-
André Lureau <marcandre.lureau@...>
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@...]
Sent: Friday, July 10, 2020 7:27 AM
To: devel@edk2.groups.io; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Lee, Terry <terry.lee@...>; Yao, Jiewen
<jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>; Zhang,
Chao B <chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...>
Cc: Terry Lee <terry.lee@...>; Yao, Jiewen
<jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>;
Zhang, Chao B <chao.b.zhang@...>
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@...>

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@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Chao Zhang <chao.b.zhang@...>
Signed-off-by: Zhichao Gao <zhichao.gao@...>
---
.../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






Yao, Jiewen
 

Fair enough.

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

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@...>
Sent: Friday, October 16, 2020 1:32 PM
To: Yao, Jiewen <jiewen.yao@...>; devel@edk2.groups.io;
stefanb@...; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...]
Sent: Thursday, October 15, 2020 7:31 PM
To: Lee, Terry <terry.lee@...>; devel@edk2.groups.io;
stefanb@...; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...>
Sent: Friday, October 16, 2020 10:25 AM
To: Yao, Jiewen <jiewen.yao@...>; devel@edk2.groups.io;
stefanb@...; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...]
Sent: Thursday, October 15, 2020 6:09 PM
To: Lee, Terry <terry.lee@...>; devel@edk2.groups.io;
stefanb@...; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...>
Sent: Friday, October 16, 2020 12:59 AM
To: devel@edk2.groups.io; stefanb@...; lersek@...;
Gao, Zhichao <zhichao.gao@...>
Cc: Yao, Jiewen <jiewen.yao@...>; Wang, Jian J
<jian.j.wang@...>; Zhang, Chao B <chao.b.zhang@...>;
Marc- André Lureau <marcandre.lureau@...>
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@...]
Sent: Friday, July 10, 2020 7:27 AM
To: devel@edk2.groups.io; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Lee, Terry <terry.lee@...>; Yao, Jiewen
<jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>; Zhang,
Chao B <chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...>
Cc: Terry Lee <terry.lee@...>; Yao, Jiewen
<jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>;
Zhang, Chao B <chao.b.zhang@...>
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@...>

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@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Chao Zhang <chao.b.zhang@...>
Signed-off-by: Zhichao Gao <zhichao.gao@...>
---
.../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






Lee, Terry <terry.lee@...>
 

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@...]
Sent: Thursday, October 15, 2020 7:31 PM
To: Lee, Terry <terry.lee@...>; devel@edk2.groups.io; stefanb@...; lersek@...; Gao, Zhichao <zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B <chao.b.zhang@...>; Marc-André Lureau <marcandre.lureau@...>
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@...>
Sent: Friday, October 16, 2020 10:25 AM
To: Yao, Jiewen <jiewen.yao@...>; devel@edk2.groups.io;
stefanb@...; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...]
Sent: Thursday, October 15, 2020 6:09 PM
To: Lee, Terry <terry.lee@...>; devel@edk2.groups.io;
stefanb@...; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...>
Sent: Friday, October 16, 2020 12:59 AM
To: devel@edk2.groups.io; stefanb@...; lersek@...;
Gao, Zhichao <zhichao.gao@...>
Cc: Yao, Jiewen <jiewen.yao@...>; Wang, Jian J
<jian.j.wang@...>; Zhang, Chao B <chao.b.zhang@...>;
Marc- André Lureau <marcandre.lureau@...>
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@...]
Sent: Friday, July 10, 2020 7:27 AM
To: devel@edk2.groups.io; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Lee, Terry <terry.lee@...>; Yao, Jiewen
<jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>; Zhang,
Chao B <chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...>
Cc: Terry Lee <terry.lee@...>; Yao, Jiewen
<jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>;
Zhang, Chao B <chao.b.zhang@...>
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@...>

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@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Chao Zhang <chao.b.zhang@...>
Signed-off-by: Zhichao Gao <zhichao.gao@...>
---
.../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






Lee, Terry <terry.lee@...>
 

Jiewen,

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

Terry

-----Original Message-----
From: Yao, Jiewen [mailto:jiewen.yao@...]
Sent: Thursday, October 15, 2020 6:09 PM
To: Lee, Terry <terry.lee@...>; devel@edk2.groups.io; stefanb@...; lersek@...; Gao, Zhichao <zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B <chao.b.zhang@...>; Marc-André Lureau <marcandre.lureau@...>
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@...>
Sent: Friday, October 16, 2020 12:59 AM
To: devel@edk2.groups.io; stefanb@...; lersek@...;
Gao, Zhichao <zhichao.gao@...>
Cc: Yao, Jiewen <jiewen.yao@...>; Wang, Jian J
<jian.j.wang@...>; Zhang, Chao B <chao.b.zhang@...>; Marc-
André Lureau <marcandre.lureau@...>
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@...]
Sent: Friday, July 10, 2020 7:27 AM
To: devel@edk2.groups.io; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Lee, Terry <terry.lee@...>; Yao, Jiewen
<jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>; Zhang,
Chao B <chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...>
Cc: Terry Lee <terry.lee@...>; Yao, Jiewen
<jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>;
Zhang, Chao B <chao.b.zhang@...>
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@...>

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@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Chao Zhang <chao.b.zhang@...>
Signed-off-by: Zhichao Gao <zhichao.gao@...>
---
.../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






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@...>; devel@edk2.groups.io;
stefanb@...; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix
incorrect TCG VER comparision

Fair enough.

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

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@...>
Sent: Friday, October 16, 2020 1:32 PM
To: Yao, Jiewen <jiewen.yao@...>; devel@edk2.groups.io;
stefanb@...; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...]
Sent: Thursday, October 15, 2020 7:31 PM
To: Lee, Terry <terry.lee@...>; devel@edk2.groups.io;
stefanb@...; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...>
Sent: Friday, October 16, 2020 10:25 AM
To: Yao, Jiewen <jiewen.yao@...>; devel@edk2.groups.io;
stefanb@...; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...]
Sent: Thursday, October 15, 2020 6:09 PM
To: Lee, Terry <terry.lee@...>; devel@edk2.groups.io;
stefanb@...; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...>
Sent: Friday, October 16, 2020 12:59 AM
To: devel@edk2.groups.io; stefanb@...; lersek@...;
Gao, Zhichao <zhichao.gao@...>
Cc: Yao, Jiewen <jiewen.yao@...>; Wang, Jian J
<jian.j.wang@...>; Zhang, Chao B <chao.b.zhang@...>;
Marc- André Lureau <marcandre.lureau@...>
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@...]
Sent: Friday, July 10, 2020 7:27 AM
To: devel@edk2.groups.io; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Lee, Terry <terry.lee@...>; Yao, Jiewen
<jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>; Zhang,
Chao B <chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...>
Cc: Terry Lee <terry.lee@...>; Yao, Jiewen
<jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>;
Zhang, Chao B <chao.b.zhang@...>
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@...>

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@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Chao Zhang <chao.b.zhang@...>
Signed-off-by: Zhichao Gao <zhichao.gao@...>
---
.../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









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@...>; devel@edk2.groups.io;
stefanb@...; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix
incorrect TCG VER comparision

Fair enough.

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

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@...>
Sent: Friday, October 16, 2020 1:32 PM
To: Yao, Jiewen <jiewen.yao@...>; devel@edk2.groups.io;
stefanb@...; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...]
Sent: Thursday, October 15, 2020 7:31 PM
To: Lee, Terry <terry.lee@...>; devel@edk2.groups.io;
stefanb@...; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...>
Sent: Friday, October 16, 2020 10:25 AM
To: Yao, Jiewen <jiewen.yao@...>; devel@edk2.groups.io;
stefanb@...; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...]
Sent: Thursday, October 15, 2020 6:09 PM
To: Lee, Terry <terry.lee@...>; devel@edk2.groups.io;
stefanb@...; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...>
Sent: Friday, October 16, 2020 12:59 AM
To: devel@edk2.groups.io; stefanb@...; lersek@...;
Gao, Zhichao <zhichao.gao@...>
Cc: Yao, Jiewen <jiewen.yao@...>; Wang, Jian J
<jian.j.wang@...>; Zhang, Chao B <chao.b.zhang@...>;
Marc- André Lureau <marcandre.lureau@...>
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@...]
Sent: Friday, July 10, 2020 7:27 AM
To: devel@edk2.groups.io; lersek@...; Gao, Zhichao
<zhichao.gao@...>
Cc: Lee, Terry <terry.lee@...>; Yao, Jiewen
<jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>; Zhang,
Chao B <chao.b.zhang@...>; Marc-André Lureau
<marcandre.lureau@...>
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@...>
Cc: Terry Lee <terry.lee@...>; Yao, Jiewen
<jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>;
Zhang, Chao B <chao.b.zhang@...>
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@...>

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@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Chao Zhang <chao.b.zhang@...>
Signed-off-by: Zhichao Gao <zhichao.gao@...>
---
.../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