add param that allows bootline control of hardened usercopy
diff mbox series

Message ID 1529939300-27461-1-git-send-email-crecklin@redhat.com
State New, archived
Headers show
Series
  • add param that allows bootline control of hardened usercopy
Related show

Commit Message

Chris von Recklinghausen June 25, 2018, 3:08 p.m. UTC
Enabling HARDENED_USER_COPY causes measurable regressions in the
networking performances, up to 8% under UDP flood.

A generic distro may want to enable HARDENED_USER_COPY in their default
kernel config, but at the same time, such distro may want to be able to
avoid the performance penalties in with the default configuration and
enable the stricter check on a per-boot basis.

This change adds a config variable and a boot parameter to conditionally
enable HARDENED_USER_COPY at boot time, and switch HUC to off if
HUC_DEFAULT_OFF is set.

Signed-off-by: Chris von Recklinghausen <crecklin@redhat.com>
---
 .../admin-guide/kernel-parameters.rst         |  2 ++
 .../admin-guide/kernel-parameters.txt         |  3 ++
 include/linux/thread_info.h                   |  7 +++++
 mm/usercopy.c                                 | 28 +++++++++++++++++++
 security/Kconfig                              | 10 +++++++
 5 files changed, 50 insertions(+)

Comments

Chris von Recklinghausen June 25, 2018, 3:22 p.m. UTC | #1
Add correct address for linux-mm

On 06/25/2018 11:08 AM, Chris von Recklinghausen wrote:
> Enabling HARDENED_USER_COPY causes measurable regressions in the
> networking performances, up to 8% under UDP flood.
>
> A generic distro may want to enable HARDENED_USER_COPY in their default
> kernel config, but at the same time, such distro may want to be able to
> avoid the performance penalties in with the default configuration and
> enable the stricter check on a per-boot basis.
>
> This change adds a config variable and a boot parameter to conditionally
> enable HARDENED_USER_COPY at boot time, and switch HUC to off if
> HUC_DEFAULT_OFF is set.
>
> Signed-off-by: Chris von Recklinghausen <crecklin@redhat.com>
> ---
>  .../admin-guide/kernel-parameters.rst         |  2 ++
>  .../admin-guide/kernel-parameters.txt         |  3 ++
>  include/linux/thread_info.h                   |  7 +++++
>  mm/usercopy.c                                 | 28 +++++++++++++++++++
>  security/Kconfig                              | 10 +++++++
>  5 files changed, 50 insertions(+)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
> index b8d0bc07ed0a..c3035038e3ae 100644
> --- a/Documentation/admin-guide/kernel-parameters.rst
> +++ b/Documentation/admin-guide/kernel-parameters.rst
> @@ -100,6 +100,8 @@ parameter is applicable::
>  	FB	The frame buffer device is enabled.
>  	FTRACE	Function tracing enabled.
>  	GCOV	GCOV profiling is enabled.
> +	HUC	Hardened usercopy is enabled
> +	HUCF	Hardened usercopy disabled at boot
>  	HW	Appropriate hardware is enabled.
>  	IA-64	IA-64 architecture is enabled.
>  	IMA     Integrity measurement architecture is enabled.
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index efc7aa7a0670..cd3354bc14d3 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -816,6 +816,9 @@
>  	disable=	[IPV6]
>  			See Documentation/networking/ipv6.txt.
>  
> +	enable_hardened_usercopy [HUC,HUCF]
> +			Enable hardened usercopy checks
> +
>  	disable_radix	[PPC]
>  			Disable RADIX MMU mode on POWER9
>  
> diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
> index 8d8821b3689a..140a36cc1c2c 100644
> --- a/include/linux/thread_info.h
> +++ b/include/linux/thread_info.h
> @@ -109,12 +109,19 @@ static inline int arch_within_stack_frames(const void * const stack,
>  #endif
>  
>  #ifdef CONFIG_HARDENED_USERCOPY
> +#include <linux/jump_label.h>
> +
> +DECLARE_STATIC_KEY_FALSE(bypass_usercopy_checks);
> +
>  extern void __check_object_size(const void *ptr, unsigned long n,
>  					bool to_user);
>  
>  static __always_inline void check_object_size(const void *ptr, unsigned long n,
>  					      bool to_user)
>  {
> +	if (static_branch_likely(&bypass_usercopy_checks))
> +		return;
> +
>  	if (!__builtin_constant_p(n))
>  		__check_object_size(ptr, n, to_user);
>  }
> diff --git a/mm/usercopy.c b/mm/usercopy.c
> index e9e9325f7638..ce3996da1b2e 100644
> --- a/mm/usercopy.c
> +++ b/mm/usercopy.c
> @@ -279,3 +279,31 @@ void __check_object_size(const void *ptr, unsigned long n, bool to_user)
>  	check_kernel_text_object((const unsigned long)ptr, n, to_user);
>  }
>  EXPORT_SYMBOL(__check_object_size);
> +
> +DEFINE_STATIC_KEY_FALSE(bypass_usercopy_checks);
> +EXPORT_SYMBOL(bypass_usercopy_checks);
> +
> +#ifdef CONFIG_HUC_DEFAULT_OFF
> +#define HUC_DEFAULT false
> +#else
> +#define HUC_DEFAULT true
> +#endif
> +
> +static bool enable_huc_atboot = HUC_DEFAULT;
> +
> +static int __init parse_enable_usercopy(char *str)
> +{
> +	enable_huc_atboot = true;
> +	return 1;
> +}
> +
> +static int __init set_enable_usercopy(void)
> +{
> +	if (enable_huc_atboot == false)
> +		static_branch_enable(&bypass_usercopy_checks);
> +	return 1;
> +}
> +
> +__setup("enable_hardened_usercopy", parse_enable_usercopy);
> +
> +late_initcall(set_enable_usercopy);
> diff --git a/security/Kconfig b/security/Kconfig
> index c4302067a3ad..a6173897b85c 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -189,6 +189,16 @@ config HARDENED_USERCOPY_PAGESPAN
>  	  been removed. This config is intended to be used only while
>  	  trying to find such users.
>  
> +config HUC_DEFAULT_OFF
> +	bool "allow CONFIG_HARDENED_USERCOPY to be configured but disabled"
> +	depends on HARDENED_USERCOPY
> +	help
> +	  When CONFIG_HARDENED_USERCOPY is enabled, disable its
> +	  functionality unless it is enabled via at boot time
> +	  via the "enable_hardened_usercopy" boot parameter. This allows
> +	  the functionality of hardened usercopy to be present but not
> +	  impact performance unless it is needed.
> +
>  config FORTIFY_SOURCE
>  	bool "Harden common str/mem functions against buffer overflows"
>  	depends on ARCH_HAS_FORTIFY_SOURCE
Randy Dunlap June 25, 2018, 4:09 p.m. UTC | #2
On 06/25/2018 08:08 AM, Chris von Recklinghausen wrote:
> Enabling HARDENED_USER_COPY causes measurable regressions in the
> networking performances, up to 8% under UDP flood.
> 
> A generic distro may want to enable HARDENED_USER_COPY in their default
> kernel config, but at the same time, such distro may want to be able to
> avoid the performance penalties in with the default configuration and
> enable the stricter check on a per-boot basis.
> 
> This change adds a config variable and a boot parameter to conditionally
> enable HARDENED_USER_COPY at boot time, and switch HUC to off if
> HUC_DEFAULT_OFF is set.
> 
> Signed-off-by: Chris von Recklinghausen <crecklin@redhat.com>
> ---
>  .../admin-guide/kernel-parameters.rst         |  2 ++
>  .../admin-guide/kernel-parameters.txt         |  3 ++
>  include/linux/thread_info.h                   |  7 +++++
>  mm/usercopy.c                                 | 28 +++++++++++++++++++
>  security/Kconfig                              | 10 +++++++
>  5 files changed, 50 insertions(+)
> 

