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

["Yao, Jiewen" <jiewen.yao@intel.com>]

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