From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mx.groups.io with SMTP id smtpd.web08.26224.1660721859441621606 for ; Wed, 17 Aug 2022 00:37:39 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: huawei.com, ip: 45.249.212.187, mailfrom: xiewenyi2@huawei.com) Received: from dggemv703-chm.china.huawei.com (unknown [172.30.72.55]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4M70Bs66B7zkWLs; Wed, 17 Aug 2022 15:34:13 +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; Wed, 17 Aug 2022 15:37:34 +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; Wed, 17 Aug 2022 15:37:33 +0800 Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1] MdeModulePkg/PiSmmCore:Avoid overflow risk To: "Ni, Ray" , "devel@edk2.groups.io" , "Wang, Jian J" , "Gao, Liming" , "Dong, Eric" CC: "songdongkuang@huawei.com" References: <20220815114529.665138-1-xiewenyi2@huawei.com> <20220815114529.665138-2-xiewenyi2@huawei.com> From: "wenyi,xie" Message-ID: <8f349024-e4ad-f7bd-5c95-d49a55512547@huawei.com> Date: Wed, 17 Aug 2022 15:37:29 +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: X-Originating-IP: [10.174.253.58] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To kwepemm600004.china.huawei.com (7.193.23.242) X-CFilter-Loop: Reflected Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit On 2022/8/16 9:49, Ni, Ray wrote: >> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c >> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c >> @@ -621,6 +621,9 @@ InternalIsBufferOverlapped ( >> IN UINTN Size2 >> ) >> { >> + if (((UINTN)Buff1 > MAX_UINTN - Size1) || ((UINTN)Buff2 > MAX_UINTN - >> Size2)) { >> + return TRUE; >> + } > 1. The change looks good because it avoids integer overflow in below code that adds Size1 to Buff1 and > adds Size2 to Buff2. > Can you please add comments to explain the logic? > OK, I will add comments. > >> >> + if (CommunicateHeader->MessageLength > MAX_UINTN - OFFSET_OF >> (EFI_SMM_COMMUNICATE_HEADER, Data)) { >> + return EFI_INVALID_PARAMETER; >> + } > 2. Above check avoids integer overflow in below code that adds CommunicateHeader->MessageLength > to OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data). > Can it be moved to inside the below if-clause? > > >> + >> if (CommSize == NULL) { >> TempCommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) >> + CommunicateHeader->MessageLength; >> } else { > 3. I further reviewed the else-clause logic. When CommSize is not NULL, is that needed to make sure > that *CommSize >= OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + CommunicateHeader->MessageLength? > Or is the check already in the code somewhere? > If we think the check is needed, I agree with the change #2 to have a common logic to check integer overflow. > .When CommSize is not NULL, it seems only CommSize is been used, CommunicateHeader->MessageLength is been ignored, and value of CommSize will been verified before used by SmiHandler. So I think it's no need make sure that *CommSize >= OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + CommunicateHeader->MessageLength. I will move it to inside if-clause. Thanks Wenyi >