diff mbox series

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

Message ID 20250117130337.4716-3-mgorman@techsingularity.net (mailing list archive)
State Superseded
Headers show
Series Allow default HARDENED_USERCOPY to be set at compile time | expand

Commit Message

Mel Gorman Jan. 17, 2025, 1:03 p.m. UTC 
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(-)

Comments

Kees Cook Jan. 20, 2025, 9:21 p.m. UTC | #1 
On Fri, Jan 17, 2025 at 01:03:36PM +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);

With the addition of the compile-time default, we can also provide
better hot-path hinting for the static branches (likely as a separate
patch), that would rename "bypass_usercopy_checks" to
"perform_usercopy_checks" to avoid confusing negatives, and then:

DEFINE_STATIC_KEY_MAYBE_RO(CONFIG_HARDENED_USERCOPY_DEFAULT_ON,
			   perform_usercopy_checks);

and then adjust set_hardened_usercopy:

static int __init set_hardened_usercopy(void)
{
        if (enable_checks)
                static_branch_enable(&perform_usercopy_checks);
	else
                static_branch_disable(&perform_usercopy_checks);
        return 1;
}

and finally adjust __check_object_size:

        if (!static_branch_maybe(CONFIG_HARDENED_USERCOPY_DEFAULT_ON,
				 &perform_usercopy_checks))
                return;

But if the perf difference isn't measurable (it's probably lost to the
cost of doing the checks), this change isn't needed at all, but would
fully duplicate the logic used for init_on_alloc etc.

>  
>  static int __init parse_hardened_usercopy(char *str)
>  {
> diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
> index 00e6e2ed0c43..537a6431892e 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

To avoid regressions for people moving their configs forward (and IMO
get the right setting by default), I think this should instead be
"default HARDENED_USERCOPY".

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

Otherwise looks good!
Mel Gorman Jan. 21, 2025, 12:35 p.m. UTC | #2 
On Mon, Jan 20, 2025 at 01:21:54PM -0800, Kees Cook wrote:
> On Fri, Jan 17, 2025 at 01:03:36PM +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);
> 
> With the addition of the compile-time default, we can also provide
> better hot-path hinting for the static branches (likely as a separate
> patch), that would rename "bypass_usercopy_checks" to
> "perform_usercopy_checks" to avoid confusing negatives, and then:
> 

I decided to go with the more explicit name "validate_usercopy_range"
but I'm not overly pushed about the name

> > --- 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
>
> To avoid regressions for people moving their configs forward (and IMO
> get the right setting by default), I think this should instead be
> "default HARDENED_USERCOPY".

This I'm less keen on. The intent is that all the hardening options would
default to all opt-in or all opt-out by default to be consistent. I lean
towards default opt-in because those requiring a hardened environment should
know what options to enable and why. A distribution providing a hardened
kernel would explicitly enable them by default. A distribution that wanted
to provided a general kernel for users could provide the hardening options
but leave them disabled to avoid spurious performance regression reports
after a kernel upgrade.

Micro-optimisation currently is this;

--<--
mm: security: Check early if HARDENED_USERCOPY is enabled

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

Suggested-by: Kees Cook <kees@kernel.org>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>

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;
 }
diff mbox series

Patch

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 00e6e2ed0c43..537a6431892e 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"