2021-03-09 12:38:11

by Michal Suchánek

[permalink] [raw]
Subject: [PATCH] arm64: make STACKPROTECTOR_PER_TASK configurable.

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 <[email protected]>
Signed-off-by: Michal Suchanek <[email protected]>
---
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


2021-03-09 12:43:00

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [PATCH] arm64: make STACKPROTECTOR_PER_TASK configurable.

On Tue, 9 Mar 2021 at 13:37, Michal Suchanek <[email protected]> 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 <[email protected]>
> Signed-off-by: Michal Suchanek <[email protected]>
> ---
> 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2021-03-09 13:25:26

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [PATCH] arm64: make STACKPROTECTOR_PER_TASK configurable.

On Tue, Mar 9, 2021 at 9:35 PM Michal Suchanek <[email protected]> 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 <[email protected]>
> Signed-off-by: Michal Suchanek <[email protected]>


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

2021-03-09 13:37:34

by Michal Suchánek

[permalink] [raw]
Subject: Re: [PATCH] arm64: make STACKPROTECTOR_PER_TASK configurable.

On Tue, Mar 09, 2021 at 10:22:36PM +0900, Masahiro Yamada wrote:
> On Tue, Mar 9, 2021 at 9:35 PM Michal Suchanek <[email protected]> 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 <[email protected]>
> > Signed-off-by: Michal Suchanek <[email protected]>
>
>
> 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

2021-03-09 14:56:12

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [PATCH] arm64: make STACKPROTECTOR_PER_TASK configurable.

On Tue, Mar 9, 2021 at 10:35 PM Michal Suchánek <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > Signed-off-by: Michal Suchanek <[email protected]>
> >
> >
> > 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

2021-03-09 15:13:35

by Michal Suchánek

[permalink] [raw]
Subject: Re: [PATCH] arm64: make STACKPROTECTOR_PER_TASK configurable.

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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > Signed-off-by: Michal Suchanek <[email protected]>
> > >
> > >
> > > 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

2021-03-09 19:09:55

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [PATCH] arm64: make STACKPROTECTOR_PER_TASK configurable.

On Wed, Mar 10, 2021 at 12:10 AM Michal Suchánek <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > Signed-off-by: Michal Suchanek <[email protected]>
> > > >
> > > >
> > > > 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

2021-03-09 19:45:54

by Michal Suchánek

[permalink] [raw]
Subject: Re: [PATCH] arm64: make STACKPROTECTOR_PER_TASK configurable.

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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > Signed-off-by: Michal Suchanek <[email protected]>
> > > > >
> > > > >
> > > > > 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

2021-03-10 16:28:03

by Michal Suchánek

[permalink] [raw]
Subject: Re: [PATCH] arm64: make STACKPROTECTOR_PER_TASK configurable.

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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > Signed-off-by: Michal Suchanek <[email protected]>
> > > > >
> > > > >
> > > > > 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