[RFC/RFT,v2] ARM: smp: add support for per-task stack canaries
diff mbox series

Message ID 20181127001534.24075-1-ard.biesheuvel@linaro.org
State New
Headers show
Series
  • [RFC/RFT,v2] ARM: smp: add support for per-task stack canaries
Related show

Commit Message

Ard Biesheuvel Nov. 27, 2018, 12:15 a.m. UTC
On ARM, we currently only change the value of the stack canary when
switching tasks if the kernel was built for UP. On SMP kernels, this
is impossible since the stack canary value is obtained via a global
symbol reference, which means
a) all running tasks on all CPUs must use the same value
b) we can only modify the value when no kernel stack frames are live
   on any CPU, which is effectively never.

So instead, use a GCC plugin to add a RTL pass that replaces each
reference to the address of the __stack_chk_guard symbol with an
expression that produces the address of the 'stack_canary' field
that is added to struct thread_info. This way, each task will use
its own randomized value.

Cc: Russell King <linux@armlinux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Emese Revfy <re.emese@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
Cc: Laura Abbott <labbott@redhat.com>
Cc: kernel-hardening@lists.openwall.com
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
v2:
- adopt powerpc Makefile approach to pass stack canary offset and thread
  size to the plugin as command line arguments
- depend on !XIP_DEFLATED_DATA

 arch/arm/Kconfig                              |  16 +++
 arch/arm/Makefile                             |  11 +++
 arch/arm/boot/compressed/Makefile             |   1 +
 arch/arm/include/asm/stackprotector.h         |  12 ++-
 arch/arm/include/asm/thread_info.h            |   3 +
 arch/arm/kernel/asm-offsets.c                 |   1 +
 arch/arm/kernel/process.c                     |   6 +-
 scripts/Makefile.gcc-plugins                  |   6 ++
 scripts/gcc-plugins/Kconfig                   |   4 +
 scripts/gcc-plugins/arm_ssp_per_task_plugin.c | 103 ++++++++++++++++++++
 10 files changed, 160 insertions(+), 3 deletions(-)

Comments

Nicolas Pitre Nov. 29, 2018, 10:58 p.m. UTC | #1
On Tue, 27 Nov 2018, Ard Biesheuvel wrote:

> On ARM, we currently only change the value of the stack canary when
> switching tasks if the kernel was built for UP. On SMP kernels, this
> is impossible since the stack canary value is obtained via a global
> symbol reference, which means
> a) all running tasks on all CPUs must use the same value
> b) we can only modify the value when no kernel stack frames are live
>    on any CPU, which is effectively never.
> 
> So instead, use a GCC plugin to add a RTL pass that replaces each
> reference to the address of the __stack_chk_guard symbol with an
> expression that produces the address of the 'stack_canary' field
> that is added to struct thread_info. This way, each task will use
> its own randomized value.
> 
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Emese Revfy <re.emese@gmail.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> Cc: Laura Abbott <labbott@redhat.com>
> Cc: kernel-hardening@lists.openwall.com
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> v2:
> - adopt powerpc Makefile approach to pass stack canary offset and thread
>   size to the plugin as command line arguments
> - depend on !XIP_DEFLATED_DATA
> 
>  arch/arm/Kconfig                              |  16 +++
>  arch/arm/Makefile                             |  11 +++
>  arch/arm/boot/compressed/Makefile             |   1 +
>  arch/arm/include/asm/stackprotector.h         |  12 ++-
>  arch/arm/include/asm/thread_info.h            |   3 +
>  arch/arm/kernel/asm-offsets.c                 |   1 +
>  arch/arm/kernel/process.c                     |   6 +-
>  scripts/Makefile.gcc-plugins                  |   6 ++
>  scripts/gcc-plugins/Kconfig                   |   4 +
>  scripts/gcc-plugins/arm_ssp_per_task_plugin.c | 103 ++++++++++++++++++++
>  10 files changed, 160 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 91be74d8df65..6142e83b45da 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1810,6 +1810,22 @@ config XEN
>  	help
>  	  Say Y if you want to run Linux in a Virtual Machine on Xen on ARM.
>  
> +config STACKPROTECTOR_PER_TASK
> +	bool "Use a unique stack canary value for each task"
> +	depends on GCC_PLUGINS && STACKPROTECTOR && SMP && !XIP_DEFLATED_DATA
> +	select GCC_PLUGIN_ARM_SSP_PER_TASK
> +	help
> +	  Due to the fact that GCC uses an ordinary symbol reference from
> +	  which to load the value of the stack canary, this value can only
> +	  change at reboot time on SMP systems, and all tasks running in the
> +	  kernel's address space are forced to use the same canary value for
> +	  the entire duration that the system is up.
> +
> +	  Enable this option to switch to a different method that uses a
> +	  different canary value for each task.
> +
> +	  This is not enabled by default since it relies on a GCC plugin.

