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@xxxxxxxxxx>
> > Signed-off-by: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
> > ---
> >  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