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

["Rebecca Cran" <rebecca@bsdio.com>]

Please fix the documentation block.

It should go above the function, and use Doxygen style.


Also, the commit message doesn't make sense to me - specifically ", it 
must,"


-- 

Rebecca Cran


On 7/6/23 9:26 AM, Joey Vagedes wrote:
> 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
>