From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.43, mailfrom: jian.j.wang@intel.com) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by groups.io with SMTP; Tue, 06 Aug 2019 20:21:48 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Aug 2019 20:21:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,356,1559545200"; d="scan'208";a="374261733" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga006.fm.intel.com with ESMTP; 06 Aug 2019 20:21:48 -0700 Received: from fmsmsx101.amr.corp.intel.com (10.18.124.199) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 6 Aug 2019 20:21:47 -0700 Received: from shsmsx153.ccr.corp.intel.com (10.239.6.53) by fmsmsx101.amr.corp.intel.com (10.18.124.199) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 6 Aug 2019 20:21:47 -0700 Received: from shsmsx107.ccr.corp.intel.com ([169.254.9.65]) by SHSMSX153.ccr.corp.intel.com ([169.254.12.215]) with mapi id 14.03.0439.000; Wed, 7 Aug 2019 11:21:45 +0800 From: "Wang, Jian J" To: "devel@edk2.groups.io" , "Wu, Hao A" CC: "Zeng, Star" , "Gao, Liming" Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/Variable/RuntimeDxe: Not copy SMM pointers in comm buffer Thread-Topic: [edk2-devel] [PATCH v2] MdeModulePkg/Variable/RuntimeDxe: Not copy SMM pointers in comm buffer Thread-Index: AQHVTLWh3BcnoPEFsE+AjMPtDDvtiqbvBJaw Date: Wed, 7 Aug 2019 03:21:44 +0000 Message-ID: References: <20190807001749.28220-1-hao.a.wu@intel.com> In-Reply-To: <20190807001749.28220-1-hao.a.wu@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMGJjMGU5ODAtZTMwZC00Y2ZhLWJjZjEtZjQzMTIzNTRiYzQ3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiZlNiT1drY1JuM2xaYmdqVGxWcDM5TTFLZitVZExibHpnWGJEXC9Qa3dkK1I4QWxxU21vR3ZEXC9QMjZNdzJYNUU5In0= x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: jian.j.wang@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable This is still a security bug fix, which needs CVE number added in title. Wi= th it addressed, Reviewed-by: Jian J Wang > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Wu, Hao A > Sent: Wednesday, August 07, 2019 8:18 AM > To: devel@edk2.groups.io > Cc: Wu, Hao A ; Zeng, Star ; > Gao, Liming ; Wang, Jian J > Subject: [edk2-devel] [PATCH v2] MdeModulePkg/Variable/RuntimeDxe: > Not copy SMM pointers in comm buffer >=20 > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D2002 >=20 > This commit will update the logic in function SmmVariableGetStatistics() > so that the pointer fields ('Next' and 'Name') in structure > VARIABLE_INFO_ENTRY will not be copied into the SMM communication > buffer. >=20 > Doing so will prevent SMM pointers address from being leaked into non- > SMM > environment. >=20 > Please note that newly introduced internal function CopyVarInfoEntry() > will not use CopyMem() to copy the whole VARIABLE_INFO_ENTRY > structure and > then zero out the 'Next' and 'Name' fields. This is for preventing race > conditions where the pointers value might still be read. >=20 > Cc: Star Zeng > Cc: Liming Gao > Cc: Jian J Wang > Signed-off-by: Hao A Wu > --- >=20 > V2 patch updates the type of parameter 'DstInfoEntry' from 'in' to 'out' > for the newly added function CopyVarInfoEntry(). >=20 >=20 > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 33 > ++++++++++++++++++-- > 1 file changed, 31 insertions(+), 2 deletions(-) >=20 > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c > index ec463d063e..9b59387e58 100644 > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c > @@ -312,6 +312,35 @@ GetFvbCountAndBuffer ( >=20 >=20 > /** > + Copy only the meaningful fields of the variable statistics informatio= n > from > + source buffer to the destination buffer. Other fields are filled with= zero. > + > + @param[out] DstInfoEntry A pointer to the buffer of destination > variable > + information entry. > + @param[in] SrcInfoEntry A pointer to the buffer of source variab= le > + information entry. > + > +**/ > +static > +VOID > +CopyVarInfoEntry ( > + OUT VARIABLE_INFO_ENTRY *DstInfoEntry, > + IN VARIABLE_INFO_ENTRY *SrcInfoEntry > + ) > +{ > + DstInfoEntry->Next =3D NULL; > + DstInfoEntry->Name =3D NULL; > + > + CopyGuid (&DstInfoEntry->VendorGuid, &SrcInfoEntry->VendorGuid); > + DstInfoEntry->Attributes =3D SrcInfoEntry->Attributes; > + DstInfoEntry->ReadCount =3D SrcInfoEntry->ReadCount; > + DstInfoEntry->WriteCount =3D SrcInfoEntry->WriteCount; > + DstInfoEntry->DeleteCount =3D SrcInfoEntry->DeleteCount; > + DstInfoEntry->CacheCount =3D SrcInfoEntry->CacheCount; > + DstInfoEntry->Volatile =3D SrcInfoEntry->Volatile; > +} > + > +/** > Get the variable statistics information from the information buffer > pointed by gVariableInfo. >=20 > Caution: This function may be invoked at SMM runtime. > @@ -373,7 +402,7 @@ SmmVariableGetStatistics ( > *InfoSize =3D StatisticsInfoSize; > return EFI_BUFFER_TOO_SMALL; > } > - CopyMem (InfoEntry, VariableInfo, sizeof (VARIABLE_INFO_ENTRY)); > + CopyVarInfoEntry (InfoEntry, VariableInfo); > CopyMem (InfoName, VariableInfo->Name, NameSize); > *InfoSize =3D StatisticsInfoSize; > return EFI_SUCCESS; > @@ -413,7 +442,7 @@ SmmVariableGetStatistics ( > return EFI_BUFFER_TOO_SMALL; > } >=20 > - CopyMem (InfoEntry, VariableInfo, sizeof (VARIABLE_INFO_ENTRY)); > + CopyVarInfoEntry (InfoEntry, VariableInfo); > CopyMem (InfoName, VariableInfo->Name, NameSize); > *InfoSize =3D StatisticsInfoSize; >=20 > -- > 2.12.0.windows.1 >=20 >=20 >=20