From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id EE514780091 for ; Tue, 13 Feb 2024 21:36:14 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=KhpPH9Xze9OZJgJbuoLPPtznw24GPfZ63yD+Nf1czck=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:From:To:Cc:Reply-To:References:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1707860173; v=1; b=YEDbLG/EtEdLkMf5bhdFsvcglgwuXJy38OG8/6PJNzirsn2GdcVYCmp+4I0Ncv7M18NqKREE pk6X/kNHQHExeRMTtDXakBZqRi0QbiJ2Rv5PawytKx8c8iFBNLRoUJddCcqcmZWPHHVnzOUqd2D uUs6zyg5n7ETmC0Z6mRJt/bc= X-Received: by 127.0.0.2 with SMTP id P0FbYY7687511xPQqakl8KYl; Tue, 13 Feb 2024 13:36:13 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web10.25912.1707860172740719883 for ; Tue, 13 Feb 2024 13:36:12 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-195-6H7gDFI6OZqmT0SK8wxwYw-1; Tue, 13 Feb 2024 16:36:08 -0500 X-MC-Unique: 6H7gDFI6OZqmT0SK8wxwYw-1 X-Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 7128A185A782; Tue, 13 Feb 2024 21:35:53 +0000 (UTC) X-Received: from [10.39.192.46] (unknown [10.39.192.46]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 42D7D1C060B1; Tue, 13 Feb 2024 21:35:52 +0000 (UTC) Message-ID: <5f807038-3e4b-0d82-6fee-37b81fd8e9f6@redhat.com> Date: Tue, 13 Feb 2024 22:35:51 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [edk2-stable202402 PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes From: "Laszlo Ersek" To: devel@edk2.groups.io Cc: Dun Tan , Gerd Hoffmann , Rahul Kumar , Ray Ni Reply-To: devel@edk2.groups.io,lersek@redhat.com References: <20240213210918.16372-1-lersek@redhat.com> <20240213210918.16372-2-lersek@redhat.com> In-Reply-To: <20240213210918.16372-2-lersek@redhat.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.7 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: IPjbFPHFT2G7l2DO6LGwVZ9rx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b="YEDbLG/E"; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 2/13/24 22:09, Laszlo Ersek wrote: > Commit 725acd0b9cc0 ("UefiCpuPkg: Avoid assuming only one smmbasehob", > 2023-12-12) introduced a helper function called GetSmBase(), replacing th= e > lookup of the first and only "gSmmBaseHobGuid" GUID HOB, with iterated > lookups plus memory allocation. >=20 > This introduced a new failure mode for setting "mCpuHotPlugData.SmBase". > Namely, before commit 725acd0b9cc0, "mCpuHotPlugData.SmBase" would be set > to NULL if and only if the GUID HOB was absent. After the commit, a NULL > assignment would be possible if the GUID HOB was absent, *or* one of the > memory allocations inside GetSmBase() failed. Sorry, these two paragraphs are not precise. A better version: ---------- Commit 725acd0b9cc0 ("UefiCpuPkg: Avoid assuming only one smmbasehob", 2023-12-12) introduced a helper function called GetSmBase(), replacing the lookup of the first and only "gSmmBaseHobGuid" GUID HOB and unconditional "mCpuHotPlugData.SmBase" allocation, with iterated lookups plus conditional memory allocation. This introduced a new failure mode for setting "mCpuHotPlugData.SmBase". Namely, before commit 725acd0b9cc0, "mCpuHotPlugData.SmBase" would be allocated regardless of the GUID HOB being absent. After the commit, "mCpuHotPlugData.SmBase" could remain NULL if the GUID HOB was absent, *or* one of the memory allocations inside GetSmBase() failed; and in the former case, we'd even proceed to the rest of PiCpuSmmEntry(). ---------- Sorry, it's late. If this patch set is accepted otherwise, then Mike or Liming, can you please update the first two paragraphs of the commit message upon merge? Thanks Laszlo >=20 > In relation to this conflation of distinct failure modes, commit > 725acd0b9cc0 actually introduced a NULL pointer dereference. Namely, a > NULL "mCpuHotPlugData.SmBase" is not handled properly at all now. We're > going to fix that NULL pointer dereference in a subsequent patch; however= , > as a pre-requisite for that we need to tell apart the failure modes of > GetSmBase(). >=20 > For memory allocation failures, return EFI_OUT_OF_RESOURCES. Move the > "assertion" that SMRAM cannot be exhausted happen out to the caller > (PiCpuSmmEntry()). Strengthen the assertion by adding an explicit > CpuDeadLoop() call. (Note: GetSmBase() *already* calls CpuDeadLoop() if > (NumberOfProcessors !=3D MaxNumberOfCpus).) >=20 > For the absence of the GUID HOB, return EFI_NOT_FOUND. >=20 > For good measure, make GetSmBase() STATIC (it should have been STATIC fro= m > the start). >=20 > This is just a refactoring, no behavioral difference is intended (beyond > the explicit CpuDeadLoop() upon SMRAM exhaustion). >=20 > Cc: Dun Tan > Cc: Gerd Hoffmann > Cc: Rahul Kumar > Cc: Ray Ni > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4682 > Signed-off-by: Laszlo Ersek > --- >=20 > Notes: > context:-U4 >=20 > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 40 ++++++++++++++------ > 1 file changed, 28 insertions(+), 12 deletions(-) >=20 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSm= mCpuDxeSmm/PiSmmCpuDxeSmm.c > index cd394826ffcf..09382945ddb4 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > @@ -619,16 +619,23 @@ SmBaseHobCompare ( > =20 > /** > Extract SmBase for all CPU from SmmBase HOB. > =20 > - @param[in] MaxNumberOfCpus Max NumberOfCpus. > + @param[in] MaxNumberOfCpus Max NumberOfCpus. > =20 > - @retval SmBaseBuffer Pointer to SmBase Buffer. > - @retval NULL gSmmBaseHobGuid was not been created. > + @param[out] AllocatedSmBaseBuffer Pointer to SmBase Buffer allocated > + by this function. Only set if the > + function returns EFI_SUCCESS. > + > + @retval EFI_SUCCESS SmBase Buffer output successfully. > + @retval EFI_OUT_OF_RESOURCES Memory allocation failed. > + @retval EFI_NOT_FOUND gSmmBaseHobGuid was never created. > **/ > -UINTN * > +STATIC > +EFI_STATUS > GetSmBase ( > - IN UINTN MaxNumberOfCpus > + IN UINTN MaxNumberOfCpus, > + OUT UINTN **AllocatedSmBaseBuffer > ) > { > UINTN HobCount; > EFI_HOB_GUID_TYPE *GuidHob; > @@ -649,9 +656,9 @@ GetSmBase ( > NumberOfProcessors =3D 0; > =20 > FirstSmmBaseGuidHob =3D GetFirstGuidHob (&gSmmBaseHobGuid); > if (FirstSmmBaseGuidHob =3D=3D NULL) { > - return NULL; > + return EFI_NOT_FOUND; > } > =20 > GuidHob =3D FirstSmmBaseGuidHob; > while (GuidHob !=3D NULL) { > @@ -671,11 +678,10 @@ GetSmBase ( > CpuDeadLoop (); > } > =20 > SmBaseHobs =3D AllocatePool (sizeof (SMM_BASE_HOB_DATA *) * HobCount); > - ASSERT (SmBaseHobs !=3D NULL); > if (SmBaseHobs =3D=3D NULL) { > - return NULL; > + return EFI_OUT_OF_RESOURCES; > } > =20 > // > // Record each SmmBaseHob pointer in the SmBaseHobs. > @@ -691,9 +697,9 @@ GetSmBase ( > SmBaseBuffer =3D (UINTN *)AllocatePool (sizeof (UINTN) * (MaxNumberOfC= pus)); > ASSERT (SmBaseBuffer !=3D NULL); > if (SmBaseBuffer =3D=3D NULL) { > FreePool (SmBaseHobs); > - return NULL; > + return EFI_OUT_OF_RESOURCES; > } > =20 > QuickSort (SmBaseHobs, HobCount, sizeof (SMM_BASE_HOB_DATA *), (BASE_S= ORT_COMPARE)SmBaseHobCompare, &SortBuffer); > PrevProcessorIndex =3D 0; > @@ -713,9 +719,10 @@ GetSmBase ( > PrevProcessorIndex +=3D SmBaseHobs[HobIndex]->NumberOfProcessors; > } > =20 > FreePool (SmBaseHobs); > - return SmBaseBuffer; > + *AllocatedSmBaseBuffer =3D SmBaseBuffer; > + return EFI_SUCCESS; > } > =20 > /** > Function to compare 2 MP_INFORMATION2_HOB_DATA pointer based on Proces= sorIndex. > @@ -1110,10 +1117,17 @@ PiCpuSmmEntry ( > // > // Retrive the allocated SmmBase from gSmmBaseHobGuid. If found, > // means the SmBase relocation has been done. > // > - mCpuHotPlugData.SmBase =3D GetSmBase (mMaxNumberOfCpus); > - if (mCpuHotPlugData.SmBase !=3D NULL) { > + mCpuHotPlugData.SmBase =3D NULL; > + Status =3D GetSmBase (mMaxNumberOfCpus, &mCpuHotPlugDa= ta.SmBase); > + if (Status =3D=3D EFI_OUT_OF_RESOURCES) { > + ASSERT (Status !=3D EFI_OUT_OF_RESOURCES); > + CpuDeadLoop (); > + } > + > + if (!EFI_ERROR (Status)) { > + ASSERT (mCpuHotPlugData.SmBase !=3D NULL); > // > // Check whether the Required TileSize is enough. > // > if (TileSize > SIZE_8KB) { > @@ -1125,8 +1139,10 @@ PiCpuSmmEntry ( > } > =20 > mSmmRelocated =3D TRUE; > } else { > + ASSERT (Status =3D=3D EFI_NOT_FOUND); > + ASSERT (mCpuHotPlugData.SmBase =3D=3D NULL); > // > // When the HOB doesn't exist, allocate new SMBASE itself. > // > DEBUG ((DEBUG_INFO, "PiCpuSmmEntry: gSmmBaseHobGuid not found!\n")); >=20 >=20 >=20 >=20 >=20 >=20 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115419): https://edk2.groups.io/g/devel/message/115419 Mute This Topic: https://groups.io/mt/104341342/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-