Hi,

> diff --git a/security/Kconfig b/security/Kconfig
> index c4302067a3ad..a6173897b85c 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -189,6 +189,16 @@ config HARDENED_USERCOPY_PAGESPAN
>  	  been removed. This config is intended to be used only while
>  	  trying to find such users.
>  
> +config HUC_DEFAULT_OFF
> +	bool "allow CONFIG_HARDENED_USERCOPY to be configured but disabled"
> +	depends on HARDENED_USERCOPY
> +	help
> +	  When CONFIG_HARDENED_USERCOPY is enabled, disable its
> +	  functionality unless it is enabled via at boot time

	                       it is enabled at boot time

> +	  via the "enable_hardened_usercopy" boot parameter. This allows
> +	  the functionality of hardened usercopy to be present but not
> +	  impact performance unless it is needed.
> +
>  config FORTIFY_SOURCE
>  	bool "Harden common str/mem functions against buffer overflows"
>  	depends on ARCH_HAS_FORTIFY_SOURCE
>
kernel test robot June 25, 2018, 6:05 p.m. UTC | #3
Hi Chris,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.18-rc2 next-20180625]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Chris-von-Recklinghausen/add-param-that-allows-bootline-control-of-hardened-usercopy/20180625-232835
config: m68k-allyesconfig (attached as .config)
compiler: m68k-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=m68k 

All error/warnings (new ones prefixed by >>):

   WARNING: unmet direct dependencies detected for PREEMPT_COUNT
   Depends on COLDFIRE
   Selected by
   - DEBUG_ATOMIC_SLEEP && DEBUG_KERNEL
   In file included from include/linux/thread_info.h:112:0,
   from include/asm-generic/preempt.h:5,
   from ./arch/m68k/include/generated/asm/preempt.h:1,
   from include/linux/preempt.h:81,
   from arch/m68k/include/asm/irqflags.h:6,
   from include/linux/irqflags.h:16,
   from arch/m68k/include/asm/atomic.h:6,
   from include/linux/atomic.h:5,
   from include/linux/rcupdate.h:38,
   from include/linux/rculist.h:11,
   from include/linux/pid.h:5,
   from include/linux/sched.h:14,
   from arch/m68k/kernel/asm-offsets.c:15:
   include/linux/jump_label.h: In function 'static_key_count':
>> include/linux/jump_label.h:194:9: error: implicit declaration of function 'atomic_read'; did you mean
   return atomic_read(&key->enabled);
   ^~~~~~~~~~~
   __atomic_load
   include/linux/jump_label.h: In function 'static_key_slow_inc':
>> include/linux/jump_label.h:221:2: error: implicit declaration of function 'atomic_inc'; did you mean
   atomic_inc(&key->enabled);
   ^~~~~~~~~~
   __atomic_load
   include/linux/jump_label.h: In function 'static_key_slow_dec':
>> include/linux/jump_label.h:227:2: error: implicit declaration of function 'atomic_dec'; did you mean
   atomic_dec(&key->enabled);
   ^~~~~~~~~~
   __atomic_clear
   include/linux/jump_label.h: In function 'static_key_enable':
>> include/linux/jump_label.h:254:2: error: implicit declaration of function 'atomic_set'; did you mean
   atomic_set(&key->enabled, 1);
   ^~~~~~~~~~
   __atomic_clear
   In file included from include/linux/atomic.h:5:0,
   from include/linux/rcupdate.h:38,
   from include/linux/rculist.h:11,
   from include/linux/pid.h:5,
   from include/linux/sched.h:14,
   from arch/m68k/kernel/asm-offsets.c:15:
   arch/m68k/include/asm/atomic.h: At top level:
