On 9/1/20 11:12 AM, 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 <jian.j.w...@intel.com> > Cc: Jiewen Yao <jiewen....@intel.com> > Cc: Min Xu <min.m...@intel.com> > Cc: Wenyi Xie <xiewen...@huawei.com> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215 > Signed-off-by: Laszlo Ersek <ler...@redhat.com>
Reviewed-by: Philippe Mathieu-Daude <phi...@redhat.com> > --- > 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. > // > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64904): https://edk2.groups.io/g/devel/message/64904 Mute This Topic: https://groups.io/mt/76552540/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-