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 99E67D8118F for ; Thu, 23 Nov 2023 09:54:28 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=1dZZE6c27QWBphf+eHwu/cd8/dLCjza87NF0xTBV1TE=; 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=1700733267; v=1; b=eSEqIYVM262m2QnMOyXPEn2R+4BFXpQQpEZhehHXRNXTGx64y/YRECKq8w91+sSwxBahtSI3 yofF+7Qb4xXWrF5CO8TKgBKcpFeWtcCAJPTqGDo/lxLYQs+YKc+lHw5lYv2dLGcsqHL4is2PO2B rqEaX2AQT6vzM4Hz2UAGgmAg= X-Received: by 127.0.0.2 with SMTP id Ki8VYY7687511xLSE23e4lBL; Thu, 23 Nov 2023 01:54:27 -0800 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.87944.1700733266418874450 for ; Thu, 23 Nov 2023 01:54:26 -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-6-TDMlmWdhMQqDMvOHo2gTTw-1; Thu, 23 Nov 2023 04:54:22 -0500 X-MC-Unique: TDMlmWdhMQqDMvOHo2gTTw-1 X-Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (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 CACF985A5B5; Thu, 23 Nov 2023 09:54:21 +0000 (UTC) X-Received: from [10.39.193.233] (unknown [10.39.193.233]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 5C8D4C1596F; Thu, 23 Nov 2023 09:54:20 +0000 (UTC) Message-ID: <34146be8-2fc1-1407-feb0-2704a75fc209@redhat.com> Date: Thu, 23 Nov 2023 10:54:18 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v4 11/14] UefiCpuPkg: Use Attribute From SMM MemoryAttributesTable if Nonzero To: devel@edk2.groups.io, taylor.d.beebe@gmail.com Cc: Eric Dong , Ray Ni , Rahul Kumar , Gerd Hoffmann References: <20231103171706.148-1-taylor.d.beebe@gmail.com> <20231103171706.148-12-taylor.d.beebe@gmail.com> <7afb99f9-67c1-0a4a-6d0b-5818ca017185@redhat.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.8 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: Xrc9xefRvUBVcOFuiU8YySQix7686176AA= 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=eSEqIYVM; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none) On 11/20/23 23:57, Taylor Beebe wrote: > I missed this message before heading out on vacation, sorry :) >=20 > Let me know if you have more questions. I've also split the MAT fixes int= o >=20 > separate patches for the v5 version and will wait to hear back from you >=20 > before sending to ensure the updated notes for this patch >=20 > answer all your questions. >=20 > On 11/8/2023 1:57 PM, Laszlo Ersek wrote: >> On 11/3/23 18:17, Taylor Beebe wrote: >>> The function EnforceMemoryMapAttribute() in the SMM MAT logic will >>> ensure that the CODE and DATA memory types have the desired attributes. >> EnforceMemoryMapAttribute() leaves those descriptors alone where >> Attribute is already nonzero ("PE image") [1]. >> >> For all other descriptors (i.e., where Attribute is zero), it: >> >> - either sets Attribute to EFI_MEMORY_RO [2], >> >> - or sets the EFI_MEMORY_XP *bit* in the Attribute [3] -- which is >> identical to setting Attribute to EFI_MEMORY_XP altogether, given that >> Attribute is zero to begin with. (So this |=3D operator looks like a >> thinko in the function! But that's a separate topic.) >> >>> The consumer of the SMM MAT >> So this seems to imply that the SMM MAT is produced based on >> EnforceMemoryMapAttribute(), and then SetMemMapAttributes(), modified in >> this patch, is what consumes the SMM MAT. Is that right? >=20 > This is correct. Note that the MAT is installed as a config table with th= e >=20 > gEdkiiPiSmmMemoryAttributesTableGuid but I could not find any info >=20 > in the PI spec on this table, just in the header file. Not sure if > that's usual, >=20 > I just assumed that EdkiiPi in the guid name meant it was documented >=20 > in the spec. The Edkii prefix usually means the thing is an edk2 extension. The subsequent Pi prefix likely only means that the thing extends PI in some way (and not, for example, UEFI). >=20 >> In other words, SetMemMapAttributes() relies on >> EnforceMemoryMapAttribute(); is that your point? >=20 > EnforceMemoryMapAttribute() is a final filter function run after > creating the >=20 > MAT from the EFI memory map and loaded image list. More info below. >=20 >>> should only override the Attributes field >>> in the MAT if it is nonzero. >> I don't understand -- do you mean "override the Attributes field if it >> is *zero*"? (Because, if it is nonzero, then it has been pre-populated >> by EnforceMemoryMapAttribute(), and then we should *use* it, as the >> subject line of the patch says.) >=20 > PiSmmCore fetches the EFI memory map and calls SplitTable() to split each >=20 > loaded image section into its own descriptor with EFI_MEMORY_XP marking >=20 > data sections and EFI_MEMORY_RO marking code sections. >=20 >=20 > [4] The SMM MAT logic is almost identical to the DXE MAT logic but goes > a step >=20 > further and also updates the memory map descriptors which describe >=20 > image code and data sections to be of type EfiRuntimeServicesCodeand >=20 > EfiRuntimeServicesData respectively.The consolidated MAT logic more >=20 > closely follows the DXE MAT logic which identifies image code sections > by the >=20 > presence of the attribute EFI_MEMORY_RO in the descriptor and image >=20 > data sections by the presence of the attribute EFI_MEMORY_XP. >=20 >> Further question: given that EnforceMemoryMapAttribute() exits with >> *all* descriptors having a nonzero Attribute field, how is it possible >> for SetMemMapAttributes() to see any descriptor with zero Attribute fiel= d? >=20 > It is not possible, but SetMemMapAttributes() would apply paging >=20 > attributes based on the memory type regardless of what was already >=20 > in the Attribute fieldof the SMM MAT. This worked because of [4], >=20 > but because this patch series is consolidating the DXE and SMM MAT >=20 > logic into a library this no longer works. >=20 >> I see two possibilities: >> >> - something introduces a new descriptor between >> EnforceMemoryMapAttribute() and SetMemMapAttributes() >> >> - and/or, SetMemMapAttributes() doesn't actually check the entire >> Attribute field for nullity, but only the EFI_MEMORY_ACCESS_MASK >> bitfield of it. Cases [2] and [3] above ensure the >> EFI_MEMORY_ACCESS_MASK bitmask is nonzero, but case [1] ("PE image") >> allows for an Attribute field in the descriptor that is nonzero, but >> whose EFI_MEMORY_ACCESS_MASK bitfield is zero. >=20 > There won't be a descriptor with nonzero attributes after > EnforceMemoryMapAttribute(). >=20 > However, SetMemMapAttributes() should use the attributes present in the > SMM MAT >=20 > to apply paging protections. When I wrote this patch comment, I assumed > the DXE and >=20 > SMM MAT logic was more widely understood so describing its flow was not > necessary. I'm >=20 > realizing that the MAT logic is basically a black box nowadays :D It is, to me. This rework should really be reviewed by the original MAT authors. In their absence, I can't offer a deep review. However, that should certainly not block serious bug fixes. If you can work these responses into the commit message for v5, I'll be satisfied; feel free to add Reviewed-by: Laszlo Ersek then, to the next version of this patch. Thanks! Laszlo >=20 >>> This also allows the UEFI and SMM MAT >>> logic to use ImagePropertiesRecordLib instead of carrying two copies >>> of the image properties record manipulation logic. >>> >>> Cc: Eric Dong >>> Cc: Ray Ni >>> Cc: Rahul Kumar >>> Cc: Gerd Hoffmann >>> Signed-off-by: Taylor Beebe >>> --- >>> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 19 +++++++++++---= ----- >>> 1 file changed, 11 insertions(+), 8 deletions(-) >>> >>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiC= puPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c >>> index 6f498666157e..d302a9b0cbcf 100644 >>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c >>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c >>> @@ -1062,14 +1062,17 @@ SetMemMapAttributes ( >>> MemoryMap =3D MemoryMapStart; >>> for (Index =3D 0; Index < MemoryMapEntryCount; Index++) { >>> DEBUG ((DEBUG_VERBOSE, "SetAttribute: Memory Entry - 0x%lx, 0x%x\n= ", MemoryMap->PhysicalStart, MemoryMap->NumberOfPages)); >>> - if (MemoryMap->Type =3D=3D EfiRuntimeServicesCode) { >>> - MemoryAttribute =3D EFI_MEMORY_RO; >>> - } else { >>> - ASSERT ((MemoryMap->Type =3D=3D EfiRuntimeServicesData) || (Memo= ryMap->Type =3D=3D EfiConventionalMemory)); >>> - // >>> - // Set other type memory as NX. >>> - // >>> - MemoryAttribute =3D EFI_MEMORY_XP; >>> + MemoryAttribute =3D MemoryMap->Attribute & EFI_MEMORY_ACCESS_MASK; >> OK, so this line is what makes SetMemMapAttributes() rely on >> EnforceMemoryMapAttribute(). >> >> What ensures the proper data flow, from EnforceMemoryMapAttribute() to >> SetMemMapAttributes()? >=20 > There's not really a required data flow, instead it's expected that if > the creator of the >=20 > SMM MAT populated the Attributes field then those access attributes > should be used >=20 > when the consumer sets paging attributes. >=20 >>> + if (MemoryAttribute =3D=3D 0) { >> So this seems to identify case [1] (... or an entirely newly added >> descriptor)... >> >>> + if (MemoryMap->Type =3D=3D EfiRuntimeServicesCode) { >>> + MemoryAttribute =3D EFI_MEMORY_RO; >>> + } else { >>> + ASSERT ((MemoryMap->Type =3D=3D EfiRuntimeServicesData) || (Me= moryMap->Type =3D=3D EfiConventionalMemory)); >>> + // >>> + // Set other type memory as NX. >>> + // >>> + MemoryAttribute =3D EFI_MEMORY_XP; >>> + } >>> } >>> =20 >>> // >> Makes sense to me, but there are many open questions about the data >> flow; I'd like the commit message to ELI5. >> >> Thanks >> Laszlo >> >=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 (#111665): https://edk2.groups.io/g/devel/message/111665 Mute This Topic: https://groups.io/mt/102368851/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-