From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from huawei.com (huawei.com [45.249.212.191]) by mx.groups.io with SMTP id smtpd.web11.11567.1599010327448663894 for ; Tue, 01 Sep 2020 18:32:08 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: huawei.com, ip: 45.249.212.191, mailfrom: xiewenyi2@huawei.com) Received: from DGGEMS401-HUB.china.huawei.com (unknown [172.30.72.58]) by Forcepoint Email with ESMTP id 44347E756D054A34194C; Wed, 2 Sep 2020 09:32:05 +0800 (CST) Received: from [127.0.0.1] (10.174.153.72) by DGGEMS401-HUB.china.huawei.com (10.3.19.201) with Microsoft SMTP Server id 14.3.487.0; Wed, 2 Sep 2020 09:31:59 +0800 Subject: Re: [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd, SecDataDirLeft To: Laszlo Ersek , edk2-devel-groups-io CC: Jian J Wang , Jiewen Yao , Min Xu References: <20200901091221.20948-1-lersek@redhat.com> <20200901091221.20948-2-lersek@redhat.com> From: "wenyi,xie" Message-ID: <1604a8c4-b9b3-35d5-f39f-e00dadaf6268@huawei.com> Date: Wed, 2 Sep 2020 09:31:51 +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: <20200901091221.20948-2-lersek@redhat.com> X-Originating-IP: [10.174.153.72] X-CFilter-Loop: Reflected Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit On 2020/9/1 17:12, Laszlo Ersek wrote: > The following two quantities: > > SecDataDir->VirtualAddress + SecDataDir->Size > SecDataDir->VirtualAddress + SecDataDir->Size - OffSet > > are used multiple times in DxeImageVerificationHandler(). Introduce helper > variables for them: "SecDataDirEnd" and "SecDataDirLeft", respectively. > This saves us multiple calculations and significantly simplifies the code. > > Note that all three summands above have type UINT32, therefore the new > variables are also of type UINT32. > > This patch does not change behavior. > > (Note that the code already handles the case when the > > SecDataDir->VirtualAddress + SecDataDir->Size > > UINT32 addition overflows -- namely, in that case, the certificate loop is > never entered, and the corruption check right after the loop fires.) > > Cc: Jian J Wang > Cc: Jiewen Yao > Cc: Min Xu > Cc: Wenyi Xie > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215 > Signed-off-by: Laszlo Ersek Tested-by: Wenyi Xie > --- > SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > index b08fe24e85aa..377feebb205a 100644 > --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > @@ -1652,6 +1652,8 @@ DxeImageVerificationHandler ( > UINT8 *AuthData; > UINTN AuthDataSize; > EFI_IMAGE_DATA_DIRECTORY *SecDataDir; > + UINT32 SecDataDirEnd; > + UINT32 SecDataDirLeft; > UINT32 OffSet; > CHAR16 *NameStr; > RETURN_STATUS PeCoffStatus; > @@ -1849,12 +1851,14 @@ DxeImageVerificationHandler ( > // "Attribute Certificate Table". > // The first certificate starts at offset (SecDataDir->VirtualAddress) from the start of the file. > // > + SecDataDirEnd = SecDataDir->VirtualAddress + SecDataDir->Size; > for (OffSet = SecDataDir->VirtualAddress; > - OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size); > + OffSet < SecDataDirEnd; > OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength))) { > WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet); > - if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= sizeof (WIN_CERTIFICATE) || > - (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < WinCertificate->dwLength) { > + SecDataDirLeft = SecDataDirEnd - OffSet; > + if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE) || > + SecDataDirLeft < WinCertificate->dwLength) { > break; > } > > @@ -1948,7 +1952,7 @@ DxeImageVerificationHandler ( > } > } > > - if (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size)) { > + if (OffSet != SecDataDirEnd) { > // > // The Size in Certificate Table or the attribute certificate table is corrupted. > // >