From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by mx.groups.io with SMTP id smtpd.web08.11825.1660527910504765113 for ; Sun, 14 Aug 2022 18:45:11 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: huawei.com, ip: 45.249.212.255, mailfrom: xiewenyi2@huawei.com) Received: from dggemv703-chm.china.huawei.com (unknown [172.30.72.53]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4M5cT9022tz1M7sx; Mon, 15 Aug 2022 09:41:49 +0800 (CST) Received: from kwepemm600004.china.huawei.com (7.193.23.242) by dggemv703-chm.china.huawei.com (10.3.19.46) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Mon, 15 Aug 2022 09:45:06 +0800 Received: from [10.174.253.58] (10.174.253.58) by kwepemm600004.china.huawei.com (7.193.23.242) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Mon, 15 Aug 2022 09:45:05 +0800 Subject: =?UTF-8?B?UmU6IOWbnuWkjTogW2VkazItZGV2ZWxdIFtQQVRDSCBFREsyIHYxIDEvMV0gTWRlTW9kdWxlUGtnL1BpU21tQ29yZTpBdm9pZCBvdmVyZmxvdyByaXNr?= To: gaoliming , , , , CC: References: <20220805072037.1868254-1-xiewenyi2@huawei.com> <20220805072037.1868254-2-xiewenyi2@huawei.com> <00f601d8b044$12c56260$38502720$@byosoft.com.cn> From: "wenyi,xie" Message-ID: <7bd1bccd-5d3d-ca23-3078-39a02159a4b1@huawei.com> Date: Mon, 15 Aug 2022 09:44:56 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.0.1 MIME-Version: 1.0 In-Reply-To: <00f601d8b044$12c56260$38502720$@byosoft.com.cn> X-Originating-IP: [10.174.253.58] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To kwepemm600004.china.huawei.com (7.193.23.242) X-CFilter-Loop: Reflected Content-Type: text/plain; charset="gbk" Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 2022/8/15 9:12, gaoliming wrote: > Wenyi: > I add my comments below.=20 Thank you for your comments. I agree with you that overflow can be treated = as overlap. I will update the patch and send it later. Thanks Wenyi >=20 >> -----=D3=CA=BC=FE=D4=AD=BC=FE----- >> =B7=A2=BC=FE=C8=CB: devel@edk2.groups.io =B4=FA= =B1=ED wenyi,xie via >> groups.io >> =B7=A2=CB=CD=CA=B1=BC=E4: 2022=C4=EA8=D4=C25=C8=D5 15:21 >> =CA=D5=BC=FE=C8=CB: devel@edk2.groups.io; jian.j.wang@intel.com; >> gaoliming@byosoft.com.cn; eric.dong@intel.com; ray.ni@intel.com >> =B3=AD=CB=CD: songdongkuang@huawei.com; xiewenyi2@huawei.com >> =D6=F7=CC=E2: [edk2-devel] [PATCH EDK2 v1 1/1] MdeModulePkg/PiSmmCore:Av= oid >> overflow risk >> >> As the CommunicationBuffer plus BufferSize may overflow, check the >> value first before using. >> >> Cc: Jian J Wang >> Cc: Liming Gao >> Cc: Eric Dong >> Cc: Ray Ni >> Signed-off-by: Wenyi Xie >> --- >> MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 22 +++++++++++++------- >> MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 5 +++++ >> 2 files changed, 19 insertions(+), 8 deletions(-) >> >> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c >> b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c >> index 9e5c6cbe33dd..fcf8c61d7f1b 100644 >> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c >> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c >> @@ -613,23 +613,28 @@ SmmEndOfS3ResumeHandler ( >> @retval FALSE Buffer doesn't overlap. >> >> **/ >> -BOOLEAN >> +EFI_STATUS >> InternalIsBufferOverlapped ( >> IN UINT8 *Buff1, >> IN UINTN Size1, >> IN UINT8 *Buff2, >> - IN UINTN Size2 >> + IN UINTN Size2, >> + IN BOOLEAN *IsOverlapped >> ) >> { >> + *IsOverlapped =3D TRUE; >> + if (((UINTN)Buff1 > MAX_UINTN - Size1) || ((UINTN)Buff2 > MAX_UINTN - >> Size2)) { >> + return EFI_INVALID_PARAMETER; >> + } >> // >> // If buff1's end is less than the start of buff2, then it's ok. >> // Also, if buff1's start is beyond buff2's end, then it's ok. >> // >> if (((Buff1 + Size1) <=3D Buff2) || (Buff1 >=3D (Buff2 + Size2))) { >> - return FALSE; >> + *IsOverlapped =3D FALSE; >> } >> >> - return TRUE; >> + return EFI_SUCCESS; >> } >> > If Buff1 + Size1 overflows MAX_UINTN, it can be also regarded as overlap, > return TRUE.=20 >=20 > If you think overflow is not overlap, please also update function > InternalIsBufferOverlapped name and description to avoid confuse.=20 >=20 > Thanks > Liming >=20 >> /** >> @@ -693,17 +698,18 @@ SmmEntryPoint ( >> // >> // Synchronous SMI for SMM Core or request from Communicate >> protocol >> // >> - IsOverlapped =3D InternalIsBufferOverlapped ( >> + Status =3D InternalIsBufferOverlapped ( >> (UINT8 *)CommunicationBuffer, >> BufferSize, >> (UINT8 *)gSmmCorePrivate, >> - sizeof (*gSmmCorePrivate) >> + sizeof (*gSmmCorePrivate), >> + &IsOverlapped >> ); >> - if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer, >> BufferSize) || IsOverlapped) { >> + if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer, >> BufferSize) || EFI_ERROR(Status) || IsOverlapped) { >> // >> // If CommunicationBuffer is not in valid address scope, >> // or there is overlap between gSmmCorePrivate and >> CommunicationBuffer, >> - // return EFI_INVALID_PARAMETER >> + // return EFI_ACCESS_DENIED >> // >> gSmmCorePrivate->CommunicationBuffer =3D NULL; >> gSmmCorePrivate->ReturnStatus =3D EFI_ACCESS_DENIED; >> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c >> b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c >> index 4f00cebaf5ed..bd13cf97ec93 100644 >> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c >> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c >> @@ -525,6 +525,11 @@ SmmCommunicationCommunicate ( >> >> CommunicateHeader =3D (EFI_SMM_COMMUNICATE_HEADER >> *)CommBuffer; >> >> + if (CommunicateHeader->MessageLength > MAX_UINTN - OFFSET_OF >> (EFI_SMM_COMMUNICATE_HEADER, Data)) { >> + DEBUG ((DEBUG_ERROR, "MessageLength is invalid!\n")); >> + return EFI_INVALID_PARAMETER; >> + } >> + >> if (CommSize =3D=3D NULL) { >> TempCommSize =3D OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, >> Data) + CommunicateHeader->MessageLength; >> } else { >> -- >> 2.20.1.windows.1 >> >> >> >>=20 >> >=20 >=20 >=20 > . >=20