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/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 MATSo 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 EfiRuntimeServicesCode and
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 field of 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.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Signed-off-by: Taylor Beebe <taylor.d.beebe@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