linux-hardening - Re: [PATCH 3/4] mm: security: Check early if HARDENED

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <202501221657.E657471E6B@keescook>
Date: Wed, 22 Jan 2025 17:01:21 -0800
From: Kees Cook <kees@...nel.org>
To: Mel Gorman <mgorman@...hsingularity.net>
Cc: Daniel Micay <danielmicay@...il.com>, Paul Moore <paul@...l-moore.com>,
	linux-hardening@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] mm: security: Check early if HARDENED_USERCOPY is
 enabled

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