Why wouldn't you default it to y and let the dependencies disable it 
when not met?

Other than that:

Acked-by: Nicolas Pitre <nico@linaro.org>


Nicolas
Ard Biesheuvel Nov. 30, 2018, 8:42 a.m. UTC | #2
On Thu, 29 Nov 2018 at 23:58, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
>
> On Tue, 27 Nov 2018, Ard Biesheuvel wrote:
>
> > On ARM, we currently only change the value of the stack canary when
> > switching tasks if the kernel was built for UP. On SMP kernels, this
> > is impossible since the stack canary value is obtained via a global
> > symbol reference, which means
> > a) all running tasks on all CPUs must use the same value
> > b) we can only modify the value when no kernel stack frames are live
> >    on any CPU, which is effectively never.
> >
> > So instead, use a GCC plugin to add a RTL pass that replaces each
> > reference to the address of the __stack_chk_guard symbol with an
> > expression that produces the address of the 'stack_canary' field
> > that is added to struct thread_info. This way, each task will use
> > its own randomized value.
> >
> > Cc: Russell King <linux@armlinux.org.uk>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Emese Revfy <re.emese@gmail.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> > Cc: Laura Abbott <labbott@redhat.com>
> > Cc: kernel-hardening@lists.openwall.com
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> > v2:
> > - adopt powerpc Makefile approach to pass stack canary offset and thread
> >   size to the plugin as command line arguments
> > - depend on !XIP_DEFLATED_DATA
> >
> >  arch/arm/Kconfig                              |  16 +++
> >  arch/arm/Makefile                             |  11 +++
> >  arch/arm/boot/compressed/Makefile             |   1 +
> >  arch/arm/include/asm/stackprotector.h         |  12 ++-
> >  arch/arm/include/asm/thread_info.h            |   3 +
> >  arch/arm/kernel/asm-offsets.c                 |   1 +
> >  arch/arm/kernel/process.c                     |   6 +-
> >  scripts/Makefile.gcc-plugins                  |   6 ++
> >  scripts/gcc-plugins/Kconfig                   |   4 +
> >  scripts/gcc-plugins/arm_ssp_per_task_plugin.c | 103 ++++++++++++++++++++
> >  10 files changed, 160 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index 91be74d8df65..6142e83b45da 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -1810,6 +1810,22 @@ config XEN
> >       help
> >         Say Y if you want to run Linux in a Virtual Machine on Xen on ARM.
> >
> > +config STACKPROTECTOR_PER_TASK
> > +     bool "Use a unique stack canary value for each task"
> > +     depends on GCC_PLUGINS && STACKPROTECTOR && SMP && !XIP_DEFLATED_DATA
> > +     select GCC_PLUGIN_ARM_SSP_PER_TASK
> > +     help
> > +       Due to the fact that GCC uses an ordinary symbol reference from
> > +       which to load the value of the stack canary, this value can only
> > +       change at reboot time on SMP systems, and all tasks running in the
> > +       kernel's address space are forced to use the same canary value for
> > +       the entire duration that the system is up.
> > +
> > +       Enable this option to switch to a different method that uses a
> > +       different canary value for each task.
> > +
> > +       This is not enabled by default since it relies on a GCC plugin.
>
> Why wouldn't you default it to y and let the dependencies disable it
> when not met?
>
> Other than that:
>
> Acked-by: Nicolas Pitre <nico@linaro.org>
>

Thanks Nico.

I wasn't sure about enabling it by default, though. Perhaps Kees has
any insights?
Nicolas Pitre Nov. 30, 2018, 4:37 p.m. UTC | #3
On Fri, 30 Nov 2018, Ard Biesheuvel wrote:

