[PATCH] arm64: make STACKPROTECTOR_PER_TASK configurable.

[PATCH] arm64: make STACKPROTECTOR_PER_TASK configurable.

[Michal Suchanek]

When using dummy-tools STACKPROTECTOR_PER_TASK is unconditionally
selected. This defeats the purpose of the all-enabled tool.

Description copied from arm

Cc: Masahiro Yamada <masahiroy@kernel.org>
Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 arch/arm64/Kconfig | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index a8ff7cd5f096..f59d391e31a4 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1549,9 +1549,20 @@ config RANDOMIZE_MODULE_REGION_FULL
 config CC_HAVE_STACKPROTECTOR_SYSREG
 	def_bool $(cc-option,-mstack-protector-guard=sysreg -mstack-protector-guard-reg=sp_el0 -mstack-protector-guard-offset=0)
 
+
 config STACKPROTECTOR_PER_TASK
-	def_bool y
+	bool "Use a unique stack canary value for each task"
 	depends on STACKPROTECTOR && CC_HAVE_STACKPROTECTOR_SYSREG
+	default y
+	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.
 
 endmenu
 
-- 
2.26.2

Re: [PATCH] arm64: make STACKPROTECTOR_PER_TASK configurable.

[Ard Biesheuvel]

On Tue, 9 Mar 2021 at 13:37, Michal Suchanek <msuchanek@suse.de> wrote:
>
> When using dummy-tools STACKPROTECTOR_PER_TASK is unconditionally
> selected. This defeats the purpose of the all-enabled tool.
>

What is dummy-tools and why should we care about it?

> Description copied from arm
>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
>  arch/arm64/Kconfig | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index a8ff7cd5f096..f59d391e31a4 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1549,9 +1549,20 @@ config RANDOMIZE_MODULE_REGION_FULL
>  config CC_HAVE_STACKPROTECTOR_SYSREG
>         def_bool $(cc-option,-mstack-protector-guard=sysreg -mstack-protector-guard-reg=sp_el0 -mstack-protector-guard-offset=0)
>
> +
>  config STACKPROTECTOR_PER_TASK
> -       def_bool y
> +       bool "Use a unique stack canary value for each task"
>         depends on STACKPROTECTOR && CC_HAVE_STACKPROTECTOR_SYSREG
> +       default y
> +       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.
>
>  endmenu
>
> --
> 2.26.2
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: make STACKPROTECTOR_PER_TASK configurable.

[Masahiro Yamada]

On Tue, Mar 9, 2021 at 9:35 PM Michal Suchanek <msuchanek@suse.de> wrote:
>
> When using dummy-tools STACKPROTECTOR_PER_TASK is unconditionally
> selected. This defeats the purpose of the all-enabled tool.
>
> Description copied from arm
>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>


Could you explain what problem
this patch is trying to solve?


> ---
>  arch/arm64/Kconfig | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index a8ff7cd5f096..f59d391e31a4 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1549,9 +1549,20 @@ config RANDOMIZE_MODULE_REGION_FULL
>  config CC_HAVE_STACKPROTECTOR_SYSREG
>         def_bool $(cc-option,-mstack-protector-guard=sysreg -mstack-protector-guard-reg=sp_el0 -mstack-protector-guard-offset=0)
>
> +
>  config STACKPROTECTOR_PER_TASK
> -       def_bool y
> +       bool "Use a unique stack canary value for each task"
>         depends on STACKPROTECTOR && CC_HAVE_STACKPROTECTOR_SYSREG
> +       default y
> +       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.
>
>  endmenu
>
> --
> 2.26.2
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH] arm64: make STACKPROTECTOR_PER_TASK configurable.

[Michal Suchánek]

On Tue, Mar 09, 2021 at 10:22:36PM +0900, Masahiro Yamada wrote:
> On Tue, Mar 9, 2021 at 9:35 PM Michal Suchanek <msuchanek@suse.de> wrote:
> >
> > When using dummy-tools STACKPROTECTOR_PER_TASK is unconditionally
> > selected. This defeats the purpose of the all-enabled tool.
> >
> > Description copied from arm
> >
> > Cc: Masahiro Yamada <masahiroy@kernel.org>
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> 
> 
> Could you explain what problem
> this patch is trying to solve?

The option cannot be disabled when compiler has the required capability.

Thanks

Michal
> 
> 
> > ---
> >  arch/arm64/Kconfig | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index a8ff7cd5f096..f59d391e31a4 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -1549,9 +1549,20 @@ config RANDOMIZE_MODULE_REGION_FULL
> >  config CC_HAVE_STACKPROTECTOR_SYSREG
> >         def_bool $(cc-option,-mstack-protector-guard=sysreg -mstack-protector-guard-reg=sp_el0 -mstack-protector-guard-offset=0)
> >
> > +
> >  config STACKPROTECTOR_PER_TASK
> > -       def_bool y
> > +       bool "Use a unique stack canary value for each task"
> >         depends on STACKPROTECTOR && CC_HAVE_STACKPROTECTOR_SYSREG
> > +       default y
> > +       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.
> >
> >  endmenu
> >
> > --
> > 2.26.2
> >
> 
> 
> -- 
> Best Regards
> Masahiro Yamada

