From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0b-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by mx.groups.io with SMTP id smtpd.web10.11845.1594391228247833242 for ; Fri, 10 Jul 2020 07:27:08 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: linux.ibm.com, ip: 148.163.158.5, mailfrom: stefanb@linux.ibm.com) Received: from pps.filterd (m0098417.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 06AE2WMV147484; Fri, 10 Jul 2020 10:27:07 -0400 Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 326bpsebkx-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 10 Jul 2020 10:27:07 -0400 Received: from m0098417.ppops.net (m0098417.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.36/8.16.0.36) with SMTP id 06AE3vIH151909; Fri, 10 Jul 2020 10:27:07 -0400 Received: from ppma02dal.us.ibm.com (a.bd.3ea9.ip4.static.sl-reverse.com [169.62.189.10]) by mx0a-001b2d01.pphosted.com with ESMTP id 326bpsebkn-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 10 Jul 2020 10:27:07 -0400 Received: from pps.filterd (ppma02dal.us.ibm.com [127.0.0.1]) by ppma02dal.us.ibm.com (8.16.0.42/8.16.0.42) with SMTP id 06AEJhHV010787; Fri, 10 Jul 2020 14:27:06 GMT Received: from b01cxnp22036.gho.pok.ibm.com (b01cxnp22036.gho.pok.ibm.com [9.57.198.26]) by ppma02dal.us.ibm.com with ESMTP id 326bbupmxa-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 10 Jul 2020 14:27:06 +0000 Received: from b01ledav006.gho.pok.ibm.com (b01ledav006.gho.pok.ibm.com [9.57.199.111]) by b01cxnp22036.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 06AER5k139256356 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 10 Jul 2020 14:27:05 GMT Received: from b01ledav006.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 64CBDAC05E; Fri, 10 Jul 2020 14:27:05 +0000 (GMT) Received: from b01ledav006.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 524F1AC059; Fri, 10 Jul 2020 14:27:05 +0000 (GMT) Received: from sbct-3.pok.ibm.com (unknown [9.47.158.153]) by b01ledav006.gho.pok.ibm.com (Postfix) with ESMTP; Fri, 10 Jul 2020 14:27:05 +0000 (GMT) Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER comparision From: "Stefan Berger" To: devel@edk2.groups.io, lersek@redhat.com, "Gao, Zhichao" Cc: Terry Lee , "Yao, Jiewen" , "Wang, Jian J" , "Zhang, Chao B" , =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= Reply-To: devel@edk2.groups.io, stefanb@linux.ibm.com References: <20200709024647.31672-1-zhichao.gao@intel.com> <280267d2-3d61-04a6-26da-96fd46ad5439@redhat.com> <1620688EE0DC3449.7755@groups.io> Message-ID: <6db37279-ddd0-4adf-6439-403ac90dd1e9@linux.ibm.com> Date: Fri, 10 Jul 2020 10:27:05 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <1620688EE0DC3449.7755@groups.io> X-TM-AS-GCONF: 00 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.235,18.0.687 definitions=2020-07-10_07:2020-07-10,2020-07-10 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 clxscore=1015 spamscore=0 mlxlogscore=999 impostorscore=0 priorityscore=1501 lowpriorityscore=0 bulkscore=0 malwarescore=0 mlxscore=0 phishscore=0 adultscore=0 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2006250000 definitions=main-2007100095 X-MIME-Autoconverted: from 8bit to quoted-printable by mx0a-001b2d01.pphosted.com id 06AE2WMV147484 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 7/10/20 9:53 AM, Stefan Berger wrote: > On 7/10/20 1:43 AM, Laszlo Ersek wrote: >> (+Marc-Andr=C3=A9, Stefan) >> >> On 07/10/20 02:44, Gao, Zhichao wrote: >>> This bug is not obeserved by me. But I view the code. The condition=20 >>> is incorrect and it would affect the TCG operation: >>> =C2=A0=C2=A0=C2=A0=C2=A0 if (!mIsTcg2PPVerLowerThan_1_3) { >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (OperationRequest = <=20 >>> TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION) { >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // TCG2 P= P1.3 spec defined operations that are reserved=20 >>> or un-implemented >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return TC= G_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED; >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } else { >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // TCG PP lower than 1.3. (= 1.0, 1.1, 1.2) >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (OperationRequest <=3D T= CG2_PHYSICAL_PRESENCE_NO_ACTION_MAX) { >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 RequestConfirme= d =3D TRUE; >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } else if (OperationRequest= <=20 >>> TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION) { >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return TCG_PP_G= ET_USER_CONFIRMATION_NOT_IMPLEMENTED; >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >>> >> 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 assig= n >> "RequestConfirmed =3D 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=20 > switch statement that handles most of the common PPI operations=20 > independent of this change, so that at least is good. > > I would say that in the worst case some of the operations not=20 > otherwise handled may have mistakenly failed or could have been=20 > executed without user confirmation/interaction. On Linux at least PPI=20 > requests can only be sent by root. I am running a somewhat dated version of edk2 (Fedora 31). The=20 operations advertised are: 0,5,14,21,22,23,24,33,96,97. All of these are= =20 individually handled in the switch statement, so there should no be any=20 impact. I am currently not aware of whether this list can be extended=20 with some sort of module. > > >> >> Thanks >> Laszlo >> >>> So I think it should be fixed. >>> >>> Thanks, >>> Zhichao >>> >>>> -----Original Message----- >>>> From: devel@edk2.groups.io On Behalf Of=20 >>>> Laszlo Ersek >>>> Sent: Thursday, July 9, 2020 6:02 PM >>>> To: devel@edk2.groups.io; Gao, Zhichao >>>> Cc: Terry Lee ; Yao, Jiewen=20 >>>> ; Wang, >>>> Jian J ; Zhang, Chao B >>>> Subject: Re: [edk2-devel] [PATCH]=20 >>>> SecurityPkg/Tcg2PhysicalPresenceLib: Fix >>>> incorrect TCG VER comparision >>>> >>>> On 07/09/20 04:46, Gao, Zhichao wrote: >>>>> From: Terry Lee >>>>> >>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2697 >>>>> >>>>> Tcg2PhysicalPresenceLibConstructor set the module variable >>>>> mIsTcg2PPVerLowerThan_1_3 with incorrect TCG version comparision. >>>>> >>>>> Cc: Jiewen Yao >>>>> Cc: Jian J Wang >>>>> Cc: Chao Zhang >>>>> Signed-off-by: Zhichao Gao >>>>> --- >>>>> .../SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c | 2 +- >>>>> =C2=A0 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git >>>>> a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPres= en=20 >>>>> >>>>> ceLib.c >>>>> b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPres= en=20 >>>>> >>>>> ceLib.c >>>>> index 1c46d5e69d..8afaa0a785 100644 >>>>> --- >>>>> a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPres= en=20 >>>>> >>>>> ceLib.c >>>>> +++=20 >>>>> b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr >>>>> +++ esenceLib.c >>>>> @@ -387,7 +387,7 @@ Tcg2PhysicalPresenceLibConstructor ( { >>>>> =C2=A0=C2=A0=C2=A0 EFI_STATUS=C2=A0 Status; >>>>> >>>>> -=C2=A0 if (AsciiStrnCmp(PP_INF_VERSION_1_2, (CHAR8 >>>>> *)PcdGetPtr(PcdTcgPhysicalPresenceInterfaceVer), >>>>> sizeof(PP_INF_VERSION_1_2) - 1) <=3D 0) { >>>>> +=C2=A0 if (AsciiStrnCmp(PP_INF_VERSION_1_2, (CHAR8 >>>>> + *)PcdGetPtr(PcdTcgPhysicalPresenceInterfaceVer), >>>>> + sizeof(PP_INF_VERSION_1_2) - 1) >=3D 0) { >>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mIsTcg2PPVerLowerThan_1_3 =3D TRUE; >>>>> =C2=A0=C2=A0=C2=A0 } >>>>> >>>>> >>>> What is the practical impact of this bug / fix? >>>> >>>> Thanks >>>> Laszlo >>>> >>>> >>>> >> >> >> > > >=20 >