The function EnforceMemoryMapAttribute() in the SMM MAT logic will
ensure that the CODE and DATA memory types have the desired attributes.
The consumer of the SMM MAT should only override the Attributes field
in the MAT if it is nonzero. 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;
+ if (MemoryAttribute == 0) {
+ 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;
+ }
}
//
--
2.42.0.windows.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110659): https://edk2.groups.io/g/devel/message/110659
Mute This Topic: https://groups.io/mt/102368851/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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? 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. > > 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()? > + 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 (#110929): https://edk2.groups.io/g/devel/message/110929 Mute This Topic: https://groups.io/mt/102368851/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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.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 > -=-=-=-=-=-=-=-=-=-=-=- 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/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 11/20/23 23:57, Taylor Beebe wrote: > 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. 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). > >> 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 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 <lersek@redhat.com> then, to the next version of this patch. Thanks! Laszlo > >>> 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 >> > -=-=-=-=-=-=-=-=-=-=-=- 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/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.