From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.31, mailfrom: jiewen.yao@intel.com) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by groups.io with SMTP; Tue, 06 Aug 2019 20:43:36 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Aug 2019 20:43:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,356,1559545200"; d="scan'208";a="258230882" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga001.jf.intel.com with ESMTP; 06 Aug 2019 20:43:35 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) 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:43:34 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.19]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.112]) with mapi id 14.03.0439.000; Wed, 7 Aug 2019 11:43:33 +0800 From: "Yao, Jiewen" To: "devel@edk2.groups.io" , "Wang, Jian J" , "Wu, Hao A" CC: "Zeng, Star" , "Gao, Liming" , "Yao, Jiewen" 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: AQHVTLWXtGO9j26PSEi5Q2x/zN8OiqbufvIAgACG3FA= Date: Wed, 7 Aug 2019 03:43:33 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503F74640F@shsmsx102.ccr.corp.intel.com> References: <20190807001749.28220-1-hao.a.wu@intel.com> In-Reply-To: Accept-Language: zh-CN, 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.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: jiewen.yao@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable If this is considered as a security bug, we need fix all possible informati= on leak. Not only this one. For example, the SMM data is installed the DXE database, such as loaded im= age protocol. Just fixing in one place is not good enough to resolve all concern. To me, this patch is like to harden the door, but leave windows open. It is necessary but not sufficient. Thank you Yao Jiewen > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Wang, Jian J > Sent: Wednesday, August 7, 2019 11:22 AM > 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 >=20 > This is still a security bug fix, which needs CVE number added in title.= With it > addressed, >=20 > Reviewed-by: Jian J Wang >=20 > > -----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 > > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D2002 > > > > 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. > > > > Doing so will prevent SMM pointers address from being leaked into non- > > SMM > > environment. > > > > 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 rac= e > > conditions where the pointers value might still be read. > > > > Cc: Star Zeng > > Cc: Liming Gao > > Cc: Jian J Wang > > Signed-off-by: Hao A Wu > > --- > > > > V2 patch updates the type of parameter 'DstInfoEntry' from 'in' to 'ou= t' > > for the newly added function CopyVarInfoEntry(). > > > > > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 33 > > ++++++++++++++++++-- > > 1 file changed, 31 insertions(+), 2 deletions(-) > > > > 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 ( > > > > > > /** > > + Copy only the meaningful fields of the variable statistics informat= ion > > from > > + source buffer to the destination buffer. Other fields are filled wi= th zero. > > + > > + @param[out] DstInfoEntry A pointer to the buffer of destination > > variable > > + information entry. > > + @param[in] SrcInfoEntry A pointer to the buffer of source > variable > > + 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. > > > > 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; > > } > > > > - CopyMem (InfoEntry, VariableInfo, sizeof (VARIABLE_INFO_ENTRY)); > > + CopyVarInfoEntry (InfoEntry, VariableInfo); > > CopyMem (InfoName, VariableInfo->Name, NameSize); > > *InfoSize =3D StatisticsInfoSize; > > > > -- > > 2.12.0.windows.1 > > > > > > >=20 >=20 >=20