[PATCH v1 0/2] Automatically set NXCOMPAT bit if requirements are met

[PATCH v1 0/2] Automatically set NXCOMPAT bit if requirements are met

[Joey Vagedes]

Utilize GenFw to automatically set the NXCOMPAT bit of the DLL Characteristics 
field of the Optional Header if the following requirements are met:

1. It is a 64bit PE
2. The section alignment is evently divisible by 4K
3. No section is both EFI_IMAGE_SCN_MEM_EXECUTE and EFI_IMAGE_SCN_MEM_WRITE

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Rebecca Cran <rebecca@bsdio.com>
Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Yuwei Chen <yuwei.chen@intel.com>

Joey Vagedes (2):
  MdePkg: IndustryStandard: Add DLL Characteristics
  BaseTools: GenFw: auto-set nxcompat flag

 MdePkg/Include/IndustryStandard/PeImage.h | 15 +++++
 BaseTools/Source/C/GenFw/GenFw.c          | 59 ++++++++++++++++++++
 2 files changed, 74 insertions(+)

-- 
2.41.0.windows.1

[PATCH v1 1/2] MdePkg: IndustryStandard: Add DLL Characteristics

[Joey Vagedes]

Add the bit masks for DLL Characteristics, used within the optional
header of a PE, to the PeImage.h header file.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Signed-off-by: Joey Vagedes <joeyvagedes@gmail.com>
---
 MdePkg/Include/IndustryStandard/PeImage.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/MdePkg/Include/IndustryStandard/PeImage.h b/MdePkg/Include/IndustryStandard/PeImage.h
