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 <[email protected]> > > --- > > 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 <[email protected]> Signed-off-by: Mel Gorman <[email protected]> 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; }
