[PATCH v2 0/4] Allow default HARDENED_USERCOPY to be set at compile time

[PATCH v2 0/4] Allow default HARDENED_USERCOPY to be set at compile time

[Mel Gorman]

Changelog since v1
o Menu section rename
o Make static branch usage similar to init_on_alloc
o Change ordering of menu options

Some hardening options like HARDENED_USERCOPY can be set at boot time
and have negligible cost when disabled. The default for options like
init_on_alloc= can be set at compile time but hardened usercopy is
enabled by default if built in. This incurs overhead when a kernel
wishes to provide optional hardening but the user does not necessarily
care.

Hardening is desirable in some environments but ideally they would be opt-in
by kernel command line as hardening is typically a deliberate decision
whereas the performance overhead is not always obvious to all users.
Patches 1 and 2 move HARDENED_USERCOPY to the Kconfig.hardening and
default it to disabled. Patch 3 moves the static branch check to a fast
path similar to init_on_*. Patch 4 moves FORTIFY_SOURCE to hardening only
because the option is related to hardening and happened to be declared
near HARDENED_USERCOPY.

Building HARDENED_USERCOPY but disabled at runtime has neligible effect
within the noise. Enabling the option by default generally incurs 2-10%
of overhead depending on the workload with some extreme outliers depending
on the exact CPU. While the benchmarks are somewhat synthetic, the overhead
IO-intensive and network-intensive is easily detectable but the root cause
may not be obvious (e.g. 2-14% overhead for netperf TCP_STREAM running
over localhost with different ranges depending on the CPU).

 .../admin-guide/kernel-parameters.txt         |  4 ++-
 include/linux/thread_info.h                   |  8 +++++
 mm/usercopy.c                                 | 14 ++++----
 security/Kconfig                              | 21 ------------
 security/Kconfig.hardening                    | 33 +++++++++++++++++++
 5 files changed, 52 insertions(+), 28 deletions(-)

-- 
2.43.0

[PATCH 1/4] mm: security: Move hardened usercopy under 'Kernel hardening options'

[Mel Gorman]

There is a submenu for 'Kernel hardening options' under "Security".
Move HARDENED_USERCOPY under the hardening options as it is clearly
related.

Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
Acked-by: Paul Moore <paul@paul-moore.com>
---
 security/Kconfig           | 12 ------------
 security/Kconfig.hardening | 16 ++++++++++++++++
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/security/Kconfig b/security/Kconfig
index 28e685f53bd1..fe7346dc4bc3 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -159,18 +159,6 @@ config LSM_MMAP_MIN_ADDR
 	  this low address space will need the permission specific to the
 	  systems running LSM.
 
-config HARDENED_USERCOPY
-	bool "Harden memory copies between kernel and userspace"
-	imply STRICT_DEVMEM
-	help
-	  This option checks for obviously wrong memory regions when
-	  copying memory to/from the kernel (via copy_to_user() and
-	  copy_from_user() functions) by rejecting memory ranges that
-	  are larger than the specified heap object, span multiple
-	  separately allocated pages, are not on the process stack,
-	  or are part of the kernel text. This prevents entire classes
-	  of heap overflow exploits and similar kernel memory exposures.
-
 config FORTIFY_SOURCE
 	bool "Harden common str/mem functions against buffer overflows"
 	depends on ARCH_HAS_FORTIFY_SOURCE
diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
index c9d5ca3d8d08..9088d613d519 100644
--- a/security/Kconfig.hardening
+++ b/security/Kconfig.hardening
@@ -279,6 +279,22 @@ config ZERO_CALL_USED_REGS
 
 endmenu
 
