From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from huawei.com (huawei.com [45.249.212.190]) by mx.groups.io with SMTP id smtpd.web11.11575.1599010349001823152 for ; Tue, 01 Sep 2020 18:32:29 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: huawei.com, ip: 45.249.212.190, mailfrom: xiewenyi2@huawei.com) Received: from DGGEMS411-HUB.china.huawei.com (unknown [172.30.72.60]) by Forcepoint Email with ESMTP id 1B323EA9E4FB25E7533D; Wed, 2 Sep 2020 09:32:24 +0800 (CST) Received: from [127.0.0.1] (10.174.153.72) by DGGEMS411-HUB.china.huawei.com (10.3.19.211) with Microsoft SMTP Server id 14.3.487.0; Wed, 2 Sep 2020 09:32:18 +0800 Subject: Re: [PATCH 2/3] SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size check To: Laszlo Ersek , edk2-devel-groups-io CC: Jian J Wang , Jiewen Yao , Min Xu References: <20200901091221.20948-1-lersek@redhat.com> <20200901091221.20948-3-lersek@redhat.com> From: "wenyi,xie" Message-ID: Date: Wed, 2 Sep 2020 09:32:18 +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-3-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: > Currently the (SecDataDirLeft <= sizeof (WIN_CERTIFICATE)) check only > guards the de-referencing of the "WinCertificate" pointer. It does not > guard the calculation of the pointer itself: > > WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet); > > This is wrong; if we don't know for sure that we have enough room for a > WIN_CERTIFICATE, then even creating such a pointer, not just > de-referencing it, may invoke undefined behavior. > > Move the pointer calculation after the size check. > > 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 | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > index 377feebb205a..100739eb3eb6 100644 > --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > @@ -1855,10 +1855,12 @@ DxeImageVerificationHandler ( > for (OffSet = SecDataDir->VirtualAddress; > OffSet < SecDataDirEnd; > OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength))) { > - WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet); > SecDataDirLeft = SecDataDirEnd - OffSet; > - if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE) || > - SecDataDirLeft < WinCertificate->dwLength) { > + if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE)) { > + break; > + } > + WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet); > + if (SecDataDirLeft < WinCertificate->dwLength) { > break; > } > >