>> arch/m68k/include/asm/atomic.h:125:20: warning: conflicting types for 'atomic_inc'
   static inline void atomic_inc(atomic_t
   ^~~~~~~~~~
>> arch/m68k/include/asm/atomic.h:125:20: error: static declaration of 'atomic_inc' follows non-static declaration
   In file included from include/linux/thread_info.h:112:0,
   from include/asm-generic/preempt.h:5,
   from ./arch/m68k/include/generated/asm/preempt.h:1,
   from include/linux/preempt.h:81,
   from arch/m68k/include/asm/irqflags.h:6,
   from include/linux/irqflags.h:16,
   from arch/m68k/include/asm/atomic.h:6,
   from include/linux/atomic.h:5,
   from include/linux/rcupdate.h:38,
   from include/linux/rculist.h:11,
   from include/linux/pid.h:5,
   from include/linux/sched.h:14,
   from arch/m68k/kernel/asm-offsets.c:15:
   include/linux/jump_label.h:221:2: note: previous implicit declaration of 'atomic_inc' was here
   atomic_inc(&key->enabled);
   ^~~~~~~~~~
   In file included from include/linux/atomic.h:5:0,
   from include/linux/rcupdate.h:38,
   from include/linux/rculist.h:11,
   from include/linux/pid.h:5,
   from include/linux/sched.h:14,
   from arch/m68k/kernel/asm-offsets.c:15:
>> arch/m68k/include/asm/atomic.h:130:20: warning: conflicting types for 'atomic_dec'
   static inline void atomic_dec(atomic_t
   ^~~~~~~~~~
>> arch/m68k/include/asm/atomic.h:130:20: error: static declaration of 'atomic_dec' follows non-static declaration
   In file included from include/linux/thread_info.h:112:0,
   from include/asm-generic/preempt.h:5,
   from ./arch/m68k/include/generated/asm/preempt.h:1,
   from include/linux/preempt.h:81,
   from arch/m68k/include/asm/irqflags.h:6,
   from include/linux/irqflags.h:16,
   from arch/m68k/include/asm/atomic.h:6,
   from include/linux/atomic.h:5,
   from include/linux/rcupdate.h:38,
   from include/linux/rculist.h:11,
   from include/linux/pid.h:5,
   from include/linux/sched.h:14,
   from arch/m68k/kernel/asm-offsets.c:15:
   include/linux/jump_label.h:227:2: note: previous implicit declaration of 'atomic_dec' was here
   atomic_dec(&key->enabled);
   ^~~~~~~~~~
   cc1: some warnings being treated as errors
   Makefile Module.symvers System.map arch block built-in.a certs crypto drivers firmware fs include init ipc kernel lib mm modules.builtin modules.order net scripts security sound source usr virt vmlinux vmlinux.gz vmlinux.o Error 1
   Target '__build' not remade because of errors.
   Makefile Module.symvers System.map arch block built-in.a certs crypto drivers firmware fs include init ipc kernel lib mm modules.builtin modules.order net scripts security sound source usr virt vmlinux vmlinux.gz vmlinux.o Error 2
   Target 'prepare' not remade because of errors.
   make: Makefile Module.symvers System.map arch block built-in.a certs crypto drivers firmware fs include init ipc kernel lib mm modules.builtin modules.order net scripts security sound source usr virt vmlinux vmlinux.gz vmlinux.o Error 2

vim +/atomic_read +194 include/linux/jump_label.h

1f69bf9c6 Jason Baron          2016-08-03  191  
4c5ea0a9c Paolo Bonzini        2016-06-21  192  static inline int static_key_count(struct static_key *key)
4c5ea0a9c Paolo Bonzini        2016-06-21  193  {
4c5ea0a9c Paolo Bonzini        2016-06-21 @194  	return atomic_read(&key->enabled);
4c5ea0a9c Paolo Bonzini        2016-06-21  195  }
4c5ea0a9c Paolo Bonzini        2016-06-21  196  
97ce2c88f Jeremy Fitzhardinge  2011-10-12  197  static __always_inline void jump_label_init(void)
97ce2c88f Jeremy Fitzhardinge  2011-10-12  198  {
c4b2c0c5f Hannes Frederic Sowa 2013-10-19  199  	static_key_initialized = true;
97ce2c88f Jeremy Fitzhardinge  2011-10-12  200  }
97ce2c88f Jeremy Fitzhardinge  2011-10-12  201  
578ae447e Josh Poimboeuf       2018-03-19  202  static inline void jump_label_invalidate_initmem(void) {}
333522447 Josh Poimboeuf       2018-02-20  203  
c5905afb0 Ingo Molnar          2012-02-24  204  static __always_inline bool static_key_false(struct static_key *key)
c5905afb0 Ingo Molnar          2012-02-24  205  {
ea5e9539a Mel Gorman           2014-06-04  206  	if (unlikely(static_key_count(key) > 0))
c5905afb0 Ingo Molnar          2012-02-24  207  		return true;
c5905afb0 Ingo Molnar          2012-02-24  208  	return false;
c5905afb0 Ingo Molnar          2012-02-24  209  }
c5905afb0 Ingo Molnar          2012-02-24  210  
c5905afb0 Ingo Molnar          2012-02-24  211  static __always_inline bool static_key_true(struct static_key *key)
d430d3d7e Jason Baron          2011-03-16  212  {
ea5e9539a Mel Gorman           2014-06-04  213  	if (likely(static_key_count(key) > 0))
d430d3d7e Jason Baron          2011-03-16  214  		return true;
d430d3d7e Jason Baron          2011-03-16  215  	return false;
d430d3d7e Jason Baron          2011-03-16  216  }
bf5438fca Jason Baron          2010-09-17  217  
c5905afb0 Ingo Molnar          2012-02-24  218  static inline void static_key_slow_inc(struct static_key *key)
d430d3d7e Jason Baron          2011-03-16  219  {
5cdda5117 Borislav Petkov      2017-10-18  220  	STATIC_KEY_CHECK_USE(key);
d430d3d7e Jason Baron          2011-03-16 @221  	atomic_inc(&key->enabled);
d430d3d7e Jason Baron          2011-03-16  222  }
bf5438fca Jason Baron          2010-09-17  223  
c5905afb0 Ingo Molnar          2012-02-24  224  static inline void static_key_slow_dec(struct static_key *key)
bf5438fca Jason Baron          2010-09-17  225  {
5cdda5117 Borislav Petkov      2017-10-18  226  	STATIC_KEY_CHECK_USE(key);
d430d3d7e Jason Baron          2011-03-16 @227  	atomic_dec(&key->enabled);
bf5438fca Jason Baron          2010-09-17  228  }
bf5438fca Jason Baron          2010-09-17  229  
ce48c1464 Peter Zijlstra       2018-01-22  230  #define static_key_slow_inc_cpuslocked(key) static_key_slow_inc(key)
ce48c1464 Peter Zijlstra       2018-01-22  231  #define static_key_slow_dec_cpuslocked(key) static_key_slow_dec(key)
ce48c1464 Peter Zijlstra       2018-01-22  232  
4c3ef6d79 Jason Baron          2010-09-17  233  static inline int jump_label_text_reserved(void *start, void *end)
4c3ef6d79 Jason Baron          2010-09-17  234  {
4c3ef6d79 Jason Baron          2010-09-17  235  	return 0;
4c3ef6d79 Jason Baron          2010-09-17  236  }
4c3ef6d79 Jason Baron          2010-09-17  237  
91bad2f8d Jason Baron          2010-10-01  238  static inline void jump_label_lock(void) {}
91bad2f8d Jason Baron          2010-10-01  239  static inline void jump_label_unlock(void) {}
91bad2f8d Jason Baron          2010-10-01  240  
d430d3d7e Jason Baron          2011-03-16  241  static inline int jump_label_apply_nops(struct module *mod)
d430d3d7e Jason Baron          2011-03-16  242  {
d430d3d7e Jason Baron          2011-03-16  243  	return 0;
d430d3d7e Jason Baron          2011-03-16  244  }
b20295207 Gleb Natapov         2011-11-27  245  
e33886b38 Peter Zijlstra       2015-07-24  246  static inline void static_key_enable(struct static_key *key)
e33886b38 Peter Zijlstra       2015-07-24  247  {
5cdda5117 Borislav Petkov      2017-10-18  248  	STATIC_KEY_CHECK_USE(key);
e33886b38 Peter Zijlstra       2015-07-24  249  
1dbb6704d Paolo Bonzini        2017-08-01  250  	if (atomic_read(&key->enabled) != 0) {
1dbb6704d Paolo Bonzini        2017-08-01  251  		WARN_ON_ONCE(atomic_read(&key->enabled) != 1);
1dbb6704d Paolo Bonzini        2017-08-01  252  		return;
1dbb6704d Paolo Bonzini        2017-08-01  253  	}
1dbb6704d Paolo Bonzini        2017-08-01 @254  	atomic_set(&key->enabled, 1);
e33886b38 Peter Zijlstra       2015-07-24  255  }
e33886b38 Peter Zijlstra       2015-07-24  256  

:::::: The code at line 194 was first introduced by commit
:::::: 4c5ea0a9cd02d6aa8adc86e100b2a4cff8d614ff locking/static_key: Fix concurrent static_key_slow_inc()

:::::: TO: Paolo Bonzini <pbonzini@redhat.com>
:::::: CC: Ingo Molnar <mingo@kernel.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Kees Cook June 25, 2018, 6:21 p.m. UTC | #4
On Mon, Jun 25, 2018 at 8:08 AM, Chris von Recklinghausen
<crecklin@redhat.com> wrote:
> Enabling HARDENED_USER_COPY causes measurable regressions in the
> networking performances, up to 8% under UDP flood.

Which function is "hot"? i.e. which copy*user() is taking up the time?
Do you have a workload that at can be used to reproduce the problem?

-Kees
kernel test robot June 25, 2018, 6:53 p.m. UTC | #5
Hi Chris,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.18-rc2 next-20180625]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Chris-von-Recklinghausen/add-param-that-allows-bootline-control-of-hardened-usercopy/20180625-232835
config: m68k-multi_defconfig (attached as .config)
compiler: m68k-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=m68k 

All errors (new ones prefixed by >>):

   In file included from include/linux/thread_info.h:112:0,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/m68k/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:81,
                    from arch/m68k/include/asm/irqflags.h:6,
                    from include/linux/irqflags.h:16,
                    from arch/m68k/include/asm/atomic.h:6,
                    from include/linux/atomic.h:5,
                    from include/linux/rcupdate.h:38,
                    from include/linux/rculist.h:11,
                    from include/linux/pid.h:5,
                    from include/linux/sched.h:14,
                    from arch/m68k/kernel/asm-offsets.c:15:
   include/linux/jump_label.h: In function 'static_key_count':
>> include/linux/jump_label.h:194:9: error: implicit declaration of function 'atomic_read'; did you mean '__atomic_load'? [-Werror=implicit-function-declaration]
     return atomic_read(&key->enabled);
            ^~~~~~~~~~~
            __atomic_load
   include/linux/jump_label.h: In function 'static_key_slow_inc':
>> include/linux/jump_label.h:221:2: error: implicit declaration of function 'atomic_inc'; did you mean '__atomic_load'? [-Werror=implicit-function-declaration]
     atomic_inc(&key->enabled);
     ^~~~~~~~~~
     __atomic_load
   include/linux/jump_label.h: In function 'static_key_slow_dec':
>> include/linux/jump_label.h:227:2: error: implicit declaration of function 'atomic_dec'; did you mean '__atomic_clear'? [-Werror=implicit-function-declaration]
     atomic_dec(&key->enabled);
     ^~~~~~~~~~
     __atomic_clear
   include/linux/jump_label.h: In function 'static_key_enable':
>> include/linux/jump_label.h:254:2: error: implicit declaration of function 'atomic_set'; did you mean '__atomic_clear'? [-Werror=implicit-function-declaration]
     atomic_set(&key->enabled, 1);
     ^~~~~~~~~~
     __atomic_clear
   In file included from include/linux/atomic.h:5:0,
                    from include/linux/rcupdate.h:38,
                    from include/linux/rculist.h:11,
                    from include/linux/pid.h:5,
                    from include/linux/sched.h:14,
                    from arch/m68k/kernel/asm-offsets.c:15:
   arch/m68k/include/asm/atomic.h: At top level:
   arch/m68k/include/asm/atomic.h:125:20: warning: conflicting types for 'atomic_inc'
    static inline void atomic_inc(atomic_t *v)
                       ^~~~~~~~~~
   arch/m68k/include/asm/atomic.h:125:20: error: static declaration of 'atomic_inc' follows non-static declaration
   In file included from include/linux/thread_info.h:112:0,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/m68k/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:81,
                    from arch/m68k/include/asm/irqflags.h:6,
                    from include/linux/irqflags.h:16,
                    from arch/m68k/include/asm/atomic.h:6,
                    from include/linux/atomic.h:5,
                    from include/linux/rcupdate.h:38,
                    from include/linux/rculist.h:11,
                    from include/linux/pid.h:5,
                    from include/linux/sched.h:14,
                    from arch/m68k/kernel/asm-offsets.c:15:
   include/linux/jump_label.h:221:2: note: previous implicit declaration of 'atomic_inc' was here
     atomic_inc(&key->enabled);
     ^~~~~~~~~~
   In file included from include/linux/atomic.h:5:0,
                    from include/linux/rcupdate.h:38,
                    from include/linux/rculist.h:11,
                    from include/linux/pid.h:5,
                    from include/linux/sched.h:14,
                    from arch/m68k/kernel/asm-offsets.c:15:
   arch/m68k/include/asm/atomic.h:130:20: warning: conflicting types for 'atomic_dec'
    static inline void atomic_dec(atomic_t *v)
                       ^~~~~~~~~~
   arch/m68k/include/asm/atomic.h:130:20: error: static declaration of 'atomic_dec' follows non-static declaration
   In file included from include/linux/thread_info.h:112:0,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/m68k/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:81,
                    from arch/m68k/include/asm/irqflags.h:6,
                    from include/linux/irqflags.h:16,
                    from arch/m68k/include/asm/atomic.h:6,
                    from include/linux/atomic.h:5,
                    from include/linux/rcupdate.h:38,
                    from include/linux/rculist.h:11,
                    from include/linux/pid.h:5,
                    from include/linux/sched.h:14,
                    from arch/m68k/kernel/asm-offsets.c:15:
   include/linux/jump_label.h:227:2: note: previous implicit declaration of 'atomic_dec' was here
     atomic_dec(&key->enabled);
     ^~~~~~~~~~
   cc1: some warnings being treated as errors
   make[2]: *** [arch/m68k/kernel/asm-offsets.s] Error 1
   make[2]: Target '__build' not remade because of errors.
   make[1]: *** [prepare0] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [sub-make] Error 2

vim +194 include/linux/jump_label.h

1f69bf9c6 Jason Baron          2016-08-03  191  
4c5ea0a9c Paolo Bonzini        2016-06-21  192  static inline int static_key_count(struct static_key *key)
4c5ea0a9c Paolo Bonzini        2016-06-21  193  {
4c5ea0a9c Paolo Bonzini        2016-06-21 @194  	return atomic_read(&key->enabled);
4c5ea0a9c Paolo Bonzini        2016-06-21  195  }
4c5ea0a9c Paolo Bonzini        2016-06-21  196  
97ce2c88f Jeremy Fitzhardinge  2011-10-12  197  static __always_inline void jump_label_init(void)
97ce2c88f Jeremy Fitzhardinge  2011-10-12  198  {
c4b2c0c5f Hannes Frederic Sowa 2013-10-19  199  	static_key_initialized = true;
97ce2c88f Jeremy Fitzhardinge  2011-10-12  200  }
97ce2c88f Jeremy Fitzhardinge  2011-10-12  201  
578ae447e Josh Poimboeuf       2018-03-19  202  static inline void jump_label_invalidate_initmem(void) {}
333522447 Josh Poimboeuf       2018-02-20  203  
c5905afb0 Ingo Molnar          2012-02-24  204  static __always_inline bool static_key_false(struct static_key *key)
c5905afb0 Ingo Molnar          2012-02-24  205  {
ea5e9539a Mel Gorman           2014-06-04  206  	if (unlikely(static_key_count(key) > 0))
c5905afb0 Ingo Molnar          2012-02-24  207  		return true;
c5905afb0 Ingo Molnar          2012-02-24  208  	return false;
c5905afb0 Ingo Molnar          2012-02-24  209  }
c5905afb0 Ingo Molnar          2012-02-24  210  
c5905afb0 Ingo Molnar          2012-02-24  211  static __always_inline bool static_key_true(struct static_key *key)
d430d3d7e Jason Baron          2011-03-16  212  {
ea5e9539a Mel Gorman           2014-06-04  213  	if (likely(static_key_count(key) > 0))
d430d3d7e Jason Baron          2011-03-16  214  		return true;
d430d3d7e Jason Baron          2011-03-16  215  	return false;
d430d3d7e Jason Baron          2011-03-16  216  }
bf5438fca Jason Baron          2010-09-17  217  
c5905afb0 Ingo Molnar          2012-02-24  218  static inline void static_key_slow_inc(struct static_key *key)
d430d3d7e Jason Baron          2011-03-16  219  {
5cdda5117 Borislav Petkov      2017-10-18  220  	STATIC_KEY_CHECK_USE(key);
d430d3d7e Jason Baron          2011-03-16 @221  	atomic_inc(&key->enabled);
d430d3d7e Jason Baron          2011-03-16  222  }
bf5438fca Jason Baron          2010-09-17  223  
c5905afb0 Ingo Molnar          2012-02-24  224  static inline void static_key_slow_dec(struct static_key *key)
bf5438fca Jason Baron          2010-09-17  225  {
5cdda5117 Borislav Petkov      2017-10-18  226  	STATIC_KEY_CHECK_USE(key);
d430d3d7e Jason Baron          2011-03-16 @227  	atomic_dec(&key->enabled);
bf5438fca Jason Baron          2010-09-17  228  }
bf5438fca Jason Baron          2010-09-17  229  
ce48c1464 Peter Zijlstra       2018-01-22  230  #define static_key_slow_inc_cpuslocked(key) static_key_slow_inc(key)
ce48c1464 Peter Zijlstra       2018-01-22  231  #define static_key_slow_dec_cpuslocked(key) static_key_slow_dec(key)
ce48c1464 Peter Zijlstra       2018-01-22  232  
4c3ef6d79 Jason Baron          2010-09-17  233  static inline int jump_label_text_reserved(void *start, void *end)
4c3ef6d79 Jason Baron          2010-09-17  234  {
4c3ef6d79 Jason Baron          2010-09-17  235  	return 0;
4c3ef6d79 Jason Baron          2010-09-17  236  }
4c3ef6d79 Jason Baron          2010-09-17  237  
91bad2f8d Jason Baron          2010-10-01  238  static inline void jump_label_lock(void) {}
91bad2f8d Jason Baron          2010-10-01  239  static inline void jump_label_unlock(void) {}
91bad2f8d Jason Baron          2010-10-01  240  
d430d3d7e Jason Baron          2011-03-16  241  static inline int jump_label_apply_nops(struct module *mod)
d430d3d7e Jason Baron          2011-03-16  242  {
d430d3d7e Jason Baron          2011-03-16  243  	return 0;
d430d3d7e Jason Baron          2011-03-16  244  }
b20295207 Gleb Natapov         2011-11-27  245  
e33886b38 Peter Zijlstra       2015-07-24  246  static inline void static_key_enable(struct static_key *key)
e33886b38 Peter Zijlstra       2015-07-24  247  {
5cdda5117 Borislav Petkov      2017-10-18  248  	STATIC_KEY_CHECK_USE(key);
e33886b38 Peter Zijlstra       2015-07-24  249  
1dbb6704d Paolo Bonzini        2017-08-01  250  	if (atomic_read(&key->enabled) != 0) {
1dbb6704d Paolo Bonzini        2017-08-01  251  		WARN_ON_ONCE(atomic_read(&key->enabled) != 1);
1dbb6704d Paolo Bonzini        2017-08-01  252  		return;
1dbb6704d Paolo Bonzini        2017-08-01  253  	}
1dbb6704d Paolo Bonzini        2017-08-01 @254  	atomic_set(&key->enabled, 1);
e33886b38 Peter Zijlstra       2015-07-24  255  }
e33886b38 Peter Zijlstra       2015-07-24  256  

:::::: The code at line 194 was first introduced by commit
:::::: 4c5ea0a9cd02d6aa8adc86e100b2a4cff8d614ff locking/static_key: Fix concurrent static_key_slow_inc()

:::::: TO: Paolo Bonzini <pbonzini@redhat.com>
:::::: CC: Ingo Molnar <mingo@kernel.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Laura Abbott June 25, 2018, 7:44 p.m. UTC | #6
On 06/25/2018 08:22 AM, Christoph von Recklinghausen wrote:
> Add correct address for linux-mm
> 
> On 06/25/2018 11:08 AM, Chris von Recklinghausen wrote:
>> Enabling HARDENED_USER_COPY causes measurable regressions in the
>> networking performances, up to 8% under UDP flood.
>>
>> A generic distro may want to enable HARDENED_USER_COPY in their default
>> kernel config, but at the same time, such distro may want to be able to
>> avoid the performance penalties in with the default configuration and
>> enable the stricter check on a per-boot basis.
>>
>> This change adds a config variable and a boot parameter to conditionally
>> enable HARDENED_USER_COPY at boot time, and switch HUC to off if
>> HUC_DEFAULT_OFF is set.
>>
>> Signed-off-by: Chris von Recklinghausen <crecklin@redhat.com>
>> ---
>>   .../admin-guide/kernel-parameters.rst         |  2 ++
>>   .../admin-guide/kernel-parameters.txt         |  3 ++
>>   include/linux/thread_info.h                   |  7 +++++
>>   mm/usercopy.c                                 | 28 +++++++++++++++++++
>>   security/Kconfig                              | 10 +++++++
>>   5 files changed, 50 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
>> index b8d0bc07ed0a..c3035038e3ae 100644
>> --- a/Documentation/admin-guide/kernel-parameters.rst
>> +++ b/Documentation/admin-guide/kernel-parameters.rst
>> @@ -100,6 +100,8 @@ parameter is applicable::
>>   	FB	The frame buffer device is enabled.
>>   	FTRACE	Function tracing enabled.
>>   	GCOV	GCOV profiling is enabled.
>> +	HUC	Hardened usercopy is enabled
>> +	HUCF	Hardened usercopy disabled at boot
>>   	HW	Appropriate hardware is enabled.
>>   	IA-64	IA-64 architecture is enabled.
>>   	IMA     Integrity measurement architecture is enabled.
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index efc7aa7a0670..cd3354bc14d3 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -816,6 +816,9 @@
>>   	disable=	[IPV6]
>>   			See Documentation/networking/ipv6.txt.
>>   
>> +	enable_hardened_usercopy [HUC,HUCF]
>> +			Enable hardened usercopy checks
>> +
>>   	disable_radix	[PPC]
>>   			Disable RADIX MMU mode on POWER9
>>   
>> diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
>> index 8d8821b3689a..140a36cc1c2c 100644
>> --- a/include/linux/thread_info.h
>> +++ b/include/linux/thread_info.h
>> @@ -109,12 +109,19 @@ static inline int arch_within_stack_frames(const void * const stack,
>>   #endif
>>   
>>   #ifdef CONFIG_HARDENED_USERCOPY
>> +#include <linux/jump_label.h>
>> +
>> +DECLARE_STATIC_KEY_FALSE(bypass_usercopy_checks);
>> +
>>   extern void __check_object_size(const void *ptr, unsigned long n,
>>   					bool to_user);
>>   
>>   static __always_inline void check_object_size(const void *ptr, unsigned long n,
>>   					      bool to_user)
>>   {
>> +	if (static_branch_likely(&bypass_usercopy_checks))
>> +		return;
>> +
>>   	if (!__builtin_constant_p(n))
>>   		__check_object_size(ptr, n, to_user);
>>   }
>> diff --git a/mm/usercopy.c b/mm/usercopy.c
>> index e9e9325f7638..ce3996da1b2e 100644
>> --- a/mm/usercopy.c
>> +++ b/mm/usercopy.c
>> @@ -279,3 +279,31 @@ void __check_object_size(const void *ptr, unsigned long n, bool to_user)
>>   	check_kernel_text_object((const unsigned long)ptr, n, to_user);
>>   }
>>   EXPORT_SYMBOL(__check_object_size);
>> +
>> +DEFINE_STATIC_KEY_FALSE(bypass_usercopy_checks);
>> +EXPORT_SYMBOL(bypass_usercopy_checks);
>> +
>> +#ifdef CONFIG_HUC_DEFAULT_OFF
>> +#define HUC_DEFAULT false
>> +#else
>> +#define HUC_DEFAULT true
>> +#endif
>> +
>> +static bool enable_huc_atboot = HUC_DEFAULT;
>> +
>> +static int __init parse_enable_usercopy(char *str)
>> +{
>> +	enable_huc_atboot = true;
>> +	return 1;
>> +}
>> +
>> +static int __init set_enable_usercopy(void)
>> +{
>> +	if (enable_huc_atboot == false)
>> +		static_branch_enable(&bypass_usercopy_checks);
>> +	return 1;
>> +}
>> +
>> +__setup("enable_hardened_usercopy", parse_enable_usercopy);
>> +
>> +late_initcall(set_enable_usercopy);
>> diff --git a/security/Kconfig b/security/Kconfig
>> index c4302067a3ad..a6173897b85c 100644
>> --- a/security/Kconfig
>> +++ b/security/Kconfig
>> @@ -189,6 +189,16 @@ config HARDENED_USERCOPY_PAGESPAN
>>   	  been removed. This config is intended to be used only while
>>   	  trying to find such users.
>>   
>> +config HUC_DEFAULT_OFF
>> +	bool "allow CONFIG_HARDENED_USERCOPY to be configured but disabled"
>> +	depends on HARDENED_USERCOPY
>> +	help
>> +	  When CONFIG_HARDENED_USERCOPY is enabled, disable its
>> +	  functionality unless it is enabled via at boot time
>> +	  via the "enable_hardened_usercopy" boot parameter. This allows
>> +	  the functionality of hardened usercopy to be present but not
>> +	  impact performance unless it is needed.
>> +
>>   config FORTIFY_SOURCE
>>   	bool "Harden common str/mem functions against buffer overflows"
>>   	depends on ARCH_HAS_FORTIFY_SOURCE
> 
> 

This seems a bit backwards, I'd much rather see hardened user copy
default to on with the basic config option and then just have a command
line option to turn it off.

Thanks,
Laura
Chris von Recklinghausen June 25, 2018, 10:29 p.m. UTC | #7
On 06/25/2018 03:44 PM, Laura Abbott wrote:
> On 06/25/2018 08:22 AM, Christoph von Recklinghausen wrote:
>> Add correct address for linux-mm
>>
>> On 06/25/2018 11:08 AM, Chris von Recklinghausen wrote:
>>> Enabling HARDENED_USER_COPY causes measurable regressions in the
>>> networking performances, up to 8% under UDP flood.
>>>
>>> A generic distro may want to enable HARDENED_USER_COPY in their default
>>> kernel config, but at the same time, such distro may want to be able to
>>> avoid the performance penalties in with the default configuration and
>>> enable the stricter check on a per-boot basis.
>>>
>>> This change adds a config variable and a boot parameter to
>>> conditionally
>>> enable HARDENED_USER_COPY at boot time, and switch HUC to off if
>>> HUC_DEFAULT_OFF is set.
>>>
>>> Signed-off-by: Chris von Recklinghausen <crecklin@redhat.com>
>>> ---
>>>   .../admin-guide/kernel-parameters.rst         |  2 ++
>>>   .../admin-guide/kernel-parameters.txt         |  3 ++
>>>   include/linux/thread_info.h                   |  7 +++++
>>>   mm/usercopy.c                                 | 28
>>> +++++++++++++++++++
>>>   security/Kconfig                              | 10 +++++++
>>>   5 files changed, 50 insertions(+)
>>>
>>> diff --git a/Documentation/admin-guide/kernel-parameters.rst
>>> b/Documentation/admin-guide/kernel-parameters.rst
>>> index b8d0bc07ed0a..c3035038e3ae 100644
>>> --- a/Documentation/admin-guide/kernel-parameters.rst
>>> +++ b/Documentation/admin-guide/kernel-parameters.rst
>>> @@ -100,6 +100,8 @@ parameter is applicable::
>>>       FB    The frame buffer device is enabled.
>>>       FTRACE    Function tracing enabled.
>>>       GCOV    GCOV profiling is enabled.
>>> +    HUC    Hardened usercopy is enabled
>>> +    HUCF    Hardened usercopy disabled at boot
>>>       HW    Appropriate hardware is enabled.
>>>       IA-64    IA-64 architecture is enabled.
>>>       IMA     Integrity measurement architecture is enabled.
>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt
>>> b/Documentation/admin-guide/kernel-parameters.txt
>>> index efc7aa7a0670..cd3354bc14d3 100644
>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>> @@ -816,6 +816,9 @@
>>>       disable=    [IPV6]
>>>               See Documentation/networking/ipv6.txt.
>>>   +    enable_hardened_usercopy [HUC,HUCF]
>>> +            Enable hardened usercopy checks
>>> +
>>>       disable_radix    [PPC]
>>>               Disable RADIX MMU mode on POWER9
>>>   diff --git a/include/linux/thread_info.h
>>> b/include/linux/thread_info.h
>>> index 8d8821b3689a..140a36cc1c2c 100644
>>> --- a/include/linux/thread_info.h
>>> +++ b/include/linux/thread_info.h
>>> @@ -109,12 +109,19 @@ static inline int
>>> arch_within_stack_frames(const void * const stack,
>>>   #endif
>>>     #ifdef CONFIG_HARDENED_USERCOPY
>>> +#include <linux/jump_label.h>
>>> +
>>> +DECLARE_STATIC_KEY_FALSE(bypass_usercopy_checks);
>>> +
>>>   extern void __check_object_size(const void *ptr, unsigned long n,
>>>                       bool to_user);
>>>     static __always_inline void check_object_size(const void *ptr,
>>> unsigned long n,
>>>                             bool to_user)
>>>   {
>>> +    if (static_branch_likely(&bypass_usercopy_checks))
>>> +        return;
>>> +
>>>       if (!__builtin_constant_p(n))
>>>           __check_object_size(ptr, n, to_user);
>>>   }
>>> diff --git a/mm/usercopy.c b/mm/usercopy.c
>>> index e9e9325f7638..ce3996da1b2e 100644
>>> --- a/mm/usercopy.c
>>> +++ b/mm/usercopy.c
>>> @@ -279,3 +279,31 @@ void __check_object_size(const void *ptr,
>>> unsigned long n, bool to_user)
>>>       check_kernel_text_object((const unsigned long)ptr, n, to_user);
>>>   }
>>>   EXPORT_SYMBOL(__check_object_size);
>>> +
>>> +DEFINE_STATIC_KEY_FALSE(bypass_usercopy_checks);
>>> +EXPORT_SYMBOL(bypass_usercopy_checks);
>>> +
>>> +#ifdef CONFIG_HUC_DEFAULT_OFF
>>> +#define HUC_DEFAULT false
>>> +#else
>>> +#define HUC_DEFAULT true
>>> +#endif
>>> +
>>> +static bool enable_huc_atboot = HUC_DEFAULT;
>>> +
>>> +static int __init parse_enable_usercopy(char *str)
>>> +{
>>> +    enable_huc_atboot = true;
>>> +    return 1;
>>> +}
>>> +
>>> +static int __init set_enable_usercopy(void)
>>> +{
>>> +    if (enable_huc_atboot == false)
>>> +        static_branch_enable(&bypass_usercopy_checks);
>>> +    return 1;
>>> +}
>>> +
>>> +__setup("enable_hardened_usercopy", parse_enable_usercopy);
>>> +
>>> +late_initcall(set_enable_usercopy);
>>> diff --git a/security/Kconfig b/security/Kconfig
>>> index c4302067a3ad..a6173897b85c 100644
>>> --- a/security/Kconfig
>>> +++ b/security/Kconfig
>>> @@ -189,6 +189,16 @@ config HARDENED_USERCOPY_PAGESPAN
>>>         been removed. This config is intended to be used only while
>>>         trying to find such users.
>>>   +config HUC_DEFAULT_OFF
>>> +    bool "allow CONFIG_HARDENED_USERCOPY to be configured but
>>> disabled"
>>> +    depends on HARDENED_USERCOPY
>>> +    help
>>> +      When CONFIG_HARDENED_USERCOPY is enabled, disable its
>>> +      functionality unless it is enabled via at boot time
>>> +      via the "enable_hardened_usercopy" boot parameter. This allows
>>> +      the functionality of hardened usercopy to be present but not
>>> +      impact performance unless it is needed.
>>> +
>>>   config FORTIFY_SOURCE
>>>       bool "Harden common str/mem functions against buffer overflows"
>>>       depends on ARCH_HAS_FORTIFY_SOURCE
>>
>>
>
> This seems a bit backwards, I'd much rather see hardened user copy
> default to on with the basic config option and then just have a command
> line option to turn it off.
>
> Thanks,
> Laura

I have a small set of customers that want CONFIG_HARDENED_USERCOPY
enabled, and a large number of customers who would be impacted by its
default behavior (before my change).  The desire was to have the smaller
number of users need to change their boot lines to get the behavior they
wanted. Adding CONFIG_HUC_DEFAULT_OFF was an attempt to preserve the
default behavior of existing users of CONFIG_HARDENED_USERCOPY (default
enabled) and allowing that to coexist with the desires of the greater
number of my customers (default disabled).

If folks think that it's better to have it enabled by default and the
command line option to turn it off I can do that (it is simpler). Does
anyone else have opinions one way or the other?

Thanks,

Chris
Kees Cook June 25, 2018, 10:35 p.m. UTC | #8
On Mon, Jun 25, 2018 at 3:29 PM, Christoph von Recklinghausen
<crecklin@redhat.com> wrote:
> I have a small set of customers that want CONFIG_HARDENED_USERCOPY
> enabled, and a large number of customers who would be impacted by its
> default behavior (before my change).  The desire was to have the smaller
> number of users need to change their boot lines to get the behavior they
> wanted. Adding CONFIG_HUC_DEFAULT_OFF was an attempt to preserve the
> default behavior of existing users of CONFIG_HARDENED_USERCOPY (default
> enabled) and allowing that to coexist with the desires of the greater
> number of my customers (default disabled).
>
> If folks think that it's better to have it enabled by default and the
> command line option to turn it off I can do that (it is simpler). Does
> anyone else have opinions one way or the other?

I would prefer to isolate the actual problem case, and fix it if
possible. (i.e. try to make the copy fixed-length, etc) Barring that,
yes, a kernel command line to disable the protection would be okay.

Note that the test needs to be inside __check_object_size() otherwise
the inline optimization with __builtin_constant_p() gets broken and
makes everyone slower. :)

