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 4E424202E6171 for ; Tue, 17 Oct 2017 00:52:41 -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 55C117E434; Tue, 17 Oct 2017 07:56:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 55C117E434 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-43.rdu2.redhat.com [10.10.120.43]) by smtp.corp.redhat.com (Postfix) with ESMTP id 271325C1A1; Tue, 17 Oct 2017 07:56:15 +0000 (UTC) To: Ruiyu Ni , edk2-devel@lists.01.org References: <20171017014654.274700-1-ruiyu.ni@intel.com> From: Laszlo Ersek Message-ID: <4cc25dc6-89dc-a0cf-6430-61bc088b9fd9@redhat.com> Date: Tue, 17 Oct 2017 09:56:15 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171017014654.274700-1-ruiyu.ni@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.27]); Tue, 17 Oct 2017 07:56:17 +0000 (UTC) Subject: Re: [PATCH] UefiCpuPkg/MtrrLib: Fix MtrrDebugPrintAllMtrrsWorker to avoid hang X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Oct 2017 07:52:42 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/17/17 03:46, Ruiyu Ni wrote: > ARRAY_SIZE(Mtrrs->Variables.Mtrr) was used in > MtrrDebugPrintAllMtrrsWorker() to parse the MTRR registers. > Instead, the actual variable MTRR count should be used. > Otherwise, the uninitialized random data in MtrrSetting may cause > MtrrLibSetMemoryType() hang. > > Steven Shi found this bug in QEMU when using Q35 chip. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ruiyu Ni > Cc: Steven Shi > Cc: Laszlo Ersek > --- > UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c > index 2fd1d0153e..cb22558103 100644 > --- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c > +++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c > @@ -2776,6 +2776,7 @@ MtrrDebugPrintAllMtrrsWorker ( > UINTN RangeCount; > UINT64 MtrrValidBitsMask; > UINT64 MtrrValidAddressMask; > + UINT32 VariableMtrrCount; > MTRR_MEMORY_RANGE Ranges[ > ARRAY_SIZE (mMtrrLibFixedMtrrTable) * sizeof (UINT64) + 2 * ARRAY_SIZE (Mtrrs->Variables.Mtrr) + 1 > ]; > @@ -2785,6 +2786,8 @@ MtrrDebugPrintAllMtrrsWorker ( > return; > } > > + VariableMtrrCount = GetVariableMtrrCountWorker (); > + > if (MtrrSetting != NULL) { > Mtrrs = MtrrSetting; > } else { > @@ -2802,7 +2805,7 @@ MtrrDebugPrintAllMtrrsWorker ( > DEBUG((DEBUG_CACHE, "Fixed MTRR[%02d] : %016lx\n", Index, Mtrrs->Fixed.Mtrr[Index])); > } > > - for (Index = 0; Index < ARRAY_SIZE (Mtrrs->Variables.Mtrr); Index++) { > + for (Index = 0; Index < VariableMtrrCount; Index++) { > if (((MSR_IA32_MTRR_PHYSMASK_REGISTER *)&Mtrrs->Variables.Mtrr[Index].Mask)->Bits.V == 0) { > // > // If mask is not valid, then do not display range > @@ -2829,11 +2832,11 @@ MtrrDebugPrintAllMtrrsWorker ( > RangeCount = 1; > > MtrrLibGetRawVariableRanges ( > - &Mtrrs->Variables, ARRAY_SIZE (Mtrrs->Variables.Mtrr), > + &Mtrrs->Variables, VariableMtrrCount, > MtrrValidBitsMask, MtrrValidAddressMask, RawVariableRanges > ); > MtrrLibApplyVariableMtrrs ( > - RawVariableRanges, ARRAY_SIZE (RawVariableRanges), > + RawVariableRanges, VariableMtrrCount, > Ranges, ARRAY_SIZE (Ranges), &RangeCount > ); > > Assuming this patch is not committed yet: Reviewed-by: Laszlo Ersek I have another (independent) comment: This function is currently named "MtrrDebugPrintAllMtrrsWorker", and its leading comment says "Worker function prints all MTRRs for debugging." Because of this name and the documentation, I didn't understand initially how the problem could cause a hang, given that none of the printing loops would actually access anything out-of-bounds. Some of the information printed would be garbage, but it should not cause a hang. That's when I noticed that the function actually *applies* MTRR settings, by calling MtrrLibApplyVariableMtrrs(). Even worse, the MtrrLibApplyVariableMtrrs() and MtrrLibApplyFixedMtrrs() function calls are wrapped by DEBUG_CODE(). This means that in a DEBUG/NOOPT build, the function will apply MTRR settings, and in a RELEASE build, it won't. I think this is wrong and should be fixed. A debug function (esp. one that behaves differently in DEBUG/NOOPT vs. RELEASE) should have no side effects. The current situation is similar to: ASSERT (FunctionWithSideEffects () == EXPECTED_RETURN_VALUE); which we all know is wrong. Thanks Laszlo