From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web12.16027.1598974669614163812 for ; Tue, 01 Sep 2020 08:37:49 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=b74u2KYg; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: philmd@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1598974668; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:autocrypt:autocrypt; bh=5FUBWqxl7tgsf61BcY0fMB7wgC0y6ilCQh//s/tMinE=; b=b74u2KYgx43xNfUO0QiQbtov0X9tD6GiHMj7bHTwXewOoFPov3pu/2j4xLuXuWdK48Ks7y QjPkLHteb/LSLOSJVqFkgXy3LUaXAGkCHCqSwP4oiRkSIh/Q+7oqEMp02mpIWMYwOp9z5s Ju+8DO2i1kmil4hAKj9kEWuwEHUM8TY= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-17-r31T6mOBNNK99NBW3wFyTg-1; Tue, 01 Sep 2020 11:37:44 -0400 X-MC-Unique: r31T6mOBNNK99NBW3wFyTg-1 Received: by mail-wr1-f71.google.com with SMTP id e14so728675wrr.7 for ; Tue, 01 Sep 2020 08:37:43 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:autocrypt :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=5FUBWqxl7tgsf61BcY0fMB7wgC0y6ilCQh//s/tMinE=; b=m1jToG6doXFXwIwx+ijWgby5bJWA1Et7LEhQlDXmE6gf93J2eF2qbMwO9L4+uq75za b2QeNusgLRyVbPuJIjkEATgKCcVJYngZz4EJOS0a1wyTlQ3p96AvSiibmns9j37mTnrw so+i/L9jsg8U1ri3dezjWjZaGOvojqVyWCP9qIC5tvV3O6uvAY+wyyI+YjDw76voMjzo BW55tjXmEk12p/ydmIjQRlwp9ycyT9y8yairnA6REGaN220mGaY89umxEFTLvWUc3vpX GWvYnoMSq5vV+nrX0/kqcR14DFG3GAYQTfkEQKfc4LRdMZm+itKYq9IZA7D35t+W50zm lSZw== X-Gm-Message-State: AOAM530c9YDg8PU6PmKLr+X6cVjHNx675Hp3pI8Ph7A641J3QQ6j3wE6 X+TGq6oxTkkBzqyNERNo83OmfjbbDc9APUTwC2DpM0EUUzzvX7KCqWYzG0E2DjQ2wPZKOJ3lLKK ySVQLIJyHnwiOow== X-Received: by 2002:a5d:5273:: with SMTP id l19mr2719557wrc.64.1598974663028; Tue, 01 Sep 2020 08:37:43 -0700 (PDT) X-Google-Smtp-Source: ABdhPJy3lkP6PgHr0+zp1phOpukqWpTxMPLe52UvBsErWsayoUUQfeyz6+gAVdRrFAoq4ojydGX1dA== X-Received: by 2002:a5d:5273:: with SMTP id l19mr2719538wrc.64.1598974662808; Tue, 01 Sep 2020 08:37:42 -0700 (PDT) Return-Path: Received: from [192.168.1.36] (50.red-83-52-54.dynamicip.rima-tde.net. [83.52.54.50]) by smtp.gmail.com with ESMTPSA id p9sm2662107wrt.21.2020.09.01.08.37.41 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 01 Sep 2020 08:37:42 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd, SecDataDirLeft To: devel@edk2.groups.io, lersek@redhat.com Cc: Jian J Wang , Jiewen Yao , Min Xu , Wenyi Xie References: <20200901091221.20948-1-lersek@redhat.com> <20200901091221.20948-2-lersek@redhat.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Autocrypt: addr=philmd@redhat.com; keydata= mQINBDXML8YBEADXCtUkDBKQvNsQA7sDpw6YLE/1tKHwm24A1au9Hfy/OFmkpzo+MD+dYc+7 bvnqWAeGweq2SDq8zbzFZ1gJBd6+e5v1a/UrTxvwBk51yEkadrpRbi+r2bDpTJwXc/uEtYAB GvsTZMtiQVA4kRID1KCdgLa3zztPLCj5H1VZhqZsiGvXa/nMIlhvacRXdbgllPPJ72cLUkXf z1Zu4AkEKpccZaJspmLWGSzGu6UTZ7UfVeR2Hcc2KI9oZB1qthmZ1+PZyGZ/Dy+z+zklC0xl XIpQPmnfy9+/1hj1LzJ+pe3HzEodtlVA+rdttSvA6nmHKIt8Ul6b/h1DFTmUT1lN1WbAGxmg CH1O26cz5nTrzdjoqC/b8PpZiT0kO5MKKgiu5S4PRIxW2+RA4H9nq7nztNZ1Y39bDpzwE5Sp bDHzd5owmLxMLZAINtCtQuRbSOcMjZlg4zohA9TQP9krGIk+qTR+H4CV22sWldSkVtsoTaA2 qNeSJhfHQY0TyQvFbqRsSNIe2gTDzzEQ8itsmdHHE/yzhcCVvlUzXhAT6pIN0OT+cdsTTfif MIcDboys92auTuJ7U+4jWF1+WUaJ8gDL69ThAsu7mGDBbm80P3vvUZ4fQM14NkxOnuGRrJxO qjWNJ2ZUxgyHAh5TCxMLKWZoL5hpnvx3dF3Ti9HW2dsUUWICSQARAQABtDJQaGlsaXBwZSBN YXRoaWV1LURhdWTDqSAoUGhpbCkgPHBoaWxtZEByZWRoYXQuY29tPokCVQQTAQgAPwIbDwYL CQgHAwIGFQgCCQoLBBYCAwECHgECF4AWIQSJweePYB7obIZ0lcuio/1u3q3A3gUCXsfWwAUJ KtymWgAKCRCio/1u3q3A3ircD/9Vjh3aFNJ3uF3hddeoFg1H038wZr/xi8/rX27M1Vj2j9VH 0B8Olp4KUQw/hyO6kUxqkoojmzRpmzvlpZ0cUiZJo2bQIWnvScyHxFCv33kHe+YEIqoJlaQc JfKYlbCoubz+02E2A6bFD9+BvCY0LBbEj5POwyKGiDMjHKCGuzSuDRbCn0Mz4kCa7nFMF5Jv piC+JemRdiBd6102ThqgIsyGEBXuf1sy0QIVyXgaqr9O2b/0VoXpQId7yY7OJuYYxs7kQoXI 6WzSMpmuXGkmfxOgbc/L6YbzB0JOriX0iRClxu4dEUg8Bs2pNnr6huY2Ft+qb41RzCJvvMyu gS32LfN0bTZ6Qm2A8ayMtUQgnwZDSO23OKgQWZVglGliY3ezHZ6lVwC24Vjkmq/2yBSLakZE 6DZUjZzCW1nvtRK05ebyK6tofRsx8xB8pL/kcBb9nCuh70aLR+5cmE41X4O+MVJbwfP5s/RW 9BFSL3qgXuXso/3XuWTQjJJGgKhB6xXjMmb1J4q/h5IuVV4juv1Fem9sfmyrh+Wi5V1IzKI7 RPJ3KVb937eBgSENk53P0gUorwzUcO+ASEo3Z1cBKkJSPigDbeEjVfXQMzNt0oDRzpQqH2vp apo2jHnidWt8BsckuWZpxcZ9+/9obQ55DyVQHGiTN39hkETy3Emdnz1JVHTU0Q== Message-ID: <8160a251-1c39-8a93-eec8-ab5cf6441db5@redhat.com> Date: Tue, 1 Sep 2020 17:37:41 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: <20200901091221.20948-2-lersek@redhat.com> Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=philmd@redhat.com X-Mimecast-Spam-Score: 0.002 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US 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 > Cc: Jiewen Yao > Cc: Min Xu > Cc: Wenyi Xie > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215 > Signed-off-by: Laszlo Ersek Reviewed-by: Philippe Mathieu-Daude > --- > 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. > // >