| Message ID | 20250122171925.25472-4-mgorman@techsingularity.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Allow default HARDENED_USERCOPY to be set at compile time | expand |
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@kernel.org> > Signed-off-by: Mel Gorman <mgorman@techsingularity.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 >
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@kernel.org> > > Signed-off-by: Mel Gorman <mgorman@techsingularity.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() > 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.
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; }
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@kernel.org> Signed-off-by: Mel Gorman <mgorman@techsingularity.net> --- include/linux/thread_info.h | 8 ++++++++ mm/usercopy.c | 11 ++++++----- 2 files changed, 14 insertions(+), 5 deletions(-)