index 47037049348c..430e8988f550 100644
--- a/MdePkg/Include/IndustryStandard/PeImage.h
+++ b/MdePkg/Include/IndustryStandard/PeImage.h
@@ -269,6 +269,21 @@ typedef struct {
 #define EFI_IMAGE_SUBSYSTEM_OS2_CUI      5
 #define EFI_IMAGE_SUBSYSTEM_POSIX_CUI    7
 
+//
+// DLL Characteristics
+//
+#define IMAGE_DLLCHARACTERISTICS_HIGH_ENTROPY_VA        0x0020
+#define IMAGE_DLLCHARACTERISTICS_DYNAMIC_BASE           0x0040
+#define IMAGE_DLLCHARACTERISTICS_FORCE_INTEGRITY        0x0080
+#define IMAGE_DLLCHARACTERISTICS_NX_COMPAT              0x0100
+#define IMAGE_DLLCHARACTERISTICS_NO_ISOLATION           0x0200
+#define IMAGE_DLLCHARACTERISTICS_NO_SEH                 0x0400
+#define IMAGE_DLLCHARACTERISTICS_NO_BIND                0x0800
+#define IMAGE_DLLCHARACTERISTICS_APPCONTAINER           0x1000
+#define IMAGE_DLLCHARACTERISTICS_WDM_DRIVER             0x2000
+#define IMAGE_DLLCHARACTERISTICS_GUARD_CF               0x4000
+#define IMAGE_DLLCHARACTERISTICS_TERMINAL_SERVER_AWARE  0x8000
+
 ///
 /// Length of ShortName.
 ///
-- 
2.41.0.windows.1

Re: [PATCH v1 1/2] MdePkg: IndustryStandard: Add DLL Characteristics

[Michael D Kinney]

Hi Joey,

Was the link to the PE/COFF specs that added these updated in the file header?

Also, shouldn't it be DLL_CHARACTERISRICS instead of DLLCHARACTERISRICS?

Mike

> -----Original Message-----
> From: Joey Vagedes <joey.vagedes@gmail.com>
> Sent: Friday, June 23, 2023 8:45 AM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>
> Subject: [PATCH v1 1/2] MdePkg: IndustryStandard: Add DLL Characteristics
> 
> Add the bit masks for DLL Characteristics, used within the optional
> header of a PE, to the PeImage.h header file.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Signed-off-by: Joey Vagedes <joeyvagedes@gmail.com>
> ---
>  MdePkg/Include/IndustryStandard/PeImage.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/MdePkg/Include/IndustryStandard/PeImage.h
> b/MdePkg/Include/IndustryStandard/PeImage.h
> index 47037049348c..430e8988f550 100644
> --- a/MdePkg/Include/IndustryStandard/PeImage.h
> +++ b/MdePkg/Include/IndustryStandard/PeImage.h
> @@ -269,6 +269,21 @@ typedef struct {
>  #define EFI_IMAGE_SUBSYSTEM_OS2_CUI      5
>  #define EFI_IMAGE_SUBSYSTEM_POSIX_CUI    7
> 
> +//
> +// DLL Characteristics
> +//
> +#define IMAGE_DLLCHARACTERISTICS_HIGH_ENTROPY_VA        0x0020
> +#define IMAGE_DLLCHARACTERISTICS_DYNAMIC_BASE           0x0040
> +#define IMAGE_DLLCHARACTERISTICS_FORCE_INTEGRITY        0x0080
> +#define IMAGE_DLLCHARACTERISTICS_NX_COMPAT              0x0100
> +#define IMAGE_DLLCHARACTERISTICS_NO_ISOLATION           0x0200
> +#define IMAGE_DLLCHARACTERISTICS_NO_SEH                 0x0400
> +#define IMAGE_DLLCHARACTERISTICS_NO_BIND                0x0800
> +#define IMAGE_DLLCHARACTERISTICS_APPCONTAINER           0x1000
> +#define IMAGE_DLLCHARACTERISTICS_WDM_DRIVER             0x2000
> +#define IMAGE_DLLCHARACTERISTICS_GUARD_CF               0x4000
> +#define IMAGE_DLLCHARACTERISTICS_TERMINAL_SERVER_AWARE  0x8000
> +
>  ///
>  /// Length of ShortName.
>  ///
> --
> 2.41.0.windows.1

Re: [PATCH v1 1/2] MdePkg: IndustryStandard: Add DLL Characteristics

[Joey Vagedes]

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

Hi Michael,

PeImage.h currently references rev 8.3 as the latest revision. The
DLLCHARACTERISTICS field of the optional header existed in the revision,
but I believe it was never added as it was not needed. I'm happy to add
information to the header if needed, just let me know what you're looking
for. Would you like me to update it to v9.3 or stay at v8.3? If we stay at
8.3, I will need to remove  IMAGE_DLLCHARACTERISTICS_HIGH_ENTROPY_VA as it
did not exist in v8.3.

as for the naming, The latest revision (9.3) of the spec has the name as
IMAGE_DLLCHARACTERISTICS_*. In revision 8.3, this is also true, except for
the first three entries which do include the space as you mentioned.

Please let me know if you would like to stay at v8.3 or move up to v9.3 and
I will update these defines appropriately.

On Tue, Jun 27, 2023 at 1:12 PM Kinney, Michael D <
michael.d.kinney@intel.com> wrote:

> Hi Joey,
>
> Was the link to the PE/COFF specs that added these updated in the file
> header?
>
> Also, shouldn't it be DLL_CHARACTERISRICS instead of DLLCHARACTERISRICS?
>
> Mike
>
> > -----Original Message-----
> > From: Joey Vagedes <joey.vagedes@gmail.com>
> > Sent: Friday, June 23, 2023 8:45 AM
> > To: devel@edk2.groups.io
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>
> > Subject: [PATCH v1 1/2] MdePkg: IndustryStandard: Add DLL Characteristics
> >
> > Add the bit masks for DLL Characteristics, used within the optional
> > header of a PE, to the PeImage.h header file.
> >
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > Signed-off-by: Joey Vagedes <joeyvagedes@gmail.com>
> > ---
> >  MdePkg/Include/IndustryStandard/PeImage.h | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/MdePkg/Include/IndustryStandard/PeImage.h
> > b/MdePkg/Include/IndustryStandard/PeImage.h
> > index 47037049348c..430e8988f550 100644
> > --- a/MdePkg/Include/IndustryStandard/PeImage.h
> > +++ b/MdePkg/Include/IndustryStandard/PeImage.h
> > @@ -269,6 +269,21 @@ typedef struct {
> >  #define EFI_IMAGE_SUBSYSTEM_OS2_CUI      5
> >  #define EFI_IMAGE_SUBSYSTEM_POSIX_CUI    7
> >
> > +//
> > +// DLL Characteristics
> > +//
> > +#define IMAGE_DLLCHARACTERISTICS_HIGH_ENTROPY_VA        0x0020
> > +#define IMAGE_DLLCHARACTERISTICS_DYNAMIC_BASE           0x0040
> > +#define IMAGE_DLLCHARACTERISTICS_FORCE_INTEGRITY        0x0080
> > +#define IMAGE_DLLCHARACTERISTICS_NX_COMPAT              0x0100
> > +#define IMAGE_DLLCHARACTERISTICS_NO_ISOLATION           0x0200
> > +#define IMAGE_DLLCHARACTERISTICS_NO_SEH                 0x0400
> > +#define IMAGE_DLLCHARACTERISTICS_NO_BIND                0x0800
> > +#define IMAGE_DLLCHARACTERISTICS_APPCONTAINER           0x1000
> > +#define IMAGE_DLLCHARACTERISTICS_WDM_DRIVER             0x2000
> > +#define IMAGE_DLLCHARACTERISTICS_GUARD_CF               0x4000
> > +#define IMAGE_DLLCHARACTERISTICS_TERMINAL_SERVER_AWARE  0x8000
> > +
> >  ///
> >  /// Length of ShortName.
> >  ///
> > --
> > 2.41.0.windows.1
>
>

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

Re: [PATCH v1 1/2] MdePkg: IndustryStandard: Add DLL Characteristics

[Michael D Kinney]

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

Sounds like 9.3 would be better for consistent names.

Mike

From: Joey Vagedes <joey.vagedes@gmail.com>
Sent: Tuesday, June 27, 2023 2:42 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: devel@edk2.groups.io; Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>
Subject: Re: [PATCH v1 1/2] MdePkg: IndustryStandard: Add DLL Characteristics

Hi Michael,

PeImage.h currently references rev 8.3 as the latest revision. The DLLCHARACTERISTICS field of the optional header existed in the revision, but I believe it was never added as it was not needed. I'm happy to add information to the header if needed, just let me know what you're looking for. Would you like me to update it to v9.3 or stay at v8.3? If we stay at 8.3, I will need to remove  IMAGE_DLLCHARACTERISTICS_HIGH_ENTROPY_VA as it did not exist in v8.3.

as for the naming, The latest revision (9.3) of the spec has the name as IMAGE_DLLCHARACTERISTICS_*. In revision 8.3, this is also true, except for the first three entries which do include the space as you mentioned.

Please let me know if you would like to stay at v8.3 or move up to v9.3 and I will update these defines appropriately.

On Tue, Jun 27, 2023 at 1:12 PM Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> wrote:
Hi Joey,

Was the link to the PE/COFF specs that added these updated in the file header?

Also, shouldn't it be DLL_CHARACTERISRICS instead of DLLCHARACTERISRICS?

Mike

> -----Original Message-----
> From: Joey Vagedes <joey.vagedes@gmail.com<mailto:joey.vagedes@gmail.com>>
> Sent: Friday, June 23, 2023 8:45 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Gao, Liming
> <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>; Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>
> Subject: [PATCH v1 1/2] MdePkg: IndustryStandard: Add DLL Characteristics
>
> Add the bit masks for DLL Characteristics, used within the optional
> header of a PE, to the PeImage.h header file.
>
> Cc: Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
> Cc: Liming Gao <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>
> Signed-off-by: Joey Vagedes <joeyvagedes@gmail.com<mailto:joeyvagedes@gmail.com>>
> ---
>  MdePkg/Include/IndustryStandard/PeImage.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/MdePkg/Include/IndustryStandard/PeImage.h
> b/MdePkg/Include/IndustryStandard/PeImage.h
> index 47037049348c..430e8988f550 100644
> --- a/MdePkg/Include/IndustryStandard/PeImage.h
> +++ b/MdePkg/Include/IndustryStandard/PeImage.h
> @@ -269,6 +269,21 @@ typedef struct {
>  #define EFI_IMAGE_SUBSYSTEM_OS2_CUI      5
>  #define EFI_IMAGE_SUBSYSTEM_POSIX_CUI    7
>
> +//
> +// DLL Characteristics
> +//
> +#define IMAGE_DLLCHARACTERISTICS_HIGH_ENTROPY_VA        0x0020
> +#define IMAGE_DLLCHARACTERISTICS_DYNAMIC_BASE           0x0040
> +#define IMAGE_DLLCHARACTERISTICS_FORCE_INTEGRITY        0x0080
> +#define IMAGE_DLLCHARACTERISTICS_NX_COMPAT              0x0100
> +#define IMAGE_DLLCHARACTERISTICS_NO_ISOLATION           0x0200
> +#define IMAGE_DLLCHARACTERISTICS_NO_SEH                 0x0400
> +#define IMAGE_DLLCHARACTERISTICS_NO_BIND                0x0800
> +#define IMAGE_DLLCHARACTERISTICS_APPCONTAINER           0x1000
> +#define IMAGE_DLLCHARACTERISTICS_WDM_DRIVER             0x2000
> +#define IMAGE_DLLCHARACTERISTICS_GUARD_CF               0x4000
> +#define IMAGE_DLLCHARACTERISTICS_TERMINAL_SERVER_AWARE  0x8000
> +
>  ///
>  /// Length of ShortName.
>  ///
> --
> 2.41.0.windows.1

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

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

[Joey Vagedes]

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

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

[Joey Vagedes]

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

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

[Rebecca Cran]

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
>

Re: [edk2-devel] [PATCH v1 0/2] Automatically set NXCOMPAT bit if requirements are met

[Ard Biesheuvel]

On Fri, 23 Jun 2023 at 18:03, Joey Vagedes <joey.vagedes@gmail.com> wrote:
>
> Utilize GenFw to automatically set the NXCOMPAT bit of the DLL Characteristics
> field of the Optional Header if the following requirements are met:
>
> 1. It is a 64bit PE
> 2. The section alignment is evently divisible by 4K
> 3. No section is both EFI_IMAGE_SCN_MEM_EXECUTE and EFI_IMAGE_SCN_MEM_WRITE
>

Is this sufficient? For example, the EBC DXE driver creates code
trampolines in page allocations, and expects them to be executable.
However, this change would flag that driver as NX compat too.

Re: [edk2-devel] [PATCH v1 0/2] Automatically set NXCOMPAT bit if requirements are met

[Joey Vagedes]

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

Hi Ard,

Talked to the team and I think the appropriate answer to this is to follow
what is currently done through the MSVC FLAGS, i.e. that any PE that
knowingly does not meet these requirements manually opts out of NXCOMPAT
via the /NXCOMPAT:no flag. That means adding a flag to GENFW: "--nxcompat
no". Then this can be appended to existing flags in the INF of any
incompatible component.

Invalid PEs could be reviewed after this patch is in, or as a part of it.
We have a list of modules that modules that are potentially non-nx compat,
but would need to be evaluated by package owners and may not be all
encompassing.

Thanks,
Joey

On Fri, Jun 23, 2023 at 9:11 AM Ard Biesheuvel <ardb@kernel.org> wrote:

> On Fri, 23 Jun 2023 at 18:03, Joey Vagedes <joey.vagedes@gmail.com> wrote:
> >
> > Utilize GenFw to automatically set the NXCOMPAT bit of the DLL
> Characteristics
> > field of the Optional Header if the following requirements are met:
> >
> > 1. It is a 64bit PE
> > 2. The section alignment is evently divisible by 4K
> > 3. No section is both EFI_IMAGE_SCN_MEM_EXECUTE and
> EFI_IMAGE_SCN_MEM_WRITE
> >
>
> Is this sufficient? For example, the EBC DXE driver creates code
> trampolines in page allocations, and expects them to be executable.
> However, this change would flag that driver as NX compat too.
>

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

回复: [edk2-devel] [PATCH v1 0/2] Automatically set NXCOMPAT bit if requirements are met

[gaoliming]

Joey:
  Can you describe the full usage of NXCOMPAT bit? This patch sets NXCOMPAT
bit. And, which module will consume NXCOMPAT bit, how use it? DxeCore?

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Joey Vagedes
> 发送时间: 2023年6月23日 23:45
> 收件人: devel@edk2.groups.io
> 抄送: Michael D Kinney <michael.d.kinney@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Zhiguang Liu <zhiguang.liu@intel.com>;
> Rebecca Cran <rebecca@bsdio.com>; Bob Feng <bob.c.feng@intel.com>;
> Yuwei Chen <yuwei.chen@intel.com>
> 主题: [edk2-devel] [PATCH v1 0/2] Automatically set NXCOMPAT bit if
> requirements are met
> 
> Utilize GenFw to automatically set the NXCOMPAT bit of the DLL
> Characteristics
> field of the Optional Header if the following requirements are met:
> 
> 1. It is a 64bit PE
> 2. The section alignment is evently divisible by 4K
> 3. No section is both EFI_IMAGE_SCN_MEM_EXECUTE and
> EFI_IMAGE_SCN_MEM_WRITE
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Yuwei Chen <yuwei.chen@intel.com>
> 
> Joey Vagedes (2):
>   MdePkg: IndustryStandard: Add DLL Characteristics
>   BaseTools: GenFw: auto-set nxcompat flag
> 
>  MdePkg/Include/IndustryStandard/PeImage.h | 15 +++++
>  BaseTools/Source/C/GenFw/GenFw.c          | 59
> ++++++++++++++++++++
>  2 files changed, 74 insertions(+)
> 
> --
> 2.41.0.windows.1
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v1 0/2] Automatically set NXCOMPAT bit if requirements are met

[Joey Vagedes]

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

Hi Liming,

This is being done as a part of the memory protections work which can be
reviewed here: Task Table · Memory Protections (github.com)
<https://github.com/orgs/tianocore/projects/3>

Overall, DxeCore will ingest the NX_COMPAT flag on image load.

In the base case, when modules make allocations of type EfiLoaderCode,
EfiBootServicesCode, and EfiRuntimeServicesCode, the EFI_MEMORY_XP access
attribute will be applied with the expectation that the allocating modules
will remove the EFI_MEMORY_XP attribute and apply the EFI_MEMORY_RO
attribute once they have loaded their code into the buffer for execution.

In the exception case, if an EFI_APPLICATION type image is loaded without
the NX_COMPAT flag then allocations of type EfiLoaderCode,
EfiBootServicesCode, and EfiRuntimeServicesCode will no longer have
EFI_MEMORY_XP applied for the remainder of boot.

Note that a patch series for the supporting DxeCore logic has not yet been
submitted.

Thanks,
Joey

On Sat, Jun 24, 2023 at 7:46 PM gaoliming <gaoliming@byosoft.com.cn> wrote:

> Joey:
>   Can you describe the full usage of NXCOMPAT bit? This patch sets NXCOMPAT
> bit. And, which module will consume NXCOMPAT bit, how use it? DxeCore?
>
> Thanks
> Liming
> > -----邮件原件-----
> > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Joey Vagedes
> > 发送时间: 2023年6月23日 23:45
> > 收件人: devel@edk2.groups.io
> > 抄送: Michael D Kinney <michael.d.kinney@intel.com>; Liming Gao
> > <gaoliming@byosoft.com.cn>; Zhiguang Liu <zhiguang.liu@intel.com>;
> > Rebecca Cran <rebecca@bsdio.com>; Bob Feng <bob.c.feng@intel.com>;
> > Yuwei Chen <yuwei.chen@intel.com>
> > 主题: [edk2-devel] [PATCH v1 0/2] Automatically set NXCOMPAT bit if
> > requirements are met
> >
> > Utilize GenFw to automatically set the NXCOMPAT bit of the DLL
> > Characteristics
> > field of the Optional Header if the following requirements are met:
> >
> > 1. It is a 64bit PE
> > 2. The section alignment is evently divisible by 4K
> > 3. No section is both EFI_IMAGE_SCN_MEM_EXECUTE and
> > EFI_IMAGE_SCN_MEM_WRITE
> >
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > Cc: Rebecca Cran <rebecca@bsdio.com>
> > Cc: Bob Feng <bob.c.feng@intel.com>
> > Cc: Yuwei Chen <yuwei.chen@intel.com>
> >
> > Joey Vagedes (2):
> >   MdePkg: IndustryStandard: Add DLL Characteristics
> >   BaseTools: GenFw: auto-set nxcompat flag
> >
> >  MdePkg/Include/IndustryStandard/PeImage.h | 15 +++++
> >  BaseTools/Source/C/GenFw/GenFw.c          | 59
> > ++++++++++++++++++++
> >  2 files changed, 74 insertions(+)
> >
> > --
> > 2.41.0.windows.1
> >
> >
> >
> > 
> >
>
>
>
>

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