Re: [edk2-devel] [PATCH v4 11/14] UefiCpuPkg: Use Attribute From SMM MemoryAttributesTable if Nonzero

["Laszlo Ersek" <lersek@redhat.com>]

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-