-Kees
Chris von Recklinghausen June 25, 2018, 11:17 p.m. UTC | #9
On 06/25/2018 06:35 PM, Kees Cook wrote:
> On Mon, Jun 25, 2018 at 3:29 PM, Christoph von Recklinghausen
> <crecklin@redhat.com> wrote:
>> I have a small set of customers that want CONFIG_HARDENED_USERCOPY
>> enabled, and a large number of customers who would be impacted by its
>> default behavior (before my change).  The desire was to have the smaller
>> number of users need to change their boot lines to get the behavior they
>> wanted. Adding CONFIG_HUC_DEFAULT_OFF was an attempt to preserve the
>> default behavior of existing users of CONFIG_HARDENED_USERCOPY (default
>> enabled) and allowing that to coexist with the desires of the greater
>> number of my customers (default disabled).
>>
>> If folks think that it's better to have it enabled by default and the
>> command line option to turn it off I can do that (it is simpler). Does
>> anyone else have opinions one way or the other?
> I would prefer to isolate the actual problem case, and fix it if
> possible. (i.e. try to make the copy fixed-length, etc) Barring that,
> yes, a kernel command line to disable the protection would be okay.
>
> Note that the test needs to be inside __check_object_size() otherwise
> the inline optimization with __builtin_constant_p() gets broken and
> makes everyone slower. :)
>
> -Kees
>
Thanks Kees,

