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

["Lee, Terry" <terry.lee@hpe.com>]

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