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 EA1F7AC0BAE for ; Mon, 9 Oct 2023 06:54:16 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=FZvBaNpo1CwXVFhigCemfeQH53oMiVW/OUT4P9srUsY=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1696834455; v=1; b=pE3qWmv3osA3sSkh6suFDOgDVpcqEosj5N2UI00+UdIC6AbzI9El8J1dFrGC9MGjVvrxvgFa YgnD9gSTOPDCrY18QCQV5sboBk5/Vw6LYMXZmLBiO7xpte7F3DZctNhkSn+rqlwsBgQxp/Q23au nIEL3Wh3tbBfVRdsMLFA9jfA= X-Received: by 127.0.0.2 with SMTP id 73JpYY7687511x0GHr6hXXC5; Sun, 08 Oct 2023 23:54:15 -0700 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.55453.1696834454869598767 for ; Sun, 08 Oct 2023 23:54:15 -0700 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-256-cDHHozdGNdK-c5kQ8AoRKg-1; Mon, 09 Oct 2023 02:54:12 -0400 X-MC-Unique: cDHHozdGNdK-c5kQ8AoRKg-1 X-Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id BF44A29AB451; Mon, 9 Oct 2023 06:54:11 +0000 (UTC) X-Received: from [10.39.192.114] (unknown [10.39.192.114]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C6ABC140E962; Mon, 9 Oct 2023 06:54:10 +0000 (UTC) Message-ID: <5f91a481-3ffd-8263-1c05-27e731af27fa@redhat.com> Date: Mon, 9 Oct 2023 08:54:09 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v5 06/28] MdeModulePkg: Apply Protections to the HOB List To: devel@edk2.groups.io, taylor.d.beebe@gmail.com Cc: Jian J Wang , Liming Gao , Dandan Bi References: <20231009000742.1792-1-taylor.d.beebe@gmail.com> <20231009000742.1792-7-taylor.d.beebe@gmail.com> From: "Laszlo Ersek" In-Reply-To: <20231009000742.1792-7-taylor.d.beebe@gmail.com> X-Scanned-By: MIMEDefang 3.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 Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 6kAmmnUwMyDDaUJwplFTEgF9x7686176AA= 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=pE3qWmv3; 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 10/9/23 02:07, Taylor Beebe wrote: > Because the platform memory protection settings will be stored > in the HOB, the HOB list should be marked read-only and non-executable > as soon as possible in boot. >=20 > This patch page-aligns the allocated HOB list in DXE and marks > it RO/NX during memory protection initialization. >=20 > Signed-off-by: Taylor Beebe > Cc: Jian J Wang > Cc: Liming Gao > Cc: Dandan Bi > --- > MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 18 ++++++------ > MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 29 ++++++++++++++++++++ > 2 files changed, 38 insertions(+), 9 deletions(-) >=20 > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/= Gcd.c > index 792cd2e0af23..72bd036eab1e 100644 > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c > @@ -2764,21 +2764,21 @@ CoreInitializeGcdServices ( > } > =20 > // > - // Relocate HOB List to an allocated pool buffer. > + // Relocate HOB List to allocated pages. > // The relocation should be at after all the tested memory resources a= dded > // (except the memory space that covers HOB List) to the memory servic= es, > // because the memory resource found in CoreInitializeMemoryServices() > // may have not enough remaining resource for HOB List. > // > - NewHobList =3D AllocateCopyPool ( > - (UINTN)PhitHob->EfiFreeMemoryBottom - (UINTN)(*HobStart= ), > - *HobStart > - ); > - ASSERT (NewHobList !=3D NULL); > - > - *HobStart =3D NewHobList; > - gHobList =3D NewHobList; > + NewHobList =3D AllocatePages (EFI_SIZE_TO_PAGES ((UINTN)PhitHob->EfiFr= eeMemoryBottom - (UINTN)(*HobStart))); > + if (NewHobList !=3D NULL) { > + CopyMem (NewHobList, *HobStart, (UINTN)PhitHob->EfiFreeMemoryBottom = - (UINTN)(*HobStart)); > + *HobStart =3D NewHobList; > + } else { > + ASSERT (NewHobList !=3D NULL); > + } > =20 > + gHobList =3D *HobStart; > if (MemorySpaceMapHobList !=3D NULL) { > // > // Add and allocate the memory space that covers HOB List to the mem= ory services I suggest using this as an opportunity to eliminate one instance of ASSERT-for-error-checking. > diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg= /Core/Dxe/Misc/MemoryProtection.c > index 7cc829b17402..94ed3111688b 100644 > --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c > +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c > @@ -967,6 +967,32 @@ InitializeDxeNxMemoryProtectionPolicy ( > } > } > =20 > +/** > + Mark the HOB list as read-only and non-executable. > +**/ > +STATIC > +VOID > +ProtectHobList ( > + VOID > + ) > +{ > + EFI_PEI_HOB_POINTERS Hob; > + > + Hob.Raw =3D GetHobList (); > + > + // Find the end of the HOB list. > + while (!END_OF_HOB_LIST (Hob)) { > + Hob.Raw =3D GET_NEXT_HOB (Hob); > + } > + > + // Protect the HOB list. > + SetUefiImageMemoryAttributes ( > + (UINTN)gHobList, > + ALIGN_VALUE (((UINTN)Hob.Raw + GET_HOB_LENGTH (Hob)) - (UINTN)GetHob= List (), EFI_PAGE_SIZE), > + EFI_MEMORY_XP | EFI_MEMORY_RO > + ); > +} > + > /** > A notification for CPU_ARCH protocol. > =20 > @@ -1007,6 +1033,9 @@ MemoryProtectionCpuArchProtocolNotify ( > // > HeapGuardCpuArchProtocolNotify (); > =20 > + // Mark the HOB list XP and RO. > + ProtectHobList (); > + > if (mImageProtectionPolicy =3D=3D 0) { > goto Done; > } I was curious if this step was consistent with the HOB Design from the PI spec, and it seems to be. In PI v1.7 volume 3, section "4.4 HOB List", we have: Only HOB producer phase components are allowed to make additions or changes to HOBs. Once the HOB list is passed into the HOB consumer phase, it is effectively read only. The ramification of a read-only HOB list is that handoff information, such as boot mode, must be handled in a distinguished fashion. For example, if the HOB consumer phase were to engender a recovery condition, it would not update the boot mode but instead would implement the action using a special type of reset call. I'm not checking your implementation, but from that r/o perspective at least: Acked-by: Laszlo Ersek -=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 (#109436): https://edk2.groups.io/g/devel/message/109436 Mute This Topic: https://groups.io/mt/101843347/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-