From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9E11D21163270 for ; Wed, 24 Oct 2018 09:59:24 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 74172300DA38; Wed, 24 Oct 2018 16:59:23 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-116.rdu2.redhat.com [10.10.121.116]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9048E66D20; Wed, 24 Oct 2018 16:59:22 +0000 (UTC) To: Long Qin , edk2-devel@lists.01.org Cc: ting.ye@intel.com References: <20181024132202.10596-1-qin.long@intel.com> From: Laszlo Ersek Message-ID: <9f9769c6-8c3e-73d0-1a0c-06d81de27704@redhat.com> Date: Wed, 24 Oct 2018 18:59:21 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181024132202.10596-1-qin.long@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.47]); Wed, 24 Oct 2018 16:59:23 +0000 (UTC) Subject: Re: [PATCH] CryptoPkg/BaseCryptLib: Fix potential integer overflow issue. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Oct 2018 16:59:24 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/24/18 15:22, Long Qin wrote: > The LookupFreeMemRegion() in RuntimeMemAllocate.c is used to look-up > free memory region for runtime resource allocation, which was designed > to support runtime authenticated variable service. > The direct offset subtractions in this function may bring possible > integer overflow issue. > > This patch is to add the extra parameter checks to remove this possible > overflow risk. > > Cc: Ye Ting > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Long Qin > --- > .../Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c b/CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c > index 463f2bf855..92bb9ddccd 100644 > --- a/CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c > +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c > @@ -2,7 +2,7 @@ > Light-weight Memory Management Routines for OpenSSL-based Crypto > Library at Runtime Phase. > > -Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.
> +Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD License > which accompanies this distribution. The full text of the license may be found at > @@ -141,6 +141,12 @@ LookupFreeMemRegion ( > > StartPageIndex = RT_SIZE_TO_PAGES (mRTPageTable->LastEmptyPageOffset); > ReqPages = RT_SIZE_TO_PAGES (AllocationSize); > + if (ReqPages > mRTPageTable->PageCount) { > + // > + // No enough region for object allocation. > + // > + return (UINTN)(-1); > + } > > // > // Look up the free memory region with in current memory map table. > @@ -176,6 +182,12 @@ LookupFreeMemRegion ( > // Look up the free memory region from the beginning of the memory table > // until the StartCursorOffset > // > + if (ReqPages > StartPageIndex) { > + // > + // No enough region for object allocation. > + // > + return (UINTN)(-1); > + } > for (Index = 0; Index < (StartPageIndex - ReqPages); ) { > // > // Check Consecutive ReqPages Pages. > As far as I can see, "RuntimeCryptLib.inf" (where this file is used) is only linked into runtime DXE modules -- not SMM modules. That means this issue is not a security bug, because runtime DXE modules can be overwritten by the OS anyway. (They reside in normal RAM.) Can you please confirm? Nonetheless, it would be nice to explain in the commit message, what exactly "ReqPages" depends on. If needed, please file a BZ as well. (I'm not saying it's required, but you might want to consider it, and reference it in the commit message.) Thanks Laszlo