diff mbox series

ld/PE: restrict non-zero default DLL characteristics to MinGW

Message ID ee4bb6d4-dda5-4f6a-ab02-73aa6d3216c1@suse.com
State New
Headers
Series ld/PE: restrict non-zero default DLL characteristics to MinGW |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Jan Beulich April 4, 2025, 10:07 a.m. UTC 
  While commit ef6379e16dd1 ("Set the default DLL chracteristics to 0 for
Cygwin based targets") tried to undo the too broad earlier 514b4e191d5f
("Change the default characteristics of DLLs built by the linker to more
secure settings"), it didn't go quite far enough. Apparently the
assumption was that if it's not MinGW, it must be Cygwin. Whether it
really is okay to default three of the flags to non-zero on MinGW also
remains unclear - sadly neither of the commits came with any description
whatsoever.

Setting effectively any of the DLL characteristics flags depends on
properties of the binary being linked. While defaulting to "more secure"
is a fair goal, it's only the programmer who can know whether their code
is actually compatible with the respective settings. On the assumption
that the change of defaults was indeed deliberate (and justifiable) for
MinGW, limit them to just that. In particular, don't default any of the
flags to set also for non-MinGW, non-Cygwin targets, like e.g. UEFI. At
least the mere applicability of the high-entropy-VA bit is pretty
questionable there in the first place - UEFI applications, after all,
run in "physical mode", i.e. either unpaged or (where paging is a
requirement, like for x86-64) direct-mapped.

The situation is particularly problematic with NX-compat: Many UEFI
implementations respect the "physical mode" property, where permissions
can't be enforced anyway. Some, like reportedly OVMF, even have a build
option to behave either way. Hence successfully testing a UEFI binary on
any number of systems does not guarantee it won't crash elsewhere if the
flag is wrongly set.

Get rid of excess semicolons as well.
---
An option may be to add an umbrella option allowing to set all "more
secure" bits in one go, to simplify life for people negatively affected
by the backwards step here.

An overall better way of defaulting NX-compat may be to go from there
not being any RWX sections.

The default being a pure configure/build time decision isn't quite right
either. Even with a MinGW toolchain people may want to build UEFI
binaries, where the MinGW default better wouldn't apply, due to
aforementioned reasons. The primary goal here is to have Linux-targeting
toolchains (with PE/PEP as a secondary target) work correctly again.

It remains unclear to me in how far DLL characteristics are applicable
in the first place to images not setting IMAGE_FILE_DLL. MS
documentation does not say anything on the matter either way.

Comments

Jeremy Drake April 4, 2025, 4:59 p.m. UTC | #1 
On Fri, 4 Apr 2025, Jan Beulich wrote:

> While commit ef6379e16dd1 ("Set the default DLL chracteristics to 0 for
> Cygwin based targets") tried to undo the too broad earlier 514b4e191d5f
> ("Change the default characteristics of DLLs built by the linker to more
> secure settings"), it didn't go quite far enough. Apparently the
> assumption was that if it's not MinGW, it must be Cygwin. Whether it
> really is okay to default three of the flags to non-zero on MinGW also
> remains unclear - sadly neither of the commits came with any description
> whatsoever.

A bit of history on 514b4e191d5f is that I posted a patch on bugzilla on a
years-old bug requesting these flags changes, and was surprised that it
was applied so quickly.  I didn't write the commit message.
Jan Beulich April 8, 2025, 8:30 a.m. UTC | #2 
On 04.04.2025 12:07, Jan Beulich wrote:
> While commit ef6379e16dd1 ("Set the default DLL chracteristics to 0 for
> Cygwin based targets") tried to undo the too broad earlier 514b4e191d5f
> ("Change the default characteristics of DLLs built by the linker to more
> secure settings"), it didn't go quite far enough. Apparently the
> assumption was that if it's not MinGW, it must be Cygwin. Whether it
> really is okay to default three of the flags to non-zero on MinGW also
> remains unclear - sadly neither of the commits came with any description
> whatsoever.

It was brought to my attention that documentation also wasn't updated
when restoring the defaults for Cygwin. Hence I've now added the hunks below.

Jan

--- a/ld/ld.texi
+++ b/ld/ld.texi
@@ -3712,7 +3712,8 @@ of the PE file header:
 @item --high-entropy-va
 @itemx --disable-high-entropy-va
 Image is compatible with 64-bit address space layout randomization
-(ASLR).  This option is enabled by default for 64-bit PE images.
+(ASLR).  This option is enabled by default for 64-bit PE images in
+MinGW targets.
 
 This option also implies @option{--dynamicbase} and
 @option{--enable-reloc-section}.
@@ -3722,9 +3723,9 @@ This option also implies @option{--dynam
 @itemx --disable-dynamicbase
 The image base address may be relocated using address space layout
 randomization (ASLR).  This feature was introduced with MS Windows
-Vista for i386 PE targets.  This option is enabled by default but
-can be disabled via the @option{--disable-dynamicbase} option.
-This option also implies @option{--enable-reloc-section}.
+Vista for i386 PE targets.  This option is enabled by default for MinGW
+targets but can be disabled via the @option{--disable-dynamicbase}
+option. This option also implies @option{--enable-reloc-section}.
 
 @kindex --forceinteg
 @item --forceinteg
@@ -3737,7 +3738,7 @@ default.
 @item --disable-nxcompat
 The image is compatible with the Data Execution Prevention.
 This feature was introduced with MS Windows XP SP2 for i386 PE
-targets.  The option is enabled by default.
+targets.  The option is enabled by default for MinGW targets.
 
 @kindex --no-isolation
 @item --no-isolation
diff mbox series

Patch

--- a/ld/emultempl/pe.em
+++ b/ld/emultempl/pe.em
@@ -7,11 +7,11 @@  else
 fi
 
 case ${target} in
-  *-*-cygwin*)
-    cygwin_behavior=1
+  *-*-mingw*)
+    mingw_behavior=1
     ;;
   *)
-    cygwin_behavior=0;
+    mingw_behavior=0
     ;;
 esac
 
@@ -126,9 +126,10 @@  fragment <<EOF
 #define DEFAULT_PSEUDO_RELOC_VERSION 1
 #endif
 
-#define DEFAULT_DLL_CHARACTERISTICS	(${cygwin_behavior} ? 0 : \
-					   IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE \
-					 | IMAGE_DLL_CHARACTERISTICS_NX_COMPAT)
+#define DEFAULT_DLL_CHARACTERISTICS	(${mingw_behavior} \
+					 ? IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE \
+					   | IMAGE_DLL_CHARACTERISTICS_NX_COMPAT \
+					 : 0)
 
 #if defined(TARGET_IS_i386pe) || ! defined(DLL_SUPPORT)
 #define	PE_DEF_SUBSYSTEM		IMAGE_SUBSYSTEM_WINDOWS_CUI
--- a/ld/emultempl/pep.em
+++ b/ld/emultempl/pep.em
@@ -9,11 +9,15 @@  fi
 case ${target} in
   *-*-cygwin*)
     move_default_addr_high=1
-    cygwin_behavior=1
+    mingw_behavior=0
+    ;;
+  *-*-mingw*)
+    move_default_addr_high=0
+    mingw_behavior=1
     ;;
   *)
-    move_default_addr_high=0;
-    cygwin_behavior=0;
+    move_default_addr_high=0
+    mingw_behavior=0
     ;;
 esac
 
@@ -126,10 +130,11 @@  fragment <<EOF
 #define DLL_SUPPORT
 #endif
 
-#define DEFAULT_DLL_CHARACTERISTICS	(${cygwin_behavior} ? 0 : \
-					   IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE \
-					 | IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA \
-  					 | IMAGE_DLL_CHARACTERISTICS_NX_COMPAT)
+#define DEFAULT_DLL_CHARACTERISTICS	(${mingw_behavior} \
+					 ? IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE \
+					   | IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA \
+					   | IMAGE_DLL_CHARACTERISTICS_NX_COMPAT \
+					 : 0)
 
 #if defined(TARGET_IS_i386pep) || defined(COFF_WITH_peAArch64) || ! defined(DLL_SUPPORT)
 #define	PE_DEF_SUBSYSTEM		IMAGE_SUBSYSTEM_WINDOWS_CUI