I'll make that change and retest.

Chris
Paolo Abeni June 26, 2018, 9:48 a.m. UTC | #10
Hi,

On Mon, 25 Jun 2018 11:21:38 -0700 Kees Cook <keescook@chromium.org> wrote:
> On Mon, Jun 25, 2018 at 8:08 AM, Chris von Recklinghausen
> <crecklin@redhat.com> wrote:
> > Enabling HARDENED_USER_COPY causes measurable regressions in the
> > networking performances, up to 8% under UDP flood.
> 
> Which function is "hot"? i.e. which copy*user() is taking up the time?
> Do you have a workload that at can be used to reproduce the problem?

I'm running an a small packet UDP flood using pktgen vs. an host b2b
connected. On the receiver side the UDP packets are processed by a
simple user space process that just read and drop them:

https://github.com/netoptimizer/network-testing/blob/master/src/udp_sink.c

Not very useful from a functional PoV, it helps mostly pin-pointing
bottle-neck in the networking stack.

When running a kernel with CONFIG_HARDENED_USERCOPY=y, I see a 5-8%
regression in the receive tput, compared to the same kernel without
such option.

With CONFIG_HARDENED_USERCOPY=y, perf shows ~6% of CPU time spent
cumulatively in __check_object_size (~4%) and __virt_addr_valid (~2%).