> On Thu, 29 Nov 2018 at 23:58, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> >
> > On Tue, 27 Nov 2018, Ard Biesheuvel wrote:
> >
> > > On ARM, we currently only change the value of the stack canary when
> > > switching tasks if the kernel was built for UP. On SMP kernels, this
> > > is impossible since the stack canary value is obtained via a global
> > > symbol reference, which means
> > > a) all running tasks on all CPUs must use the same value
> > > b) we can only modify the value when no kernel stack frames are live
> > >    on any CPU, which is effectively never.
> > >
> > > So instead, use a GCC plugin to add a RTL pass that replaces each
> > > reference to the address of the __stack_chk_guard symbol with an
> > > expression that produces the address of the 'stack_canary' field
> > > that is added to struct thread_info. This way, each task will use
> > > its own randomized value.
> > >
> > > Cc: Russell King <linux@armlinux.org.uk>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Emese Revfy <re.emese@gmail.com>
> > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> > > Cc: Laura Abbott <labbott@redhat.com>
> > > Cc: kernel-hardening@lists.openwall.com
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > > v2:
> > > - adopt powerpc Makefile approach to pass stack canary offset and thread
> > >   size to the plugin as command line arguments
> > > - depend on !XIP_DEFLATED_DATA
> > >
> > >  arch/arm/Kconfig                              |  16 +++
> > >  arch/arm/Makefile                             |  11 +++
> > >  arch/arm/boot/compressed/Makefile             |   1 +
> > >  arch/arm/include/asm/stackprotector.h         |  12 ++-
> > >  arch/arm/include/asm/thread_info.h            |   3 +
> > >  arch/arm/kernel/asm-offsets.c                 |   1 +
> > >  arch/arm/kernel/process.c                     |   6 +-
> > >  scripts/Makefile.gcc-plugins                  |   6 ++
> > >  scripts/gcc-plugins/Kconfig                   |   4 +
> > >  scripts/gcc-plugins/arm_ssp_per_task_plugin.c | 103 ++++++++++++++++++++
> > >  10 files changed, 160 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > > index 91be74d8df65..6142e83b45da 100644
> > > --- a/arch/arm/Kconfig
> > > +++ b/arch/arm/Kconfig
> > > @@ -1810,6 +1810,22 @@ config XEN
> > >       help
> > >         Say Y if you want to run Linux in a Virtual Machine on Xen on ARM.
> > >
> > > +config STACKPROTECTOR_PER_TASK
> > > +     bool "Use a unique stack canary value for each task"
> > > +     depends on GCC_PLUGINS && STACKPROTECTOR && SMP && !XIP_DEFLATED_DATA
> > > +     select GCC_PLUGIN_ARM_SSP_PER_TASK
> > > +     help
> > > +       Due to the fact that GCC uses an ordinary symbol reference from
> > > +       which to load the value of the stack canary, this value can only
> > > +       change at reboot time on SMP systems, and all tasks running in the
> > > +       kernel's address space are forced to use the same canary value for
> > > +       the entire duration that the system is up.
> > > +
> > > +       Enable this option to switch to a different method that uses a
> > > +       different canary value for each task.
> > > +
> > > +       This is not enabled by default since it relies on a GCC plugin.
> >
> > Why wouldn't you default it to y and let the dependencies disable it
> > when not met?
> >
> > Other than that:
> >
> > Acked-by: Nicolas Pitre <nico@linaro.org>
> >
> 
> Thanks Nico.
> 
> I wasn't sure about enabling it by default, though. Perhaps Kees has
> any insights?

I'd say that if you have CONFIG_GCC_PLUGINS=y and 
CONFIG_STACKPROTECTOR=y then it is pretty clear what the user expects 
already. The fact that CONFIG_STACKPROTECTOR requires a twist on SMP is 
an implementation detail not a feature.


