Re: [PATCH] MdeModulePkg/MemoryProtection: split protect and unprotect paths

["Yao, Jiewen" <jiewen.yao@intel.com>]

Good idea.

Reviewed-by: Jiewen.yao@Intel.com



> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Tuesday, March 21, 2017 9:49 PM
> To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Zeng, Star <star.zeng@intel.com>; Tian, Feng <feng.tian@intel.com>; Gao,
> Liming <liming.gao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: [PATCH] MdeModulePkg/MemoryProtection: split protect and
> unprotect paths
> 
> Currently, the PE/COFF image memory protection code uses the same code
> paths for protecting and unprotecting an image. This is strange, since
> unprotecting an image involves a single call into the CPU arch protocol
> to clear the permission attributes of the entire range, and there is no
> need to parse the PE/COFF headers again.
> 
> So let's store the ImageRecord entries in a linked list, so we can find
> it again at unprotect time, and simply clear the permissions.
> 
> Note that this fixes a DEBUG hang on an ASSERT() that occurs when the
> PE/COFF image fails to load, which causes UnprotectUefiImage() to be
> invoked before the image is fully loaded.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> 
> This is an alternative fix for the issue addressed by patch
> 
>    'MdeModulePkg/DxeCore: ignore PdbPointer if ImageAddress == 0'
> 
> (https://lists.01.org/pipermail/edk2-devel/2017-March/008766.html)
> 
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 85 ++++++++------------
>  1 file changed, 35 insertions(+), 50 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index 451cc35b9290..93f96f0c9502 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -74,6 +74,8 @@ UINT32   mImageProtectionPolicy;
> 
>  extern LIST_ENTRY         mGcdMemorySpaceMap;
> 
> +STATIC LIST_ENTRY         mProtectedImageRecordList;
> +
>  /**
>    Sort code section in image record, based upon CodeSegmentBase from low to
> high.
> 
> @@ -238,13 +240,10 @@ SetUefiImageMemoryAttributes (
>    Set UEFI image protection attributes.
> 
>    @param[in]  ImageRecord    A UEFI image record
> -  @param[in]  Protect        TRUE:  Protect the UEFI image.
> -                             FALSE: Unprotect the UEFI image.
>  **/
>  VOID
>  SetUefiImageProtectionAttributes (
> -  IN IMAGE_PROPERTIES_RECORD     *ImageRecord,
> -  IN BOOLEAN                     Protect
> +  IN IMAGE_PROPERTIES_RECORD     *ImageRecord
>    )
>  {
>    IMAGE_PROPERTIES_RECORD_CODE_SECTION
> *ImageRecordCodeSection;
> @@ -253,7 +252,6 @@ SetUefiImageProtectionAttributes (
>    LIST_ENTRY
> *ImageRecordCodeSectionList;
>    UINT64                                    CurrentBase;
>    UINT64                                    ImageEnd;
> -  UINT64                                    Attribute;
> 
>    ImageRecordCodeSectionList = &ImageRecord->CodeSegmentList;
> 
> @@ -276,29 +274,19 @@ SetUefiImageProtectionAttributes (
>        //
>        // DATA
>        //
> -      if (Protect) {
> -        Attribute = EFI_MEMORY_XP;
> -      } else {
> -        Attribute = 0;
> -      }
>        SetUefiImageMemoryAttributes (
>          CurrentBase,
>          ImageRecordCodeSection->CodeSegmentBase - CurrentBase,
> -        Attribute
> +        EFI_MEMORY_XP
>          );
>      }
>      //
>      // CODE
>      //
> -    if (Protect) {
> -      Attribute = EFI_MEMORY_RO;
> -    } else {
> -      Attribute = 0;
> -    }
>      SetUefiImageMemoryAttributes (
>        ImageRecordCodeSection->CodeSegmentBase,
>        ImageRecordCodeSection->CodeSegmentSize,
> -      Attribute
> +      EFI_MEMORY_RO
>        );
>      CurrentBase = ImageRecordCodeSection->CodeSegmentBase +
> ImageRecordCodeSection->CodeSegmentSize;
>    }
> @@ -310,15 +298,10 @@ SetUefiImageProtectionAttributes (
>      //
>      // DATA
>      //
> -    if (Protect) {
> -      Attribute = EFI_MEMORY_XP;
> -    } else {
> -      Attribute = 0;
> -    }
>      SetUefiImageMemoryAttributes (
>        CurrentBase,
>        ImageEnd - CurrentBase,
> -      Attribute
> +      EFI_MEMORY_XP
>        );
>    }
>    return ;
> @@ -401,18 +384,15 @@ FreeImageRecord (
>  }
> 
>  /**
> -  Protect or unprotect UEFI image common function.
> +  Protect UEFI PE/COFF image
> 
>    @param[in]  LoadedImage              The loaded image protocol
>    @param[in]  LoadedImageDevicePath    The loaded image device path
> protocol
> -  @param[in]  Protect                  TRUE:  Protect the UEFI image.
> -                                       FALSE: Unprotect the UEFI image.
>  **/
>  VOID
> -ProtectUefiImageCommon (
> +ProtectUefiImage (
>    IN EFI_LOADED_IMAGE_PROTOCOL   *LoadedImage,
> -  IN EFI_DEVICE_PATH_PROTOCOL    *LoadedImageDevicePath,
> -  IN BOOLEAN                     Protect
> +  IN EFI_DEVICE_PATH_PROTOCOL    *LoadedImageDevicePath
>    )
>  {
>    VOID                                 *ImageAddress;
> @@ -617,35 +597,18 @@ ProtectUefiImageCommon (
>    //
>    // CPU ARCH present. Update memory attribute directly.
>    //
> -  SetUefiImageProtectionAttributes (ImageRecord, Protect);
> +  SetUefiImageProtectionAttributes (ImageRecord);
> 
>    //
> -  // Clean up
> +  // Record the image record in the list so we can undo the protections later
>    //
> -  FreeImageRecord (ImageRecord);
> +  InsertTailList (&mProtectedImageRecordList, &ImageRecord->Link);
> 
>  Finish:
>    return ;
>  }
> 
>  /**
> -  Protect UEFI image.
> -
> -  @param[in]  LoadedImage              The loaded image protocol
> -  @param[in]  LoadedImageDevicePath    The loaded image device path
> protocol
> -**/
> -VOID
> -ProtectUefiImage (
> -  IN EFI_LOADED_IMAGE_PROTOCOL   *LoadedImage,
> -  IN EFI_DEVICE_PATH_PROTOCOL    *LoadedImageDevicePath
> -  )
> -{
> -  if (PcdGet32(PcdImageProtectionPolicy) != 0) {
> -    ProtectUefiImageCommon (LoadedImage, LoadedImageDevicePath,
> TRUE);
> -  }
> -}
> -
> -/**
>    Unprotect UEFI image.
> 
>    @param[in]  LoadedImage              The loaded image protocol
> @@ -657,8 +620,28 @@ UnprotectUefiImage (
>    IN EFI_DEVICE_PATH_PROTOCOL    *LoadedImageDevicePath
>    )
>  {
> +  IMAGE_PROPERTIES_RECORD    *ImageRecord;
> +  LIST_ENTRY                 *ImageRecordLink;
> +
>    if (PcdGet32(PcdImageProtectionPolicy) != 0) {
> -    ProtectUefiImageCommon (LoadedImage, LoadedImageDevicePath,
> FALSE);
> +    for (ImageRecordLink = mProtectedImageRecordList.ForwardLink;
> +         ImageRecordLink != &mProtectedImageRecordList;
> +         ImageRecordLink = ImageRecordLink->ForwardLink) {
> +      ImageRecord = CR (
> +                      ImageRecordLink,
> +                      IMAGE_PROPERTIES_RECORD,
> +                      Link,
> +                      IMAGE_PROPERTIES_RECORD_SIGNATURE
> +                      );
> +
> +      if (ImageRecord->ImageBase ==
> (EFI_PHYSICAL_ADDRESS)(UINTN)LoadedImage->ImageBase) {
> +        SetUefiImageMemoryAttributes (ImageRecord->ImageBase,
> +                                      ImageRecord->ImageSize,
> +                                      0);
> +        FreeImageRecord (ImageRecord);
> +        return;
> +      }
> +    }
>    }
>  }
> 
> @@ -1027,6 +1010,8 @@ CoreInitializeMemoryProtection (
> 
>    mImageProtectionPolicy = PcdGet32(PcdImageProtectionPolicy);
> 
> +  InitializeListHead (&mProtectedImageRecordList);
> +
>    //
>    // Sanity check the PcdDxeNxMemoryProtectionPolicy setting:
>    // - code regions should have no EFI_MEMORY_XP attribute
> --
> 2.7.4