Cheers,

Paolo
Kees Cook June 26, 2018, 4:54 p.m. UTC | #11
On Tue, Jun 26, 2018 at 2:48 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> Hi,
>
> On Mon, 25 Jun 2018 11:21:38 -0700 Kees Cook <keescook@chromium.org> wrote:
>> On Mon, Jun 25, 2018 at 8:08 AM, Chris von Recklinghausen
>> <crecklin@redhat.com> wrote:
>> > Enabling HARDENED_USER_COPY causes measurable regressions in the
>> > networking performances, up to 8% under UDP flood.
>>
>> Which function is "hot"? i.e. which copy*user() is taking up the time?
>> Do you have a workload that at can be used to reproduce the problem?
>
> I'm running an a small packet UDP flood using pktgen vs. an host b2b
> connected. On the receiver side the UDP packets are processed by a
> simple user space process that just read and drop them:
>
> https://github.com/netoptimizer/network-testing/blob/master/src/udp_sink.c
>
> Not very useful from a functional PoV, it helps mostly pin-pointing
> bottle-neck in the networking stack.

Cool; thanks for the pointer!

> When running a kernel with CONFIG_HARDENED_USERCOPY=y, I see a 5-8%
> regression in the receive tput, compared to the same kernel without
> such option.
>
> With CONFIG_HARDENED_USERCOPY=y, perf shows ~6% of CPU time spent
> cumulatively in __check_object_size (~4%) and __virt_addr_valid (~2%).