Nicolas
Ard Biesheuvel Dec. 4, 2018, 3:04 p.m. UTC | #4
On Fri, 30 Nov 2018 at 17:37, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
>
> On Fri, 30 Nov 2018, Ard Biesheuvel wrote:
>
> > On Thu, 29 Nov 2018 at 23:58, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> > >
> > > On Tue, 27 Nov 2018, Ard Biesheuvel wrote:
> > >
> > > > On ARM, we currently only change the value of the stack canary when
> > > > switching tasks if the kernel was built for UP. On SMP kernels, this
> > > > is impossible since the stack canary value is obtained via a global
> > > > symbol reference, which means
> > > > a) all running tasks on all CPUs must use the same value
> > > > b) we can only modify the value when no kernel stack frames are live
> > > >    on any CPU, which is effectively never.
> > > >
> > > > So instead, use a GCC plugin to add a RTL pass that replaces each
> > > > reference to the address of the __stack_chk_guard symbol with an
> > > > expression that produces the address of the 'stack_canary' field
> > > > that is added to struct thread_info. This way, each task will use
> > > > its own randomized value.
> > > >
> > > > Cc: Russell King <linux@armlinux.org.uk>
> > > > Cc: Kees Cook <keescook@chromium.org>
> > > > Cc: Emese Revfy <re.emese@gmail.com>
> > > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > > Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> > > > Cc: Laura Abbott <labbott@redhat.com>
> > > > Cc: kernel-hardening@lists.openwall.com
> > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > ---
> > > > v2:
> > > > - adopt powerpc Makefile approach to pass stack canary offset and thread
> > > >   size to the plugin as command line arguments
> > > > - depend on !XIP_DEFLATED_DATA
> > > >
> > > >  arch/arm/Kconfig                              |  16 +++
> > > >  arch/arm/Makefile                             |  11 +++
> > > >  arch/arm/boot/compressed/Makefile             |   1 +
> > > >  arch/arm/include/asm/stackprotector.h         |  12 ++-
> > > >  arch/arm/include/asm/thread_info.h            |   3 +
> > > >  arch/arm/kernel/asm-offsets.c                 |   1 +
> > > >  arch/arm/kernel/process.c                     |   6 +-
> > > >  scripts/Makefile.gcc-plugins                  |   6 ++
> > > >  scripts/gcc-plugins/Kconfig                   |   4 +
> > > >  scripts/gcc-plugins/arm_ssp_per_task_plugin.c | 103 ++++++++++++++++++++
> > > >  10 files changed, 160 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > > > index 91be74d8df65..6142e83b45da 100644
> > > > --- a/arch/arm/Kconfig
> > > > +++ b/arch/arm/Kconfig
> > > > @@ -1810,6 +1810,22 @@ config XEN
> > > >       help
> > > >         Say Y if you want to run Linux in a Virtual Machine on Xen on ARM.
> > > >
> > > > +config STACKPROTECTOR_PER_TASK
> > > > +     bool "Use a unique stack canary value for each task"
> > > > +     depends on GCC_PLUGINS && STACKPROTECTOR && SMP && !XIP_DEFLATED_DATA
> > > > +     select GCC_PLUGIN_ARM_SSP_PER_TASK
> > > > +     help
> > > > +       Due to the fact that GCC uses an ordinary symbol reference from
> > > > +       which to load the value of the stack canary, this value can only
> > > > +       change at reboot time on SMP systems, and all tasks running in the
> > > > +       kernel's address space are forced to use the same canary value for
> > > > +       the entire duration that the system is up.
> > > > +
> > > > +       Enable this option to switch to a different method that uses a
> > > > +       different canary value for each task.
> > > > +
> > > > +       This is not enabled by default since it relies on a GCC plugin.
> > >
> > > Why wouldn't you default it to y and let the dependencies disable it
> > > when not met?
> > >
> > > Other than that:
> > >
> > > Acked-by: Nicolas Pitre <nico@linaro.org>
> > >
> >
> > Thanks Nico.
> >
> > I wasn't sure about enabling it by default, though. Perhaps Kees has
> > any insights?
>
> I'd say that if you have CONFIG_GCC_PLUGINS=y and
> CONFIG_STACKPROTECTOR=y then it is pretty clear what the user expects
> already. The fact that CONFIG_STACKPROTECTOR requires a twist on SMP is
> an implementation detail not a feature.
>

Fair point.
Kees Cook Dec. 5, 2018, 11:36 p.m. UTC | #5
On Tue, Dec 4, 2018 at 7:04 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Fri, 30 Nov 2018 at 17:37, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> >
> > On Fri, 30 Nov 2018, Ard Biesheuvel wrote:
> >
> > > On Thu, 29 Nov 2018 at 23:58, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> > > >
> > > > On Tue, 27 Nov 2018, Ard Biesheuvel wrote:
> > > >
> > > > > On ARM, we currently only change the value of the stack canary when
> > > > > switching tasks if the kernel was built for UP. On SMP kernels, this
> > > > > is impossible since the stack canary value is obtained via a global
> > > > > symbol reference, which means
> > > > > a) all running tasks on all CPUs must use the same value
> > > > > b) we can only modify the value when no kernel stack frames are live
> > > > >    on any CPU, which is effectively never.
> > > > >
> > > > > So instead, use a GCC plugin to add a RTL pass that replaces each
> > > > > reference to the address of the __stack_chk_guard symbol with an
> > > > > expression that produces the address of the 'stack_canary' field
> > > > > that is added to struct thread_info. This way, each task will use
> > > > > its own randomized value.
> > > > >
> > > > > Cc: Russell King <linux@armlinux.org.uk>
> > > > > Cc: Kees Cook <keescook@chromium.org>
> > > > > Cc: Emese Revfy <re.emese@gmail.com>
> > > > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > > > Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> > > > > Cc: Laura Abbott <labbott@redhat.com>
> > > > > Cc: kernel-hardening@lists.openwall.com
> > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > > ---
> > > > > v2:
> > > > > - adopt powerpc Makefile approach to pass stack canary offset and thread
> > > > >   size to the plugin as command line arguments
> > > > > - depend on !XIP_DEFLATED_DATA
> > > > >
> > > > >  arch/arm/Kconfig                              |  16 +++
> > > > >  arch/arm/Makefile                             |  11 +++
> > > > >  arch/arm/boot/compressed/Makefile             |   1 +
> > > > >  arch/arm/include/asm/stackprotector.h         |  12 ++-
> > > > >  arch/arm/include/asm/thread_info.h            |   3 +
> > > > >  arch/arm/kernel/asm-offsets.c                 |   1 +
> > > > >  arch/arm/kernel/process.c                     |   6 +-
> > > > >  scripts/Makefile.gcc-plugins                  |   6 ++
> > > > >  scripts/gcc-plugins/Kconfig                   |   4 +
> > > > >  scripts/gcc-plugins/arm_ssp_per_task_plugin.c | 103 ++++++++++++++++++++
> > > > >  10 files changed, 160 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > > > > index 91be74d8df65..6142e83b45da 100644
> > > > > --- a/arch/arm/Kconfig
> > > > > +++ b/arch/arm/Kconfig
> > > > > @@ -1810,6 +1810,22 @@ config XEN
> > > > >       help
> > > > >         Say Y if you want to run Linux in a Virtual Machine on Xen on ARM.
> > > > >
> > > > > +config STACKPROTECTOR_PER_TASK
> > > > > +     bool "Use a unique stack canary value for each task"
> > > > > +     depends on GCC_PLUGINS && STACKPROTECTOR && SMP && !XIP_DEFLATED_DATA
> > > > > +     select GCC_PLUGIN_ARM_SSP_PER_TASK
> > > > > +     help
> > > > > +       Due to the fact that GCC uses an ordinary symbol reference from
> > > > > +       which to load the value of the stack canary, this value can only
> > > > > +       change at reboot time on SMP systems, and all tasks running in the
> > > > > +       kernel's address space are forced to use the same canary value for
> > > > > +       the entire duration that the system is up.
> > > > > +
> > > > > +       Enable this option to switch to a different method that uses a
> > > > > +       different canary value for each task.
> > > > > +
> > > > > +       This is not enabled by default since it relies on a GCC plugin.
> > > >
> > > > Why wouldn't you default it to y and let the dependencies disable it
> > > > when not met?
> > > >
> > > > Other than that:
> > > >
> > > > Acked-by: Nicolas Pitre <nico@linaro.org>
> > > >
> > >
> > > Thanks Nico.
> > >
> > > I wasn't sure about enabling it by default, though. Perhaps Kees has
> > > any insights?
> >
> > I'd say that if you have CONFIG_GCC_PLUGINS=y and
> > CONFIG_STACKPROTECTOR=y then it is pretty clear what the user expects
> > already. The fact that CONFIG_STACKPROTECTOR requires a twist on SMP is
> > an implementation detail not a feature.
> >
>
> Fair point.

