Message-ID: <s2uigraqtmh3rs72hbsxcv2mnl3fa7dlqtq6erl2glkeizzdn7@wi6fop5p4usx>
Date: Tue, 21 Jan 2025 12:35:53 +0000
From: Mel Gorman <mgorman@...hsingularity.net>
To: Kees Cook <kees@...nel.org>
Cc: Daniel Micay <danielmicay@...il.com>, linux-hardening@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] mm: security: Allow default HARDENED_USERCOPY to be
 set at compile time

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@...hsingularity.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@...nel.org>
Signed-off-by: Mel Gorman <mgorman@...hsingularity.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;
 }
 

Powered by blists - more mailing lists