Are you able to see which network functions are making the
__check_object_size() calls?

-Kees
Paolo Abeni June 26, 2018, 5:14 p.m. UTC | #12
[hopefully fixed the 'mm' recipient]

On Tue, 2018-06-26 at 09:54 -0700, Kees Cook wrote:
> On Tue, Jun 26, 2018 at 2:48 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> > With CONFIG_HARDENED_USERCOPY=y, perf shows ~6% of CPU time spent
> > cumulatively in __check_object_size (~4%) and __virt_addr_valid (~2%).
> 
> Are you able to see which network functions are making the
> __check_object_size() calls?

The call-chain is:

__GI___libc_recvfrom                                                   
entry_SYSCALL_64_after_hwframe                                         
do_syscall_64                                                          
__x64_sys_recvfrom                                                     
__sys_recvfrom                                                         
inet_recvmsg                                                           
udp_recvmsg                                                            
__check_object_size

udp_recvmsg() actually calls copy_to_iter() (inlined) and the latters
calls check_copy_size() (again, inlined).

Cheers,

Paolo

Patch
diff mbox series

diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
index b8d0bc07ed0a..c3035038e3ae 100644
--- a/Documentation/admin-guide/kernel-parameters.rst
+++ b/Documentation/admin-guide/kernel-parameters.rst
@@ -100,6 +100,8 @@  parameter is applicable::
 	FB	The frame buffer device is enabled.
 	FTRACE	Function tracing enabled.
 	GCOV	GCOV profiling is enabled.
