From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from walk.intel-email.com (walk.intel-email.com [101.227.64.242]) by mx.groups.io with SMTP id smtpd.web10.11554.1660525949586084106 for ; Sun, 14 Aug 2022 18:12:31 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@byosoft.com.cn header.s=cloud-union header.b=Q2g4wyiJ; spf=pass (domain: byosoft.com.cn, ip: 101.227.64.242, mailfrom: gaoliming@byosoft.com.cn) Received: from walk.intel-email.com (localhost [127.0.0.1]) by walk.intel-email.com (Postfix) with ESMTP id 03712CD1F7E2 for ; Mon, 15 Aug 2022 09:12:26 +0800 (CST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=byosoft.com.cn; s=cloud-union; t=1660525946; bh=wpsWoI1g05Ykkb0qEi1y1nisDlb0BBznbjDYmDbXsk4=; h=From:To:Cc:References:In-Reply-To:Subject:Date; b=Q2g4wyiJyQdPq/Dgwh8T6yQOQds+CVSJMjMEiHFM+UjnqJivT8ffIrJX4P3WAM5kW APSgO2YjsTMa2yex5YDCOEDcSHac5OGqIJUdFnovrIr+ays3Loo+tyXuP0EQj1eFQ8 4hFwrOvo7tDPS2+oGKFkfAVkExMEwBjxRIHk5E/g= Received: from localhost (localhost [127.0.0.1]) by walk.intel-email.com (Postfix) with ESMTP id F3438CD1F7C4 for ; Mon, 15 Aug 2022 09:12:25 +0800 (CST) X-Virus-Scanned: by SpamTitan at intel-email.com Received: from walk.intel-email.com (localhost [127.0.0.1]) by walk.intel-email.com (Postfix) with ESMTP id BB269CD1F78B for ; Mon, 15 Aug 2022 09:12:25 +0800 (CST) Authentication-Results: walk.intel-email.com; none Received: from mail.byosoft.com.cn (mail.byosoft.com.cn [58.240.74.242]) by walk.intel-email.com (Postfix) with SMTP id 53707CD1F7EA for ; Mon, 15 Aug 2022 09:12:22 +0800 (CST) Received: from DESKTOPS6D0PVI ([58.246.60.130]) (envelope-sender ) by 192.168.6.13 with ESMTP for ; Mon, 15 Aug 2022 09:12:21 +0800 X-WM-Sender: gaoliming@byosoft.com.cn X-Originating-IP: 58.246.60.130 X-WM-AuthFlag: YES X-WM-AuthUser: gaoliming@byosoft.com.cn From: "gaoliming" To: , , , , Cc: References: <20220805072037.1868254-1-xiewenyi2@huawei.com> <20220805072037.1868254-2-xiewenyi2@huawei.com> In-Reply-To: <20220805072037.1868254-2-xiewenyi2@huawei.com> Subject: =?UTF-8?B?5Zue5aSNOiBbZWRrMi1kZXZlbF0gW1BBVENIIEVESzIgdjEgMS8xXSBNZGVNb2R1bGVQa2cvUGlTbW1Db3JlOkF2b2lkIG92ZXJmbG93IHJpc2s=?= Date: Mon, 15 Aug 2022 09:12:22 +0800 Message-ID: <00f601d8b044$12c56260$38502720$@byosoft.com.cn> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQKpJbIs6IL2G3ikcg51I1Po+B9BZgH6y/bNq/5k4XA= Sender: "gaoliming" Content-Type: text/plain; charset="gb2312" Content-Transfer-Encoding: quoted-printable Content-Language: zh-cn Wenyi: I add my comments below.=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:Avo= id > overflow risk >=20 > As the CommunicationBuffer plus BufferSize may overflow, check the > value first before using. >=20 > 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(-) >=20 > 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. >=20 > **/ > -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; > } >=20 > - return TRUE; > + return EFI_SUCCESS; > } >=20 If Buff1 + Size1 overflows MAX_UINTN, it can be also regarded as overlap, return TRUE.=20 If you think overflow is not overlap, please also update function InternalIsBufferOverlapped name and description to avoid confuse.=20 Thanks Liming > /** > @@ -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 ( >=20 > CommunicateHeader =3D (EFI_SMM_COMMUNICATE_HEADER > *)CommBuffer; >=20 > + 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