I missed this message before heading out on vacation, sorry :)
Let me know if you have more questions. I've also split the MAT fixes into
separate patches for the v5 version and will wait to hear back from you
before sending to ensure the updated notes for this patch
answer all your questions.
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 |= 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?
This is correct. Note that the MAT is installed as a config table with the
gEdkiiPiSmmMemoryAttributesTableGuid but I could not find any info
in the PI spec on this table, just in the header file. Not sure if
that's usual,
I just assumed that EdkiiPi in the guid name meant it was documented
in the spec.
In other words, SetMemMapAttributes() relies on
EnforceMemoryMapAttribute(); is that your point?
EnforceMemoryMapAttribute() is a final filter function run after
creating the
MAT from the EFI memory map and loaded image list. More info below.
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.)
PiSmmCore fetches the EFI memory map and calls SplitTable() to split each
loaded image section into its own descriptor with EFI_MEMORY_XP marking
data sections and EFI_MEMORY_RO marking code sections.
[4] The SMM MAT logic is almost identical to the DXE MAT logic but goes
a step
further and also updates the memory map descriptors which describe
image code and data sections to be of type EfiRuntimeServicesCodeand
EfiRuntimeServicesData respectively.The consolidated MAT logic more
closely follows the DXE MAT logic which identifies image code sections
by the
presence of the attribute EFI_MEMORY_RO in the descriptor and image
data sections by the presence of the attribute EFI_MEMORY_XP.
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?
It is not possible, but SetMemMapAttributes() would apply paging
attributes based on the memory type regardless of what was already
in the Attribute fieldof the SMM MAT. This worked because of [4],
but because this patch series is consolidating the DXE and SMM MAT
logic into a library this no longer works.
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.
There won't be a descriptor with nonzero attributes after
EnforceMemoryMapAttribute().
However, SetMemMapAttributes() should use the attributes present in the
SMM MAT
to apply paging protections. When I wrote this patch comment, I assumed
the DXE and
SMM MAT logic was more widely understood so describing its flow was not
necessary. I'm
realizing that the MAT logic is basically a black box nowadays :D
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<eric.d...@intel.com>
Cc: Ray Ni<ray...@intel.com>
Cc: Rahul Kumar<rahul1.ku...@intel.com>
Cc: Gerd Hoffmann<kra...@redhat.com>
Signed-off-by: Taylor Beebe<taylor.d.be...@gmail.com>
---
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index 6f498666157e..d302a9b0cbcf 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -1062,14 +1062,17 @@ SetMemMapAttributes (
MemoryMap = MemoryMapStart;
for (Index = 0; Index < MemoryMapEntryCount; Index++) {
DEBUG ((DEBUG_VERBOSE, "SetAttribute: Memory Entry - 0x%lx, 0x%x\n",
MemoryMap->PhysicalStart, MemoryMap->NumberOfPages));
- if (MemoryMap->Type == EfiRuntimeServicesCode) {
- MemoryAttribute = EFI_MEMORY_RO;
- } else {
- ASSERT ((MemoryMap->Type == EfiRuntimeServicesData) || (MemoryMap->Type
== EfiConventionalMemory));
- //
- // Set other type memory as NX.
- //
- MemoryAttribute = EFI_MEMORY_XP;
+ MemoryAttribute = 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()?
There's not really a required data flow, instead it's expected that if
the creator of the
SMM MAT populated the Attributes field then those access attributes
should be used
when the consumer sets paging attributes.
+ if (MemoryAttribute == 0) {
So this seems to identify case [1] (... or an entirely newly added
descriptor)...
+ if (MemoryMap->Type == EfiRuntimeServicesCode) {
+ MemoryAttribute = EFI_MEMORY_RO;
+ } else {
+ ASSERT ((MemoryMap->Type == EfiRuntimeServicesData) ||
(MemoryMap->Type == EfiConventionalMemory));
+ //
+ // Set other type memory as NX.
+ //
+ MemoryAttribute = EFI_MEMORY_XP;
+ }
}
//
Makes sense to me, but there are many open questions about the data
flow; I'd like the commit message to ELI5.
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111489): https://edk2.groups.io/g/devel/message/111489
Mute This Topic: https://groups.io/mt/102368851/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