Agreed. This should Just Work if either the compiler supports it or
GCC_PLUGINS are enabled. For v3, should I take it via gcc-plugins or
should this go via arm? I'm happy either way.
Ard Biesheuvel Dec. 5, 2018, 11:38 p.m. UTC | #6
On Thu, 6 Dec 2018 at 00:36, Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Dec 4, 2018 at 7:04 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > On Fri, 30 Nov 2018 at 17:37, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> > >
> > > On Fri, 30 Nov 2018, Ard Biesheuvel wrote:
> > >
> > > > On Thu, 29 Nov 2018 at 23:58, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> > > > >
> > > > > On Tue, 27 Nov 2018, Ard Biesheuvel wrote:
> > > > >
> > > > > > On ARM, we currently only change the value of the stack canary when
> > > > > > switching tasks if the kernel was built for UP. On SMP kernels, this
> > > > > > is impossible since the stack canary value is obtained via a global
> > > > > > symbol reference, which means
> > > > > > a) all running tasks on all CPUs must use the same value
> > > > > > b) we can only modify the value when no kernel stack frames are live
> > > > > >    on any CPU, which is effectively never.
> > > > > >
> > > > > > So instead, use a GCC plugin to add a RTL pass that replaces each
> > > > > > reference to the address of the __stack_chk_guard symbol with an
> > > > > > expression that produces the address of the 'stack_canary' field
> > > > > > that is added to struct thread_info. This way, each task will use
> > > > > > its own randomized value.
> > > > > >
> > > > > > Cc: Russell King <linux@armlinux.org.uk>
> > > > > > Cc: Kees Cook <keescook@chromium.org>
> > > > > > Cc: Emese Revfy <re.emese@gmail.com>
> > > > > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > > > > Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> > > > > > Cc: Laura Abbott <labbott@redhat.com>
> > > > > > Cc: kernel-hardening@lists.openwall.com
> > > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > > > ---
> > > > > > v2:
> > > > > > - adopt powerpc Makefile approach to pass stack canary offset and thread
> > > > > >   size to the plugin as command line arguments
> > > > > > - depend on !XIP_DEFLATED_DATA
> > > > > >
> > > > > >  arch/arm/Kconfig                              |  16 +++
> > > > > >  arch/arm/Makefile                             |  11 +++
> > > > > >  arch/arm/boot/compressed/Makefile             |   1 +
> > > > > >  arch/arm/include/asm/stackprotector.h         |  12 ++-
> > > > > >  arch/arm/include/asm/thread_info.h            |   3 +
> > > > > >  arch/arm/kernel/asm-offsets.c                 |   1 +
> > > > > >  arch/arm/kernel/process.c                     |   6 +-
> > > > > >  scripts/Makefile.gcc-plugins                  |   6 ++
> > > > > >  scripts/gcc-plugins/Kconfig                   |   4 +
> > > > > >  scripts/gcc-plugins/arm_ssp_per_task_plugin.c | 103 ++++++++++++++++++++
> > > > > >  10 files changed, 160 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > > > > > index 91be74d8df65..6142e83b45da 100644
> > > > > > --- a/arch/arm/Kconfig
> > > > > > +++ b/arch/arm/Kconfig
> > > > > > @@ -1810,6 +1810,22 @@ config XEN
> > > > > >       help
> > > > > >         Say Y if you want to run Linux in a Virtual Machine on Xen on ARM.
> > > > > >
> > > > > > +config STACKPROTECTOR_PER_TASK
> > > > > > +     bool "Use a unique stack canary value for each task"
> > > > > > +     depends on GCC_PLUGINS && STACKPROTECTOR && SMP && !XIP_DEFLATED_DATA
> > > > > > +     select GCC_PLUGIN_ARM_SSP_PER_TASK
> > > > > > +     help
> > > > > > +       Due to the fact that GCC uses an ordinary symbol reference from
> > > > > > +       which to load the value of the stack canary, this value can only
> > > > > > +       change at reboot time on SMP systems, and all tasks running in the
> > > > > > +       kernel's address space are forced to use the same canary value for
> > > > > > +       the entire duration that the system is up.
> > > > > > +
> > > > > > +       Enable this option to switch to a different method that uses a
> > > > > > +       different canary value for each task.
> > > > > > +
> > > > > > +       This is not enabled by default since it relies on a GCC plugin.
> > > > >
> > > > > Why wouldn't you default it to y and let the dependencies disable it
> > > > > when not met?
> > > > >
> > > > > Other than that:
> > > > >
> > > > > Acked-by: Nicolas Pitre <nico@linaro.org>
> > > > >
> > > >
> > > > Thanks Nico.
> > > >
> > > > I wasn't sure about enabling it by default, though. Perhaps Kees has
> > > > any insights?
> > >
> > > I'd say that if you have CONFIG_GCC_PLUGINS=y and
> > > CONFIG_STACKPROTECTOR=y then it is pretty clear what the user expects
> > > already. The fact that CONFIG_STACKPROTECTOR requires a twist on SMP is
> > > an implementation detail not a feature.
> > >
> >
> > Fair point.
>
> Agreed. This should Just Work if either the compiler supports it or
> GCC_PLUGINS are enabled. For v3, should I take it via gcc-plugins or
> should this go via arm? I'm happy either way.
>