Re: [PATCH] arm64: make STACKPROTECTOR_PER_TASK configurable.

[Masahiro Yamada]

On Tue, Mar 9, 2021 at 10:35 PM Michal Suchánek <msuchanek@suse.de> wrote:
>
> On Tue, Mar 09, 2021 at 10:22:36PM +0900, Masahiro Yamada wrote:
> > On Tue, Mar 9, 2021 at 9:35 PM Michal Suchanek <msuchanek@suse.de> wrote:
> > >
> > > When using dummy-tools STACKPROTECTOR_PER_TASK is unconditionally
> > > selected. This defeats the purpose of the all-enabled tool.
> > >
> > > Description copied from arm
> > >
> > > Cc: Masahiro Yamada <masahiroy@kernel.org>
> > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> >
> >
> > Could you explain what problem
> > this patch is trying to solve?
>
> The option cannot be disabled when compiler has the required capability.


Yes.
Currently, this symbol claims "def_bool y",
so there is no way to disable it.

But, it comes from the nature of Kconfig in general.

dummy-tools is completely unrelated here.







-- 
Best Regards
Masahiro Yamada

Re: [PATCH] arm64: make STACKPROTECTOR_PER_TASK configurable.

[Michal Suchánek]

On Tue, Mar 09, 2021 at 11:53:21PM +0900, Masahiro Yamada wrote:
> On Tue, Mar 9, 2021 at 10:35 PM Michal Suchánek <msuchanek@suse.de> wrote:
> >
> > On Tue, Mar 09, 2021 at 10:22:36PM +0900, Masahiro Yamada wrote:
> > > On Tue, Mar 9, 2021 at 9:35 PM Michal Suchanek <msuchanek@suse.de> wrote:
> > > >
> > > > When using dummy-tools STACKPROTECTOR_PER_TASK is unconditionally
> > > > selected. This defeats the purpose of the all-enabled tool.
> > > >
> > > > Description copied from arm
> > > >
> > > > Cc: Masahiro Yamada <masahiroy@kernel.org>
> > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > >
> > >
> > > Could you explain what problem
> > > this patch is trying to solve?
> >
> > The option cannot be disabled when compiler has the required capability.
> 
> 
> Yes.
> Currently, this symbol claims "def_bool y",
> so there is no way to disable it.
> 
> But, it comes from the nature of Kconfig in general.
> 
> dummy-tools is completely unrelated here.

dummy-tools makes all configuration options available in order to be
able to author configuration files on system different from the one
where the kernel is built. This prevents authoring a configuration file
with this option disabled.

Thanks

Michal

Re: [PATCH] arm64: make STACKPROTECTOR_PER_TASK configurable.

[Masahiro Yamada]

On Wed, Mar 10, 2021 at 12:10 AM Michal Suchánek <msuchanek@suse.de> wrote:
>
> On Tue, Mar 09, 2021 at 11:53:21PM +0900, Masahiro Yamada wrote:
> > On Tue, Mar 9, 2021 at 10:35 PM Michal Suchánek <msuchanek@suse.de> wrote:
> > >
> > > On Tue, Mar 09, 2021 at 10:22:36PM +0900, Masahiro Yamada wrote:
> > > > On Tue, Mar 9, 2021 at 9:35 PM Michal Suchanek <msuchanek@suse.de> wrote:
> > > > >
> > > > > When using dummy-tools STACKPROTECTOR_PER_TASK is unconditionally
> > > > > selected. This defeats the purpose of the all-enabled tool.
> > > > >
> > > > > Description copied from arm
> > > > >
> > > > > Cc: Masahiro Yamada <masahiroy@kernel.org>
> > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > >
> > > >
> > > > Could you explain what problem
> > > > this patch is trying to solve?
> > >
> > > The option cannot be disabled when compiler has the required capability.
> >
> >
> > Yes.
> > Currently, this symbol claims "def_bool y",
> > so there is no way to disable it.
> >
> > But, it comes from the nature of Kconfig in general.
> >
> > dummy-tools is completely unrelated here.
>
> dummy-tools makes all configuration options available in order to be
> able to author configuration files on system different from the one
> where the kernel is built. This prevents authoring a configuration file
> with this option disabled.


No.
dummy-tools enables as many $(cc-option, ...)
and $(shell, ...) as possible. That's it.


