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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106297): https://edk2.groups.io/g/devel/message/106297
Mute This Topic: https://groups.io/mt/99721320/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106705): https://edk2.groups.io/g/devel/message/106705
Mute This Topic: https://groups.io/mt/99721320/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106749): https://edk2.groups.io/g/devel/message/106749
Mute This Topic: https://groups.io/mt/99721320/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-