+menu "Bounds checking"
+
+config HARDENED_USERCOPY
+	bool "Harden memory copies between kernel and userspace"
+	imply STRICT_DEVMEM
+	help
+	  This option checks for obviously wrong memory regions when
+	  copying memory to/from the kernel (via copy_to_user() and
+	  copy_from_user() functions) by rejecting memory ranges that
+	  are larger than the specified heap object, span multiple
+	  separately allocated pages, are not on the process stack,
+	  or are part of the kernel text. This prevents entire classes
+	  of heap overflow exploits and similar kernel memory exposures.
+
+endmenu
+
 menu "Hardening of kernel data structures"
 
 config LIST_HARDENED
-- 
2.43.0

[PATCH 2/4] mm: security: Allow default HARDENED_USERCOPY to be set at compile time

[Mel Gorman]

HARDENED_USERCOPY defaults to on if enabled at compile time. Allow
hardened_usercopy= default to be set at compile time similar to
init_on_alloc= and init_on_free=. The intent is that hardening
options that can be disabled at runtime can set their default at
build time.

Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 Documentation/admin-guide/kernel-parameters.txt | 4 +++-
 mm/usercopy.c                                   | 3 ++-
 security/Kconfig.hardening                      | 8 ++++++++
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 3872bc6ec49d..5d759b20540a 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1773,7 +1773,9 @@
 			allocation boundaries as a proactive defense
 			against bounds-checking flaws in the kernel's
 			copy_to_user()/copy_from_user() interface.
-		on	Perform hardened usercopy checks (default).
+			The default is determined by
+			CONFIG_HARDENED_USERCOPY_DEFAULT_ON.
+		on	Perform hardened usercopy checks.
 		off	Disable hardened usercopy checks.
 
 	hardlockup_all_cpu_backtrace=
diff --git a/mm/usercopy.c b/mm/usercopy.c
index 83c164aba6e0..4cf33305347a 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -255,7 +255,8 @@ void __check_object_size(const void *ptr, unsigned long n, bool to_user)
 }
 EXPORT_SYMBOL(__check_object_size);
 