That's up to Russell.

Russell, would you mind if Kees takes this through his tree?

Patch
diff mbox series

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 91be74d8df65..6142e83b45da 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1810,6 +1810,22 @@  config XEN
 	help
 	  Say Y if you want to run Linux in a Virtual Machine on Xen on ARM.
 
+config STACKPROTECTOR_PER_TASK
+	bool "Use a unique stack canary value for each task"
+	depends on GCC_PLUGINS && STACKPROTECTOR && SMP && !XIP_DEFLATED_DATA
+	select GCC_PLUGIN_ARM_SSP_PER_TASK
+	help
+	  Due to the fact that GCC uses an ordinary symbol reference from
+	  which to load the value of the stack canary, this value can only
+	  change at reboot time on SMP systems, and all tasks running in the
+	  kernel's address space are forced to use the same canary value for
+	  the entire duration that the system is up.
+
+	  Enable this option to switch to a different method that uses a
+	  different canary value for each task.
+
+	  This is not enabled by default since it relies on a GCC plugin.
+
 endmenu
 
 menu "Boot options"
diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index 05a91d8b89f3..50860ee3e771 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -303,6 +303,17 @@  else
 KBUILD_IMAGE := $(boot)/zImage
 endif
 
+ifeq ($(CONFIG_STACKPROTECTOR_PER_TASK),y)
+prepare: stack_protector_prepare
+stack_protector_prepare: prepare0
+	$(eval KBUILD_CFLAGS += -fplugin-arg-arm_ssp_per_task_plugin-tso=$(shell\
+			awk '{if ($$2 == "THREAD_SZ_ORDER") print $$3;}'	\
+				include/generated/asm-offsets.h)		\
+		-fplugin-arg-arm_ssp_per_task_plugin-offset=$(shell		\
+			awk '{if ($$2 == "TSK_STACK_CANARY") print $$3;}'	\
+				include/generated/asm-offsets.h))
+endif
+
 all:	$(notdir $(KBUILD_IMAGE))
 
 
diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
index 1f5a5ffe7fcf..01bf2585a0fa 100644
--- a/arch/arm/boot/compressed/Makefile
+++ b/arch/arm/boot/compressed/Makefile
@@ -101,6 +101,7 @@  clean-files += piggy_data lib1funcs.S ashldi3.S bswapsdi2.S \
 		$(libfdt) $(libfdt_hdrs) hyp-stub.S
 
 KBUILD_CFLAGS += -DDISABLE_BRANCH_PROFILING