+	HUC	Hardened usercopy is enabled
+	HUCF	Hardened usercopy disabled at boot
 	HW	Appropriate hardware is enabled.
 	IA-64	IA-64 architecture is enabled.
 	IMA     Integrity measurement architecture is enabled.
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index efc7aa7a0670..cd3354bc14d3 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -816,6 +816,9 @@ 
 	disable=	[IPV6]
 			See Documentation/networking/ipv6.txt.
 
+	enable_hardened_usercopy [HUC,HUCF]
+			Enable hardened usercopy checks
+
 	disable_radix	[PPC]
 			Disable RADIX MMU mode on POWER9
 
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index 8d8821b3689a..140a36cc1c2c 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -109,12 +109,19 @@  static inline int arch_within_stack_frames(const void * const stack,
 #endif
 
 #ifdef CONFIG_HARDENED_USERCOPY
+#include <linux/jump_label.h>
+
+DECLARE_STATIC_KEY_FALSE(bypass_usercopy_checks);
+
 extern void __check_object_size(const void *ptr, unsigned long n,
 					bool to_user);
 
 static __always_inline void check_object_size(const void *ptr, unsigned long n,
 					      bool to_user)
 {
+	if (static_branch_likely(&bypass_usercopy_checks))
+		return;
+
 	if (!__builtin_constant_p(n))
 		__check_object_size(ptr, n, to_user);
 }
diff --git a/mm/usercopy.c b/mm/usercopy.c
index e9e9325f7638..ce3996da1b2e 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -279,3 +279,31 @@  void __check_object_size(const void *ptr, unsigned long n, bool to_user)
 	check_kernel_text_object((const unsigned long)ptr, n, to_user);
 }
 EXPORT_SYMBOL(__check_object_size);
+
+DEFINE_STATIC_KEY_FALSE(bypass_usercopy_checks);
+EXPORT_SYMBOL(bypass_usercopy_checks);
+
+#ifdef CONFIG_HUC_DEFAULT_OFF
+#define HUC_DEFAULT false
+#else
+#define HUC_DEFAULT true
+#endif
+
+static bool enable_huc_atboot = HUC_DEFAULT;
+
+static int __init parse_enable_usercopy(char *str)
+{
+	enable_huc_atboot = true;
+	return 1;
+}
+
+static int __init set_enable_usercopy(void)
+{
+	if (enable_huc_atboot == false)
+		static_branch_enable(&bypass_usercopy_checks);
+	return 1;
+}
+
+__setup("enable_hardened_usercopy", parse_enable_usercopy);
+
+late_initcall(set_enable_usercopy);
diff --git a/security/Kconfig b/security/Kconfig
index c4302067a3ad..a6173897b85c 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -189,6 +189,16 @@  config HARDENED_USERCOPY_PAGESPAN
 	  been removed. This config is intended to be used only while
 	  trying to find such users.
 
+config HUC_DEFAULT_OFF
+	bool "allow CONFIG_HARDENED_USERCOPY to be configured but disabled"
+	depends on HARDENED_USERCOPY
+	help
+	  When CONFIG_HARDENED_USERCOPY is enabled, disable its
+	  functionality unless it is enabled via at boot time
+	  via the "enable_hardened_usercopy" boot parameter. This allows
+	  the functionality of hardened usercopy to be present but not
+	  impact performance unless it is needed.
+
 config FORTIFY_SOURCE
 	bool "Harden common str/mem functions against buffer overflows"
 	depends on ARCH_HAS_FORTIFY_SOURCE