[PATCH v2] IntelFrameworkModulePkg/Csm: Set CSM memory executable

[PATCH v2] IntelFrameworkModulePkg/Csm: Set CSM memory executable

[Ruiyu Ni]

Commit b22a62be5cdc8fd19d87ec1ecfa5b28fb9be50ad
* IntelFrameworkModule/LegacyBios:Use reserved memory for legacy data
allocates reserved memory for holding legacy code/data.

But with PcdDxeNxMemoryProtectionPolicy set to certain value to
forbid execution when code is in certain type of memory, it's
possible that a platform forbids execution when code is in reserved
memory. The patch calls GCD service to allow such case otherwise
CPU exception may occur.

Code execution in BSCode area should be enabled by platform by
default.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
 .../Csm/LegacyBiosDxe/LegacyBios.c                 | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
index 8f14687b28..f86d00b53f 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
@@ -64,8 +64,9 @@ AllocateLegacyMemory (
   OUT EFI_PHYSICAL_ADDRESS      *Result
   )
 {
-  EFI_STATUS            Status;
-  EFI_PHYSICAL_ADDRESS  MemPage;
+  EFI_STATUS                      Status;
+  EFI_PHYSICAL_ADDRESS            MemPage;
+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc;
 
   //
   // Allocate Pages of memory less <= StartPageAddress
@@ -83,10 +84,21 @@ AllocateLegacyMemory (
   //
   if (!EFI_ERROR (Status)) {
     *Result = (EFI_PHYSICAL_ADDRESS) (UINTN) MemPage;
+    if (MemoryType != EfiBootServicesCode) {
+      //
+      // Make sure that the buffer can be used to store code.
+      //
+      Status = gDS->GetMemorySpaceDescriptor (MemPage, &MemDesc);
+      if (!EFI_ERROR (Status) && (MemDesc.Attributes & EFI_MEMORY_XP) != 0) {
+        Status = gDS->SetMemorySpaceAttributes (
+                        MemPage,
+                        EFI_PAGES_TO_SIZE (Pages),
+                        MemDesc.Attributes & (~EFI_MEMORY_XP)
+                        );
+      }
+    }
   }
-  //
-  // If reach here the status = EFI_SUCCESS
-  //
+
   return Status;
 }
 
-- 
2.16.1.windows.1

Re: [PATCH v2] IntelFrameworkModulePkg/Csm: Set CSM memory executable

[Laszlo Ersek]

Hi Ray,

On 08/07/18 07:58, Ruiyu Ni wrote:
> Commit b22a62be5cdc8fd19d87ec1ecfa5b28fb9be50ad
> * IntelFrameworkModule/LegacyBios:Use reserved memory for legacy data
> allocates reserved memory for holding legacy code/data.
> 
> But with PcdDxeNxMemoryProtectionPolicy set to certain value to
> forbid execution when code is in certain type of memory, it's
> possible that a platform forbids execution when code is in reserved
> memory. The patch calls GCD service to allow such case otherwise
> CPU exception may occur.
> 
> Code execution in BSCode area should be enabled by platform by
> default.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  .../Csm/LegacyBiosDxe/LegacyBios.c                 | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
> index 8f14687b28..f86d00b53f 100644
> --- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
> +++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
> @@ -64,8 +64,9 @@ AllocateLegacyMemory (
>    OUT EFI_PHYSICAL_ADDRESS      *Result
>    )
>  {
> -  EFI_STATUS            Status;
> -  EFI_PHYSICAL_ADDRESS  MemPage;
> +  EFI_STATUS                      Status;
> +  EFI_PHYSICAL_ADDRESS            MemPage;
> +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc;
>  
>    //
>    // Allocate Pages of memory less <= StartPageAddress
> @@ -83,10 +84,21 @@ AllocateLegacyMemory (
>    //
>    if (!EFI_ERROR (Status)) {
>      *Result = (EFI_PHYSICAL_ADDRESS) (UINTN) MemPage;
> +    if (MemoryType != EfiBootServicesCode) {
> +      //
> +      // Make sure that the buffer can be used to store code.
> +      //
> +      Status = gDS->GetMemorySpaceDescriptor (MemPage, &MemDesc);
> +      if (!EFI_ERROR (Status) && (MemDesc.Attributes & EFI_MEMORY_XP) != 0) {
> +        Status = gDS->SetMemorySpaceAttributes (
> +                        MemPage,
> +                        EFI_PAGES_TO_SIZE (Pages),
> +                        MemDesc.Attributes & (~EFI_MEMORY_XP)
> +                        );
> +      }
> +    }
>    }
> -  //
> -  // If reach here the status = EFI_SUCCESS
> -  //
> +
>    return Status;
>  }
>  
> 

thanks for the update. I think the commit message is now *great*.

I'm adding Jian (like Star did for v1) so he can comment on the general
idea.

I have another remark regarding the logic -- and I apologize for missing
it in v1 --: if the SetMemorySpaceAttributes() calls fails, we correctly
propagate the status to the caller; however, we leak the allocated
memory. If SetMemorySpaceAttributes() fails, we should release the
memory before we return the error.

Can you please CC Jian on v3 at once?

Thank you!
Laszlo