+KBUILD_CFLAGS += $(DISABLE_ARM_SSP_PER_TASK_PLUGIN)
 
 ifeq ($(CONFIG_FUNCTION_TRACER),y)
 ORIG_CFLAGS := $(KBUILD_CFLAGS)
diff --git a/arch/arm/include/asm/stackprotector.h b/arch/arm/include/asm/stackprotector.h
index ef5f7b69443e..72a20c3a0a90 100644
--- a/arch/arm/include/asm/stackprotector.h
+++ b/arch/arm/include/asm/stackprotector.h
@@ -6,8 +6,10 @@ 
  * the stack frame and verifying that it hasn't been overwritten when
  * returning from the function.  The pattern is called stack canary
  * and gcc expects it to be defined by a global variable called
- * "__stack_chk_guard" on ARM.  This unfortunately means that on SMP
- * we cannot have a different canary value per task.
+ * "__stack_chk_guard" on ARM.  This prevents SMP systems from using a
+ * different value for each task unless we enable a GCC plugin that
+ * replaces these symbol references with references to each task's own
+ * value.
  */
 
 #ifndef _ASM_STACKPROTECTOR_H
@@ -16,6 +18,8 @@ 
 #include <linux/random.h>
 #include <linux/version.h>
 
+#include <asm/thread_info.h>
+
 extern unsigned long __stack_chk_guard;
 
 /*
@@ -33,7 +37,11 @@  static __always_inline void boot_init_stack_canary(void)
 	canary ^= LINUX_VERSION_CODE;
 
 	current->stack_canary = canary;
+#ifndef CONFIG_STACKPROTECTOR_PER_TASK
 	__stack_chk_guard = current->stack_canary;
+#else
+	current_thread_info()->stack_canary = current->stack_canary;
+#endif
 }
 
 #endif	/* _ASM_STACKPROTECTOR_H */
diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index 8f55dc520a3e..bd661c7681b1 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -57,6 +57,9 @@  struct thread_info {
 	__u32			syscall;	/* syscall number */
 	__u8			used_cp[16];	/* thread used copro */
 	unsigned long		tp_value[2];	/* TLS registers */
+#ifdef CONFIG_STACKPROTECTOR_PER_TASK
+	unsigned long		stack_canary;
+#endif
 #ifdef CONFIG_CRUNCH
 	struct crunch_state	crunchstate;
 #endif
diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
index 3968d6c22455..2e82926645b3 100644
--- a/arch/arm/kernel/asm-offsets.c
+++ b/arch/arm/kernel/asm-offsets.c
@@ -52,6 +52,7 @@  int main(void)
   DEFINE(TSK_ACTIVE_MM,		offsetof(struct task_struct, active_mm));
 #ifdef CONFIG_STACKPROTECTOR
   DEFINE(TSK_STACK_CANARY,	offsetof(struct task_struct, stack_canary));
+  DEFINE(THREAD_SZ_ORDER,	THREAD_SIZE_ORDER);
 #endif
   BLANK();
   DEFINE(TI_FLAGS,		offsetof(struct thread_info, flags));
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 82ab015bf42b..16601d1442d1 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -39,7 +39,7 @@ 
 #include <asm/tls.h>
 #include <asm/vdso.h>
 
-#ifdef CONFIG_STACKPROTECTOR
+#if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
 #include <linux/stackprotector.h>
 unsigned long __stack_chk_guard __read_mostly;
 EXPORT_SYMBOL(__stack_chk_guard);
@@ -267,6 +267,10 @@  copy_thread(unsigned long clone_flags, unsigned long stack_start,
 
 	thread_notify(THREAD_NOTIFY_COPY, thread);
 
+#ifdef CONFIG_STACKPROTECTOR_PER_TASK
+	thread->stack_canary = p->stack_canary;
+#endif
+
 	return 0;
 }
 
diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins
index 46c5c6809806..048179d8c07f 100644
--- a/scripts/Makefile.gcc-plugins
+++ b/scripts/Makefile.gcc-plugins
@@ -36,6 +36,12 @@  ifdef CONFIG_GCC_PLUGIN_STACKLEAK
 endif
 export DISABLE_STACKLEAK_PLUGIN
 
+gcc-plugin-$(CONFIG_GCC_PLUGIN_ARM_SSP_PER_TASK) += arm_ssp_per_task_plugin.so
+ifdef CONFIG_GCC_PLUGIN_ARM_SSP_PER_TASK
+    DISABLE_ARM_SSP_PER_TASK_PLUGIN += -fplugin-arg-arm_ssp_per_task_plugin-disable
+endif
+export DISABLE_ARM_SSP_PER_TASK_PLUGIN
+
 # All the plugin CFLAGS are collected here in case a build target needs to
 # filter them out of the KBUILD_CFLAGS.
 GCC_PLUGINS_CFLAGS := $(strip $(addprefix -fplugin=$(objtree)/scripts/gcc-plugins/, $(gcc-plugin-y)) $(gcc-plugin-cflags-y))
diff --git a/scripts/gcc-plugins/Kconfig b/scripts/gcc-plugins/Kconfig
index 0d5c799688f0..d45f7f36b859 100644
--- a/scripts/gcc-plugins/Kconfig
+++ b/scripts/gcc-plugins/Kconfig
@@ -190,4 +190,8 @@  config STACKLEAK_RUNTIME_DISABLE
 	  runtime to control kernel stack erasing for kernels built with
 	  CONFIG_GCC_PLUGIN_STACKLEAK.
 
+config GCC_PLUGIN_ARM_SSP_PER_TASK
+	bool
+	depends on GCC_PLUGINS && ARM
+
 endif
diff --git a/scripts/gcc-plugins/arm_ssp_per_task_plugin.c b/scripts/gcc-plugins/arm_ssp_per_task_plugin.c
new file mode 100644
index 000000000000..de70b8470971
--- /dev/null
+++ b/scripts/gcc-plugins/arm_ssp_per_task_plugin.c
@@ -0,0 +1,103 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#include "gcc-common.h"
+
+__visible int plugin_is_GPL_compatible;
+
+static unsigned int sp_mask, canary_offset;
+
+static unsigned int arm_pertask_ssp_rtl_execute(void)
+{
+	rtx_insn *insn;
+
+	for (insn = get_insns(); insn; insn = NEXT_INSN(insn)) {
+		const char *sym;
+		rtx body;
+		rtx masked_sp;
+
+		/*
+		 * Find a SET insn involving a SYMBOL_REF to __stack_chk_guard
+		 */
+		if (!INSN_P(insn))
+			continue;
+		body = PATTERN(insn);
+		if (GET_CODE(body) != SET ||
+		    GET_CODE(SET_SRC(body)) != SYMBOL_REF)
+			continue;
+		sym = XSTR(SET_SRC(body), 0);
+		if (strcmp(sym, "__stack_chk_guard"))
+			continue;
+
+		/*
+		 * Replace the source of the SET insn with an expression that
+		 * produces the address of the copy of the stack canary value
+		 * stored in struct thread_info
+		 */
+		masked_sp = gen_reg_rtx(Pmode);
+
+		emit_insn_before(gen_rtx_SET(masked_sp,
+					     gen_rtx_AND(Pmode,
+							 stack_pointer_rtx,
+							 GEN_INT(sp_mask))),
+				 insn);
+
+		SET_SRC(body) = gen_rtx_PLUS(Pmode, masked_sp,
+					     GEN_INT(canary_offset));
+	}
+	return 0;
+}
+
+#define PASS_NAME arm_pertask_ssp_rtl
+
+#define NO_GATE
+#include "gcc-generate-rtl-pass.h"
+
+__visible int plugin_init(struct plugin_name_args *plugin_info,
+			  struct plugin_gcc_version *version)
+{
+	const char * const plugin_name = plugin_info->base_name;
+	const int argc = plugin_info->argc;
+	const struct plugin_argument *argv = plugin_info->argv;
+	int tso = 0;
+	int i;
+
+	if (!plugin_default_version_check(version, &gcc_version)) {
+		error(G_("incompatible gcc/plugin versions"));
+		return 1;
+	}
+
+	for (i = 0; i < argc; ++i) {
+		if (!strcmp(argv[i].key, "disable"))
+			return 0;
+
+		/* all remaining options require a value */
+		if (!argv[i].value) {
+			error(G_("no value supplied for option '-fplugin-arg-%s-%s'"),
+			      plugin_name, argv[i].key);
+			return 1;
+		}
+
+		if (!strcmp(argv[i].key, "tso")) {
+			tso = atoi(argv[i].value);
+			continue;
+		}
+
+		if (!strcmp(argv[i].key, "offset")) {
+			canary_offset = atoi(argv[i].value);
+			continue;
+		}
+		error(G_("unknown option '-fplugin-arg-%s-%s'"),
+		      plugin_name, argv[i].key);
+		return 1;
+	}
+
+	/* create the mask that produces the base of the stack */
+	sp_mask = ~((1U << (12 + tso)) - 1);
+
+	PASS_INFO(arm_pertask_ssp_rtl, "expand", 1, PASS_POS_INSERT_AFTER);
+
+	register_callback(plugin_info->base_name, PLUGIN_PASS_MANAGER_SETUP,
+			  NULL, &arm_pertask_ssp_rtl_pass_info);
+
+	return 0;
+}