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

[Laszlo Ersek <lersek@redhat.com>]

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