-static bool enable_checks __initdata = true;
+static bool enable_checks __initdata =
+		IS_ENABLED(CONFIG_HARDENED_USERCOPY_DEFAULT_ON);
 
 static int __init parse_hardened_usercopy(char *str)
 {
diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
index 9088d613d519..adcc260839c7 100644
--- a/security/Kconfig.hardening
+++ b/security/Kconfig.hardening
@@ -293,6 +293,14 @@ config HARDENED_USERCOPY
 	  or are part of the kernel text. This prevents entire classes
 	  of heap overflow exploits and similar kernel memory exposures.
 
+config HARDENED_USERCOPY_DEFAULT_ON
+	bool "Harden memory copies by default"
+	depends on HARDENED_USERCOPY
+	default n
+	help
+	  This has the effect of setting "hardened_usercopy=on" on the kernel
+	  command line. This can be disabled with "hardened_usercopy=off".
+
 endmenu
 
 menu "Hardening of kernel data structures"
-- 
2.43.0

[PATCH 3/4] mm: security: Check early if HARDENED_USERCOPY is enabled

[Mel Gorman]

HARDENED_USERCOPY is checked within a function so even if disabled, the
function overhead still exists. Move the static check inline.

This is at best a micro-optimisation and any difference in performance
was within noise but it is relatively consistent with the init_on_*
implementations.

Suggested-by: Kees Cook <kees@kernel.org>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 include/linux/thread_info.h |  8 ++++++++
 mm/usercopy.c               | 11 ++++++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index cf2446c9c30d..832f6a97e64c 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -221,9 +221,17 @@ static inline int arch_within_stack_frames(const void * const stack,
 extern void __check_object_size(const void *ptr, unsigned long n,
 					bool to_user);
 
+DECLARE_STATIC_KEY_MAYBE(CONFIG_HARDENED_USERCOPY_DEFAULT_ON,
+			   validate_usercopy_range);
+
 static __always_inline void check_object_size(const void *ptr, unsigned long n,
 					      bool to_user)
 {
+	if (static_branch_maybe(CONFIG_HARDENED_USERCOPY_DEFAULT_ON,
+				&validate_usercopy_range)) {
+		return;
+	}
+
 	if (!__builtin_constant_p(n))
 		__check_object_size(ptr, n, to_user);
 }
diff --git a/mm/usercopy.c b/mm/usercopy.c
index 4cf33305347a..2e86413ed244 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -201,7 +201,9 @@ static inline void check_heap_object(const void *ptr, unsigned long n,
 	}
 }
 
-static DEFINE_STATIC_KEY_FALSE_RO(bypass_usercopy_checks);
+DEFINE_STATIC_KEY_MAYBE_RO(CONFIG_HARDENED_USERCOPY_DEFAULT_ON,
+			   validate_usercopy_range);
+EXPORT_SYMBOL(validate_usercopy_range);
 
 /*
  * Validates that the given object is:
@@ -212,9 +214,6 @@ static DEFINE_STATIC_KEY_FALSE_RO(bypass_usercopy_checks);
  */
 void __check_object_size(const void *ptr, unsigned long n, bool to_user)
 {
-	if (static_branch_unlikely(&bypass_usercopy_checks))
-		return;
-
 	/* Skip all tests if size is zero. */
 	if (!n)
 		return;
@@ -271,7 +270,9 @@ __setup("hardened_usercopy=", parse_hardened_usercopy);
 static int __init set_hardened_usercopy(void)
 {
 	if (enable_checks == false)
-		static_branch_enable(&bypass_usercopy_checks);
+		static_branch_enable(&validate_usercopy_range);
+	else
+		static_branch_disable(&validate_usercopy_range);
 	return 1;
 }
 
-- 
2.43.0

[PATCH 4/4] fortify: Move FORTIFY_SOURCE under 'Kernel hardening options'

[Mel Gorman]

FORTIFY_SOURCE is a hardening option both at build and runtime. Move
it under 'Kernel hardening options'.

Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 security/Kconfig           | 9 ---------
 security/Kconfig.hardening | 9 +++++++++
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/security/Kconfig b/security/Kconfig
index fe7346dc4bc3..bca84f839fbe 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -159,15 +159,6 @@ config LSM_MMAP_MIN_ADDR
 	  this low address space will need the permission specific to the
 	  systems running LSM.
 
-config FORTIFY_SOURCE
-	bool "Harden common str/mem functions against buffer overflows"
-	depends on ARCH_HAS_FORTIFY_SOURCE
-	# https://github.com/llvm/llvm-project/issues/53645
-	depends on !CC_IS_CLANG || !X86_32
-	help
-	  Detect overflows of buffers in common string and memory functions
-	  where the compiler can determine and validate the buffer sizes.
-
 config STATIC_USERMODEHELPER
 	bool "Force all usermode helper calls through a single binary"
 	help
diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
index adcc260839c7..e22dc1801bee 100644
--- a/security/Kconfig.hardening
+++ b/security/Kconfig.hardening
@@ -281,6 +281,15 @@ endmenu
 
 menu "Bounds checking"
 
+config FORTIFY_SOURCE
+	bool "Harden common str/mem functions against buffer overflows"
+	depends on ARCH_HAS_FORTIFY_SOURCE
+	# https://github.com/llvm/llvm-project/issues/53645
+	depends on !CC_IS_CLANG || !X86_32
+	help
+	  Detect overflows of buffers in common string and memory functions
+	  where the compiler can determine and validate the buffer sizes.
+
 config HARDENED_USERCOPY
 	bool "Harden memory copies between kernel and userspace"
 	imply STRICT_DEVMEM
-- 
2.43.0

Re: [PATCH 4/4] fortify: Move FORTIFY_SOURCE under 'Kernel hardening options'

[Paul Moore]

On Wed, Jan 22, 2025 at 12:20 PM Mel Gorman <mgorman@techsingularity.net> wrote:
>
> FORTIFY_SOURCE is a hardening option both at build and runtime. Move
> it under 'Kernel hardening options'.
>
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> ---
>  security/Kconfig           | 9 ---------
>  security/Kconfig.hardening | 9 +++++++++
>  2 files changed, 9 insertions(+), 9 deletions(-)

Acked-by: Paul Moore <paul@paul-moore.com>

-- 
paul-moore.com

Re: [PATCH 2/4] mm: security: Allow default HARDENED_USERCOPY to be set at compile time

[Kees Cook]

On Wed, Jan 22, 2025 at 05:19:23PM +0000, Mel Gorman wrote:
> HARDENED_USERCOPY defaults to on if enabled at compile time. Allow
> hardened_usercopy= default to be set at compile time similar to
> init_on_alloc= and init_on_free=. The intent is that hardening
> options that can be disabled at runtime can set their default at
> build time.
> 
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> ---
>  Documentation/admin-guide/kernel-parameters.txt | 4 +++-
>  mm/usercopy.c                                   | 3 ++-
>  security/Kconfig.hardening                      | 8 ++++++++
>  3 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 3872bc6ec49d..5d759b20540a 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1773,7 +1773,9 @@
>  			allocation boundaries as a proactive defense
>  			against bounds-checking flaws in the kernel's
>  			copy_to_user()/copy_from_user() interface.
> -		on	Perform hardened usercopy checks (default).
> +			The default is determined by
> +			CONFIG_HARDENED_USERCOPY_DEFAULT_ON.
> +		on	Perform hardened usercopy checks.
>  		off	Disable hardened usercopy checks.
>  
>  	hardlockup_all_cpu_backtrace=
> diff --git a/mm/usercopy.c b/mm/usercopy.c
> index 83c164aba6e0..4cf33305347a 100644
> --- a/mm/usercopy.c
> +++ b/mm/usercopy.c
> @@ -255,7 +255,8 @@ void __check_object_size(const void *ptr, unsigned long n, bool to_user)
>  }
>  EXPORT_SYMBOL(__check_object_size);
>  
> -static bool enable_checks __initdata = true;
> +static bool enable_checks __initdata =
> +		IS_ENABLED(CONFIG_HARDENED_USERCOPY_DEFAULT_ON);
>  
>  static int __init parse_hardened_usercopy(char *str)
>  {
> diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
> index 9088d613d519..adcc260839c7 100644
> --- a/security/Kconfig.hardening
> +++ b/security/Kconfig.hardening
> @@ -293,6 +293,14 @@ config HARDENED_USERCOPY
>  	  or are part of the kernel text. This prevents entire classes
>  	  of heap overflow exploits and similar kernel memory exposures.
>  
> +config HARDENED_USERCOPY_DEFAULT_ON
> +	bool "Harden memory copies by default"
> +	depends on HARDENED_USERCOPY
> +	default n

This must be "default HARDENED_USERCOPY" or existing distro builds will
break. All major distros enable this by default, and I don't want to
risk HARDENED_USERCOPY_DEFAULT_ON getting missed and getting globally
disabled.

> +	help
> +	  This has the effect of setting "hardened_usercopy=on" on the kernel
> +	  command line. This can be disabled with "hardened_usercopy=off".
> +
>  endmenu
>  
>  menu "Hardening of kernel data structures"

-Kees

-- 
Kees Cook

Re: [PATCH 3/4] mm: security: Check early if HARDENED_USERCOPY is enabled

[Kees Cook]

On Wed, Jan 22, 2025 at 05:19:24PM +0000, Mel Gorman wrote:
> HARDENED_USERCOPY is checked within a function so even if disabled, the
> function overhead still exists. Move the static check inline.
> 
> This is at best a micro-optimisation and any difference in performance
> was within noise but it is relatively consistent with the init_on_*
> implementations.
> 
> Suggested-by: Kees Cook <kees@kernel.org>
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> ---
>  include/linux/thread_info.h |  8 ++++++++
>  mm/usercopy.c               | 11 ++++++-----
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
> index cf2446c9c30d..832f6a97e64c 100644
> --- a/include/linux/thread_info.h
> +++ b/include/linux/thread_info.h
> @@ -221,9 +221,17 @@ static inline int arch_within_stack_frames(const void * const stack,
>  extern void __check_object_size(const void *ptr, unsigned long n,
>  					bool to_user);
>  
> +DECLARE_STATIC_KEY_MAYBE(CONFIG_HARDENED_USERCOPY_DEFAULT_ON,
> +			   validate_usercopy_range);

This should be DECLARE_STATIC_KEY_MAYBE_RO()

>  static __always_inline void check_object_size(const void *ptr, unsigned long n,
>  					      bool to_user)
>  {
> +	if (static_branch_maybe(CONFIG_HARDENED_USERCOPY_DEFAULT_ON,
> +				&validate_usercopy_range)) {
> +		return;
> +	}
> +
>  	if (!__builtin_constant_p(n))
>  		__check_object_size(ptr, n, to_user);
>  }

This is accidentally correct ("if validate, skip" matches "if not
enabled, disable validation" below, but is very confusing. Also, yes,
this is good to be moved into the inline, but let's wrap it in the
compile-time __builtin_constant_p() check:

static __always_inline void check_object_size(const void *ptr, unsigned long n,
					      bool to_user)
{
	if (!__builtin_constant_p(n) &&
	    static_branch_maybe(CONFIG_HARDENED_USERCOPY_DEFAULT_ON,
				&validate_usercopy_range))
		__check_object_size(ptr, n, to_user);
}

> diff --git a/mm/usercopy.c b/mm/usercopy.c
> index 4cf33305347a..2e86413ed244 100644
> --- a/mm/usercopy.c
> +++ b/mm/usercopy.c
> @@ -201,7 +201,9 @@ static inline void check_heap_object(const void *ptr, unsigned long n,
>  	}
>  }
>  
> -static DEFINE_STATIC_KEY_FALSE_RO(bypass_usercopy_checks);
> +DEFINE_STATIC_KEY_MAYBE_RO(CONFIG_HARDENED_USERCOPY_DEFAULT_ON,
> +			   validate_usercopy_range);
> +EXPORT_SYMBOL(validate_usercopy_range);
>  
>  /*
>   * Validates that the given object is:
> @@ -212,9 +214,6 @@ static DEFINE_STATIC_KEY_FALSE_RO(bypass_usercopy_checks);
>   */
>  void __check_object_size(const void *ptr, unsigned long n, bool to_user)
>  {
> -	if (static_branch_unlikely(&bypass_usercopy_checks))
> -		return;
> -
>  	/* Skip all tests if size is zero. */
>  	if (!n)
>  		return;
> @@ -271,7 +270,9 @@ __setup("hardened_usercopy=", parse_hardened_usercopy);
>  static int __init set_hardened_usercopy(void)
>  {
>  	if (enable_checks == false)
> -		static_branch_enable(&bypass_usercopy_checks);
> +		static_branch_enable(&validate_usercopy_range);
> +	else
> +		static_branch_disable(&validate_usercopy_range);

This should be:

	if (enable_checks)
		static_branch_enable(&validate_usercopy_range);
	else
		static_branch_disable(&validate_usercopy_range);

>  	return 1;
>  }
>  
> -- 
> 2.43.0
> 

-- 
Kees Cook

Re: [PATCH v2 0/4] Allow default HARDENED_USERCOPY to be set at compile time

[Kees Cook]

On Wed, Jan 22, 2025 at 05:19:21PM +0000, Mel Gorman wrote:
> on the exact CPU. While the benchmarks are somewhat synthetic, the overhead
> IO-intensive and network-intensive is easily detectable but the root cause
> may not be obvious (e.g. 2-14% overhead for netperf TCP_STREAM running
> over localhost with different ranges depending on the CPU).

I would be curious to see where this overhead is coming from. That seems
extraordinarily high, and makes me think there is something more we
should be fixing. :)

-- 
Kees Cook

Re: [PATCH 2/4] mm: security: Allow default HARDENED_USERCOPY to be set at compile time

[Mel Gorman]

On Wed, Jan 22, 2025 at 04:57:37PM -0800, Kees Cook wrote:
> On Wed, Jan 22, 2025 at 05:19:23PM +0000, Mel Gorman wrote:
> > HARDENED_USERCOPY defaults to on if enabled at compile time. Allow
> > hardened_usercopy= default to be set at compile time similar to
> > init_on_alloc= and init_on_free=. The intent is that hardening
> > options that can be disabled at runtime can set their default at
> > build time.
> > 
> > Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> > ---
> >  Documentation/admin-guide/kernel-parameters.txt | 4 +++-
> >  mm/usercopy.c                                   | 3 ++-
> >  security/Kconfig.hardening                      | 8 ++++++++
> >  3 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 3872bc6ec49d..5d759b20540a 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -1773,7 +1773,9 @@
> >  			allocation boundaries as a proactive defense
> >  			against bounds-checking flaws in the kernel's
> >  			copy_to_user()/copy_from_user() interface.
> > -		on	Perform hardened usercopy checks (default).
> > +			The default is determined by
> > +			CONFIG_HARDENED_USERCOPY_DEFAULT_ON.
> > +		on	Perform hardened usercopy checks.
> >  		off	Disable hardened usercopy checks.
> >  
> >  	hardlockup_all_cpu_backtrace=
> > diff --git a/mm/usercopy.c b/mm/usercopy.c
> > index 83c164aba6e0..4cf33305347a 100644
> > --- a/mm/usercopy.c
> > +++ b/mm/usercopy.c
> > @@ -255,7 +255,8 @@ void __check_object_size(const void *ptr, unsigned long n, bool to_user)
> >  }
> >  EXPORT_SYMBOL(__check_object_size);
> >  
> > -static bool enable_checks __initdata = true;
> > +static bool enable_checks __initdata =
> > +		IS_ENABLED(CONFIG_HARDENED_USERCOPY_DEFAULT_ON);
> >  
> >  static int __init parse_hardened_usercopy(char *str)
> >  {
> > diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
> > index 9088d613d519..adcc260839c7 100644
> > --- a/security/Kconfig.hardening
> > +++ b/security/Kconfig.hardening
> > @@ -293,6 +293,14 @@ config HARDENED_USERCOPY
> >  	  or are part of the kernel text. This prevents entire classes
> >  	  of heap overflow exploits and similar kernel memory exposures.
> >  
> > +config HARDENED_USERCOPY_DEFAULT_ON
> > +	bool "Harden memory copies by default"
> > +	depends on HARDENED_USERCOPY
> > +	default n
> 
> This must be "default HARDENED_USERCOPY" or existing distro builds will
> break. All major distros enable this by default, and I don't want to
> risk HARDENED_USERCOPY_DEFAULT_ON getting missed and getting globally
> disabled.
> 

Ok. I dislike that HARDENED_USERCOPY will be inconsistent with INIT_ON*
but it's not a hill I'm willing to die on. Will be in v3

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 3/4] mm: security: Check early if HARDENED_USERCOPY is enabled

[Mel Gorman]

On Wed, Jan 22, 2025 at 05:01:21PM -0800, Kees Cook wrote:
> On Wed, Jan 22, 2025 at 05:19:24PM +0000, Mel Gorman wrote:
> > HARDENED_USERCOPY is checked within a function so even if disabled, the
> > function overhead still exists. Move the static check inline.
> > 
> > This is at best a micro-optimisation and any difference in performance
> > was within noise but it is relatively consistent with the init_on_*
> > implementations.
> > 
> > Suggested-by: Kees Cook <kees@kernel.org>
> > Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> > ---
> >  include/linux/thread_info.h |  8 ++++++++
> >  mm/usercopy.c               | 11 ++++++-----
> >  2 files changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
> > index cf2446c9c30d..832f6a97e64c 100644
> > --- a/include/linux/thread_info.h
> > +++ b/include/linux/thread_info.h
> > @@ -221,9 +221,17 @@ static inline int arch_within_stack_frames(const void * const stack,
> >  extern void __check_object_size(const void *ptr, unsigned long n,
> >  					bool to_user);
> >  
> > +DECLARE_STATIC_KEY_MAYBE(CONFIG_HARDENED_USERCOPY_DEFAULT_ON,
> > +			   validate_usercopy_range);
> 
> This should be DECLARE_STATIC_KEY_MAYBE_RO()
> 

Doesn't exist. Are you mixing it up with the DEFINE_STATIC_KEY_MAYBE_RO?


> >  static __always_inline void check_object_size(const void *ptr, unsigned long n,
> >  					      bool to_user)
> >  {
> > +	if (static_branch_maybe(CONFIG_HARDENED_USERCOPY_DEFAULT_ON,
> > +				&validate_usercopy_range)) {
> > +		return;
> > +	}
> > +
> >  	if (!__builtin_constant_p(n))
> >  		__check_object_size(ptr, n, to_user);
> >  }
> 
> This is accidentally correct ("if validate, skip" matches "if not
> enabled, disable validation" below, but is very confusing. Also, yes,
> this is good to be moved into the inline, but let's wrap it in the
> compile-time __builtin_constant_p() check:
> 
> static __always_inline void check_object_size(const void *ptr, unsigned long n,
> 					      bool to_user)
> {
> 	if (!__builtin_constant_p(n) &&
> 	    static_branch_maybe(CONFIG_HARDENED_USERCOPY_DEFAULT_ON,
> 				&validate_usercopy_range))
> 		__check_object_size(ptr, n, to_user);
> }
> 

Ok, should be fine given that it's a compile-time check anyway.

> > diff --git a/mm/usercopy.c b/mm/usercopy.c
> > index 4cf33305347a..2e86413ed244 100644
> > --- a/mm/usercopy.c
> > +++ b/mm/usercopy.c
> > @@ -201,7 +201,9 @@ static inline void check_heap_object(const void *ptr, unsigned long n,
> >  	}
> >  }
> >  
> > -static DEFINE_STATIC_KEY_FALSE_RO(bypass_usercopy_checks);
> > +DEFINE_STATIC_KEY_MAYBE_RO(CONFIG_HARDENED_USERCOPY_DEFAULT_ON,
> > +			   validate_usercopy_range);
> > +EXPORT_SYMBOL(validate_usercopy_range);
> >  
> >  /*
> >   * Validates that the given object is:
> > @@ -212,9 +214,6 @@ static DEFINE_STATIC_KEY_FALSE_RO(bypass_usercopy_checks);
> >   */
> >  void __check_object_size(const void *ptr, unsigned long n, bool to_user)
> >  {
> > -	if (static_branch_unlikely(&bypass_usercopy_checks))
> > -		return;
> > -
> >  	/* Skip all tests if size is zero. */
> >  	if (!n)
> >  		return;
> > @@ -271,7 +270,9 @@ __setup("hardened_usercopy=", parse_hardened_usercopy);
> >  static int __init set_hardened_usercopy(void)
> >  {
> >  	if (enable_checks == false)
> > -		static_branch_enable(&bypass_usercopy_checks);
> > +		static_branch_enable(&validate_usercopy_range);
> > +	else
> > +		static_branch_disable(&validate_usercopy_range);
> 
> This should be:
> 
> 	if (enable_checks)
> 		static_branch_enable(&validate_usercopy_range);
> 	else
> 		static_branch_disable(&validate_usercopy_range);
> 

Bah, fixed.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH v2 0/4] Allow default HARDENED_USERCOPY to be set at compile time

[Mel Gorman]

On Wed, Jan 22, 2025 at 05:02:29PM -0800, Kees Cook wrote:
> On Wed, Jan 22, 2025 at 05:19:21PM +0000, Mel Gorman wrote:
> > on the exact CPU. While the benchmarks are somewhat synthetic, the overhead
> > IO-intensive and network-intensive is easily detectable but the root cause
> > may not be obvious (e.g. 2-14% overhead for netperf TCP_STREAM running
> > over localhost with different ranges depending on the CPU).
> 
> I would be curious to see where this overhead is coming from. That seems
> extraordinarily high, and makes me think there is something more we
> should be fixing. :)
> 

Almost certainly yes, it could be anything really but the results are
consistent. It'll be somewhat tricky to narrow down given that it's
somewhat specific to the CPU. It was outside the scope of the series to
investigate. The primary aim was to provide the option to have hardened
usercopy available, but not surprising from a performance perspective,
via a single kernel binary.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 2/4] mm: security: Allow default HARDENED_USERCOPY to be set at compile time

[David Laight]

...  
> > +config HARDENED_USERCOPY_DEFAULT_ON
> > +	bool "Harden memory copies by default"
> > +	depends on HARDENED_USERCOPY
> > +	default n  
> 
> This must be "default HARDENED_USERCOPY" or existing distro builds will
> break. All major distros enable this by default, and I don't want to
> risk HARDENED_USERCOPY_DEFAULT_ON getting missed and getting globally
> disabled.

It'll also cause grief for anyone trying to bisect.
Although that is always going to go wrong if it has been disabled.

I had 'fun' trying to locate a massive slowdown of a single threaded
program that was caused by a side effect of one of the speculative
execution mitigations getting enabled because the config parameter
got renamed.

	David

[PATCH 1/4] mm: security: Move hardened usercopy under 'Kernel hardening options'

[Mel Gorman]

There is a submenu for 'Kernel hardening options' under "Security".
Move HARDENED_USERCOPY under the hardening options as it is clearly
related.

Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
Acked-by: Paul Moore <paul@paul-moore.com>
---
 security/Kconfig           | 12 ------------
 security/Kconfig.hardening | 16 ++++++++++++++++
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/security/Kconfig b/security/Kconfig
index 28e685f53bd1..fe7346dc4bc3 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -159,18 +159,6 @@ config LSM_MMAP_MIN_ADDR
 	  this low address space will need the permission specific to the
 	  systems running LSM.
 
-config HARDENED_USERCOPY
-	bool "Harden memory copies between kernel and userspace"
-	imply STRICT_DEVMEM
-	help
-	  This option checks for obviously wrong memory regions when
-	  copying memory to/from the kernel (via copy_to_user() and
-	  copy_from_user() functions) by rejecting memory ranges that
-	  are larger than the specified heap object, span multiple
-	  separately allocated pages, are not on the process stack,
-	  or are part of the kernel text. This prevents entire classes
-	  of heap overflow exploits and similar kernel memory exposures.
-
 config FORTIFY_SOURCE
 	bool "Harden common str/mem functions against buffer overflows"
 	depends on ARCH_HAS_FORTIFY_SOURCE
diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
index c9d5ca3d8d08..9088d613d519 100644
--- a/security/Kconfig.hardening
+++ b/security/Kconfig.hardening
@@ -279,6 +279,22 @@ config ZERO_CALL_USED_REGS
 
 endmenu
 
+menu "Bounds checking"
+
+config HARDENED_USERCOPY
+	bool "Harden memory copies between kernel and userspace"
+	imply STRICT_DEVMEM
+	help
+	  This option checks for obviously wrong memory regions when
+	  copying memory to/from the kernel (via copy_to_user() and
+	  copy_from_user() functions) by rejecting memory ranges that
+	  are larger than the specified heap object, span multiple
+	  separately allocated pages, are not on the process stack,
+	  or are part of the kernel text. This prevents entire classes
+	  of heap overflow exploits and similar kernel memory exposures.
+
+endmenu
+
 menu "Hardening of kernel data structures"
 
 config LIST_HARDENED
-- 
2.43.0