In my understanding, STACKPROTECTOR_PER_TASK
should not be user-configurable.
That is why 'def_bool y'.

--
Best Regards
Masahiro Yamada

Re: [PATCH] arm64: make STACKPROTECTOR_PER_TASK configurable.

[Michal Suchánek]

On Wed, Mar 10, 2021 at 04:07:00AM +0900, Masahiro Yamada wrote:
> On Wed, Mar 10, 2021 at 12:10 AM Michal Suchánek <msuchanek@suse.de> wrote:
> >
> > On Tue, Mar 09, 2021 at 11:53:21PM +0900, Masahiro Yamada wrote:
> > > On Tue, Mar 9, 2021 at 10:35 PM Michal Suchánek <msuchanek@suse.de> wrote:
> > > >
> > > > On Tue, Mar 09, 2021 at 10:22:36PM +0900, Masahiro Yamada wrote:
> > > > > On Tue, Mar 9, 2021 at 9:35 PM Michal Suchanek <msuchanek@suse.de> wrote:
> > > > > >
> > > > > > When using dummy-tools STACKPROTECTOR_PER_TASK is unconditionally
> > > > > > selected. This defeats the purpose of the all-enabled tool.
> > > > > >
> > > > > > Description copied from arm
> > > > > >
> > > > > > Cc: Masahiro Yamada <masahiroy@kernel.org>
> > > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > >
> > > > >
> > > > > Could you explain what problem
> > > > > this patch is trying to solve?
> > > >
> > > > The option cannot be disabled when compiler has the required capability.
> > >
> > >
> > > Yes.
> > > Currently, this symbol claims "def_bool y",
> > > so there is no way to disable it.
> > >
> > > But, it comes from the nature of Kconfig in general.
> > >
> > > dummy-tools is completely unrelated here.
> >
> > dummy-tools makes all configuration options available in order to be
> > able to author configuration files on system different from the one
> > where the kernel is built. This prevents authoring a configuration file
> > with this option disabled.
> 
> 
> No.
> dummy-tools enables as many $(cc-option, ...)
> and $(shell, ...) as possible. That's it.
> 
> 
> In my understanding, STACKPROTECTOR_PER_TASK
> should not be user-configurable.
> That is why 'def_bool y'.

How do you author a specific configuration with dummy-tools when options
are allowed to unconditionally follow these options that dummy-tools
enables?

Thanks

Michal

Re: [PATCH] arm64: make STACKPROTECTOR_PER_TASK configurable.

[Michal Suchánek]

On Wed, Mar 10, 2021 at 04:07:00AM +0900, Masahiro Yamada wrote:
> On Wed, Mar 10, 2021 at 12:10 AM Michal Suchánek <msuchanek@suse.de> wrote:
> >
> > On Tue, Mar 09, 2021 at 11:53:21PM +0900, Masahiro Yamada wrote:
> > > On Tue, Mar 9, 2021 at 10:35 PM Michal Suchánek <msuchanek@suse.de> wrote:
> > > >
> > > > On Tue, Mar 09, 2021 at 10:22:36PM +0900, Masahiro Yamada wrote:
> > > > > On Tue, Mar 9, 2021 at 9:35 PM Michal Suchanek <msuchanek@suse.de> wrote:
> > > > > >
> > > > > > When using dummy-tools STACKPROTECTOR_PER_TASK is unconditionally
> > > > > > selected. This defeats the purpose of the all-enabled tool.
> > > > > >
> > > > > > Description copied from arm
> > > > > >
> > > > > > Cc: Masahiro Yamada <masahiroy@kernel.org>
> > > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > >
> > > > >
> > > > > Could you explain what problem
> > > > > this patch is trying to solve?
> > > >
> > > > The option cannot be disabled when compiler has the required capability.
> > >
> > >
> > > Yes.
> > > Currently, this symbol claims "def_bool y",
> > > so there is no way to disable it.
> > >
> > > But, it comes from the nature of Kconfig in general.
> > >
> > > dummy-tools is completely unrelated here.
> >
> > dummy-tools makes all configuration options available in order to be
> > able to author configuration files on system different from the one
> > where the kernel is built. This prevents authoring a configuration file
> > with this option disabled.
> 
> 
> No.
> dummy-tools enables as many $(cc-option, ...)
> and $(shell, ...) as possible. That's it.
> 
> 
> In my understanding, STACKPROTECTOR_PER_TASK
> should not be user-configurable.
> That is why 'def_bool y'.

And that's wrong. Either it's a required copiler feature and then it
should be always enabled. Or it is optional and then it should be
possible to disable - one or the other.

When it is optional you should be able to

 - (de) select in randconfig
 - test/bisect the old code path without crippling your toolchain
 - author configuration file that does not have the option enabled for
   build on system that uses older toolchain

Thanks

Michal