Re: [PATCH v1 2/2] BaseTools: GenFw: auto-set nxcompat flag

["Joey Vagedes" <joeyvagedes@microsoft.com>]

[-- Attachment #1: Type: text/plain, Size: 3755 bytes --]

Hi all,

Do you have any concerns over the changes I've made to GenFw.c as seen
above? Please let me know if you have any questions, concerns, or
improvements; I would be happy to help!

Thanks,
Joey

On Fri, Jun 23, 2023 at 8:44 AM Joey Vagedes <joey.vagedes@gmail.com> wrote:

> Automatically set the nxcompat flag in the DLL Characteristics field of
> the Optional Header of the PE32+ image. For this flag to be set
> automatically, it must, the section alignment must be evenly divisible
> by 4K (EFI_PAGE_SIZE) and no section must be executable and writable.
>
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Yuwei Chen <yuwei.chen@intel.com>
> Signed-off-by: Joey Vagedes <joeyvagedes@gmail.com>
> ---
>  BaseTools/Source/C/GenFw/GenFw.c | 59 ++++++++++++++++++++
>  1 file changed, 59 insertions(+)
>
> diff --git a/BaseTools/Source/C/GenFw/GenFw.c
> b/BaseTools/Source/C/GenFw/GenFw.c
> index 0289c8ef8a5c..4581c4233c14 100644
> --- a/BaseTools/Source/C/GenFw/GenFw.c
> +++ b/BaseTools/Source/C/GenFw/GenFw.c
> @@ -441,6 +441,60 @@ Returns:
>    return STATUS_SUCCESS;
>  }
>
> +STATIC
> +BOOLEAN
> +IsNxCompatCompliant (
> +  EFI_IMAGE_OPTIONAL_HEADER_UNION  *PeHdr
> +  )
> +/*++
> +
> +Routine Description:
> +
> +  Checks if the Pe image is nxcompat. i.e. PE is 64bit, section alignment
> is
> +  evenly divisible by 4k, and no section is writable and executable.
> +
> +Arguments:
> +
> +  PeHdr      The Pe header
> +
> +Returns:
> +  TRUE       The PE is nx compat compliant
> +  FALSE      The PE is not nx compat compliant
> +
> +--*/
> +{
> +  EFI_IMAGE_SECTION_HEADER     *SectionHeader;
> +  UINT32                       Index;
> +  UINT32                       Mask;
> +
> +  // Must have an optional header to perform verification
> +  if (PeHdr->Pe32.FileHeader.SizeOfOptionalHeader == 0) {
> +    return FALSE;
> +  }
> +
> +  // Verify PE is 64 bit
> +  if (!(PeHdr->Pe32.OptionalHeader.Magic ==
> EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC)) {
> +    return FALSE;
> +  }
> +
> +  // Verify Section Alignment is divisible by 4K
> +  if (!((PeHdr->Pe32Plus.OptionalHeader.SectionAlignment % EFI_PAGE_SIZE)
> == 0)) {
> +    return FALSE;
> +  }
> +
> +  // Verify sections are not Write & Execute
> +  Mask = EFI_IMAGE_SCN_MEM_EXECUTE | EFI_IMAGE_SCN_MEM_WRITE;
> +  SectionHeader = (EFI_IMAGE_SECTION_HEADER *) ((UINT8 *)
> &(PeHdr->Pe32Plus.OptionalHeader) +
> PeHdr->Pe32Plus.FileHeader.SizeOfOptionalHeader);
> +  for (Index = 0; Index < PeHdr->Pe32Plus.FileHeader.NumberOfSections;
> Index ++, SectionHeader ++) {
> +    if ((SectionHeader->Characteristics & Mask) == Mask) {
> +      return FALSE;
> +    }
> +  }
> +
> +  // Passed all requirements, return TRUE
> +  return TRUE;
> +}
> +
>  VOID
>  SetHiiResourceHeader (
>    UINT8   *HiiBinData,
> @@ -2458,6 +2512,11 @@ Returns:
>      TEImageHeader.BaseOfCode          = Optional64->BaseOfCode;
>      TEImageHeader.ImageBase           = (UINT64) (Optional64->ImageBase);
>
> +    // Set NxCompat flag
> +    if (IsNxCompatCompliant (PeHdr)) {
> +      Optional64->DllCharacteristics |=
> IMAGE_DLLCHARACTERISTICS_NX_COMPAT;
> +    }
> +
>      if (Optional64->NumberOfRvaAndSizes >
> EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC) {
>
>  TEImageHeader.DataDirectory[EFI_TE_IMAGE_DIRECTORY_ENTRY_BASERELOC].VirtualAddress
> =
> Optional64->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC].VirtualAddress;
>
>  TEImageHeader.DataDirectory[EFI_TE_IMAGE_DIRECTORY_ENTRY_BASERELOC].Size =
> Optional64->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC].Size;
> --
> 2.41.0.windows.1
>
>

[-- Attachment #2: Type: text/html, Size: 4753 bytes --]