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 45B5AAC1B4B for ; Wed, 8 Nov 2023 21:57:58 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=AepOREgXzSoY/zzP/fBIcdPwk7AXUAYRFjPv2sTaEkk=; 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=1699480676; v=1; b=sOhjo1FmKsURSLD1iHNngwiyM5KEyfQlm52HKuhe98hvIPy2ucdJkzoceTZqIJfi9IlkoS4A VpclL2VsegUZDm+AYdasf2qYwDXz7QFsw965eCZvM25nPOM7yWqZ9HPqQC6/OGYf/reRh3GGs/U o8OjqwmDkVjDn02IheKkLx9E= X-Received: by 127.0.0.2 with SMTP id rEwUYY7687511xhoKLaFgOZG; Wed, 08 Nov 2023 13:57:56 -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.web11.105442.1699480675983507000 for ; Wed, 08 Nov 2023 13:57:56 -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-519-dgzMAhUCONmY2M1FnpI6HQ-1; Wed, 08 Nov 2023 16:57:51 -0500 X-MC-Unique: dgzMAhUCONmY2M1FnpI6HQ-1 X-Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (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 8635F811E7B; Wed, 8 Nov 2023 21:57:51 +0000 (UTC) X-Received: from [10.39.192.41] (unknown [10.39.192.41]) by smtp.corp.redhat.com (Postfix) with ESMTPS id CAD8840C6EB9; Wed, 8 Nov 2023 21:57:49 +0000 (UTC) Message-ID: <7afb99f9-67c1-0a4a-6d0b-5818ca017185@redhat.com> Date: Wed, 8 Nov 2023 22:57:48 +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> From: "Laszlo Ersek" In-Reply-To: <20231103171706.148-12-taylor.d.beebe@gmail.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.2 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: dhULO9br7hQZLqllQmrbbYb2x7686176AA= 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=sOhjo1Fm; 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 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? In other words, SetMemMapAttributes() relies on EnforceMemoryMapAttribute(); is that your point? > 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.) 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 field? 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. > This also allows the UEFI and SMM MAT > logic to use ImagePropertiesRecordLib instead of carrying two copies > of the image properties record manipulation logic. >=20 > 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(-) >=20 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpu= Pkg/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) || (Memory= Map->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()? > + 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) || (Memo= ryMap->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 -=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 (#110929): https://edk2.groups.io/g/devel/message/110929 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-