Since commit 18e66b695e78 ("x86/shstk: Add Kconfig option for shadow
stack"), AS_WRUSS is set even in 32-bit .configs. It is due to how
Kbuild works. .config is not considered during make oldconfig (and other
make *config), so standard (64-bit) gcc is invoked from 'as-instr'
Kbuild tests. And such gcc indeed reports that wruss is supported, so
AS_WRUSS=y is set.
Provided the wruss instruction is 64-bit only (and used in pure 64-bit
X86_USER_SHADOW_STACK), it has little sense to have AS_WRUSS=y set on
32-bit.
Therefore, make the whole test dependent on X86_64 to ensure it's set
only on 64-bit.
Signed-off-by: Jiri Slaby (SUSE) <[email protected]>
Cc: Yu-cheng Yu <[email protected]>
Cc: Rick Edgecombe <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Borislav Petkov (AMD) <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Mike Rapoport (IBM) <[email protected]>
Cc: Pengfei Xu <[email protected]>
Cc: John Allen <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: [email protected]
---
arch/x86/Kconfig.assembler | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/Kconfig.assembler b/arch/x86/Kconfig.assembler
index 8ad41da301e5..a5b5241711e3 100644
--- a/arch/x86/Kconfig.assembler
+++ b/arch/x86/Kconfig.assembler
@@ -27,5 +27,6 @@ config AS_GFNI
config AS_WRUSS
def_bool $(as-instr,wrussq %rax$(comma)(%rbx))
+ depends on X86_64
help
Supported by binutils >= 2.31 and LLVM integrated assembler
--
2.42.0
+ [email protected]
On Tue, Oct 31, 2023 at 11:21:11AM +0100, Jiri Slaby (SUSE) wrote:
> Since commit 18e66b695e78 ("x86/shstk: Add Kconfig option for shadow
> stack"), AS_WRUSS is set even in 32-bit .configs. It is due to how
> Kbuild works. .config is not considered during make oldconfig (and other
> make *config), so standard (64-bit) gcc is invoked from 'as-instr'
> Kbuild tests. And such gcc indeed reports that wruss is supported, so
> AS_WRUSS=y is set.
>
> Provided the wruss instruction is 64-bit only (and used in pure 64-bit
> X86_USER_SHADOW_STACK), it has little sense to have AS_WRUSS=y set on
> 32-bit.
>
> Therefore, make the whole test dependent on X86_64 to ensure it's set
> only on 64-bit.
>
> Signed-off-by: Jiri Slaby (SUSE) <[email protected]>
> Cc: Yu-cheng Yu <[email protected]>
> Cc: Rick Edgecombe <[email protected]>
> Cc: Dave Hansen <[email protected]>
> Cc: Borislav Petkov (AMD) <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Mike Rapoport (IBM) <[email protected]>
> Cc: Pengfei Xu <[email protected]>
> Cc: John Allen <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: [email protected]
> ---
> arch/x86/Kconfig.assembler | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/Kconfig.assembler b/arch/x86/Kconfig.assembler
> index 8ad41da301e5..a5b5241711e3 100644
> --- a/arch/x86/Kconfig.assembler
> +++ b/arch/x86/Kconfig.assembler
> @@ -27,5 +27,6 @@ config AS_GFNI
>
> config AS_WRUSS
> def_bool $(as-instr,wrussq %rax$(comma)(%rbx))
> + depends on X86_64
> help
> Supported by binutils >= 2.31 and LLVM integrated assembler
> --
> 2.42.0
>
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Oct 31, 2023 at 8:26 PM Borislav Petkov <[email protected]> wrote:
>
> + [email protected]
>
> On Tue, Oct 31, 2023 at 11:21:11AM +0100, Jiri Slaby (SUSE) wrote:
> > Since commit 18e66b695e78 ("x86/shstk: Add Kconfig option for shadow
> > stack"), AS_WRUSS is set even in 32-bit .configs. It is due to how
> > Kbuild works. .config is not considered during make oldconfig (and other
> > make *config), so standard (64-bit) gcc is invoked from 'as-instr'
> > Kbuild tests.
I do not mind either way.
Please note "depends on X86_64" cannot prevent gcc
from running here.
$(as-instr,wrussq %rax$(comma)(%rbx)) is replaced with 'y'
while parsing the Kconfig files.
I want to change it in the future, but that is how Kconfig works now.
You don't save the cost of running the compiler.
> And such gcc indeed reports that wruss is supported, so
> > AS_WRUSS=y is set.
> >
> > Provided the wruss instruction is 64-bit only (and used in pure 64-bit
> > X86_USER_SHADOW_STACK), it has little sense to have AS_WRUSS=y set on
> > 32-bit.
> >
> > Therefore, make the whole test dependent on X86_64 to ensure it's set
> > only on 64-bit.
> >
> > Signed-off-by: Jiri Slaby (SUSE) <[email protected]>
> > Cc: Yu-cheng Yu <[email protected]>
> > Cc: Rick Edgecombe <[email protected]>
> > Cc: Dave Hansen <[email protected]>
> > Cc: Borislav Petkov (AMD) <[email protected]>
> > Cc: Kees Cook <[email protected]>
> > Cc: Mike Rapoport (IBM) <[email protected]>
> > Cc: Pengfei Xu <[email protected]>
> > Cc: John Allen <[email protected]>
> > Cc: Kees Cook <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Ingo Molnar <[email protected]>
> > Cc: "H. Peter Anvin" <[email protected]>
> > Cc: [email protected]
> > ---
> > arch/x86/Kconfig.assembler | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/x86/Kconfig.assembler b/arch/x86/Kconfig.assembler
> > index 8ad41da301e5..a5b5241711e3 100644
> > --- a/arch/x86/Kconfig.assembler
> > +++ b/arch/x86/Kconfig.assembler
> > @@ -27,5 +27,6 @@ config AS_GFNI
> >
> > config AS_WRUSS
> > def_bool $(as-instr,wrussq %rax$(comma)(%rbx))
> > + depends on X86_64
> > help
> > Supported by binutils >= 2.31 and LLVM integrated assembler
> > --
> > 2.42.0
> >
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
--
Best Regards
Masahiro Yamada
On Tue, Oct 31, 2023 at 09:20:33PM +0900, Masahiro Yamada wrote:
> Please note "depends on X86_64" cannot prevent gcc
> from running here.
>
> $(as-instr,wrussq %rax$(comma)(%rbx)) is replaced with 'y'
> while parsing the Kconfig files.
>
Then what is this fixing?
Jiri, what openSUSE kernel build issues were you referring to on IRC?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 10/31/23 03:21, Jiri Slaby (SUSE) wrote:
...
> Provided the wruss instruction is 64-bit only (and used in pure 64-bit
> X86_USER_SHADOW_STACK), it has little sense to have AS_WRUSS=y set on
> 32-bit.
>
> Therefore, make the whole test dependent on X86_64 to ensure it's set
> only on 64-bit.
...
> config AS_WRUSS
> def_bool $(as-instr,wrussq %rax$(comma)(%rbx))
> + depends on X86_64
> help
> Supported by binutils >= 2.31 and LLVM integrated assembler
What's the downside to just leaving this alone?
This patch just seems wrong logically. Suppose some deranged person
wanted 32-bit shadow stack support. They'd have to go hunt this down
via trial and error instead of just enabling X86_USER_SHADOW_STACK.
Granted, that would take one crazy person five minutes to figure out why
their .config is broken, but it still seems wrong. It's especially
wrong without a comment because it logically reads something along the
lines of "WRUSS is only available on x86_64 configs".
A better way to do this would be:
config HAS_SHADOW_STACKS
depends on X86_64
config AS_WRUSS
...
# Avoid setting AS_WRUSS on configs that don't need it:
depends on HAS_SHADOW_STACKS
config X86_USER_SHADOW_STACK
bool "X86 userspace shadow stack"
depends on AS_WRUSS
depends on HAS_SHADOW_STACKS
But that honestly doesn't seem worth it because (circling back to the
first thing I wrote...) I don't really know what the benefit is to doing
this in the first place.
On 31. 10. 23, 14:34, Borislav Petkov wrote:
> On Tue, Oct 31, 2023 at 09:20:33PM +0900, Masahiro Yamada wrote:
>> Please note "depends on X86_64" cannot prevent gcc
>> from running here.
>>
>> $(as-instr,wrussq %rax$(comma)(%rbx)) is replaced with 'y'
>> while parsing the Kconfig files.
>>
> Then what is this fixing?
> > Jiri, what openSUSE kernel build issues were you referring to on IRC?
As I wrote, we have a config which maintainers feed through oldconfig
when updating the kernel. This time we got WRUSS=y. It was stored in git
and later passed to build the kernel. And we received a different config
from the build (WRUSS=n).
We diff the two and error out if the built kernel differs from the
passed configuration. Which is the case here.
hth,
--
js
On Tue, Oct 31, 2023 at 02:59:39PM +0100, Jiri Slaby wrote:
> As I wrote, we have a config which maintainers feed through oldconfig when
> updating the kernel. This time we got WRUSS=y. It was stored in git and
> later passed to build the kernel. And we received a different config from
> the build (WRUSS=n).
>
> We diff the two and error out if the built kernel differs from the passed
> configuration. Which is the case here.
I sure hope that diffing script can ignore Kconfig items. Because this
doesn't sound like a reason to "fix" the upstream kernel and as Masahiro
says, it doesn't change anything anyway.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 31. 10. 23, 14:53, Dave Hansen wrote:
> On 10/31/23 03:21, Jiri Slaby (SUSE) wrote:
> ...
>> Provided the wruss instruction is 64-bit only (and used in pure 64-bit
>> X86_USER_SHADOW_STACK), it has little sense to have AS_WRUSS=y set on
>> 32-bit.
>>
>> Therefore, make the whole test dependent on X86_64 to ensure it's set
>> only on 64-bit.
> ...
>> config AS_WRUSS
>> def_bool $(as-instr,wrussq %rax$(comma)(%rbx))
>> + depends on X86_64
>> help
>> Supported by binutils >= 2.31 and LLVM integrated assembler
>
> What's the downside to just leaving this alone?
>
> This patch just seems wrong logically. Suppose some deranged person
> wanted 32-bit shadow stack support. They'd have to go hunt this down
> via trial and error instead of just enabling X86_USER_SHADOW_STACK.
All wrussq, rax, rbx can never be right on 32-bit anyway...
> Granted, that would take one crazy person five minutes to figure out why
> their .config is broken, but it still seems wrong. It's especially
> wrong without a comment because it logically reads something along the
> lines of "WRUSS is only available on x86_64 configs".
Which is right, or what am I missing?
> A better way to do this would be:
>
> config HAS_SHADOW_STACKS
> depends on X86_64
>
> config AS_WRUSS
> ...
> # Avoid setting AS_WRUSS on configs that don't need it:
> depends on HAS_SHADOW_STACKS
>
> config X86_USER_SHADOW_STACK
> bool "X86 userspace shadow stack"
> depends on AS_WRUSS
> depends on HAS_SHADOW_STACKS
>
> But that honestly doesn't seem worth it because (circling back to the
> first thing I wrote...) I don't really know what the benefit is to doing
> this in the first place.
Again, to avoid nonsense in 32bit configs produced by oldconfig (as I
noted in the commit log).
thanks,
--
js
suse labs
On 31. 10. 23, 15:05, Borislav Petkov wrote:
> On Tue, Oct 31, 2023 at 02:59:39PM +0100, Jiri Slaby wrote:
>> As I wrote, we have a config which maintainers feed through oldconfig when
>> updating the kernel. This time we got WRUSS=y. It was stored in git and
>> later passed to build the kernel. And we received a different config from
>> the build (WRUSS=n).
>>
>> We diff the two and error out if the built kernel differs from the passed
>> configuration. Which is the case here.
>
> I sure hope that diffing script can ignore Kconfig items.
Definitely, I can add an ignore on that level.
> Because this
> doesn't sound like a reason to "fix" the upstream kernel and as Masahiro
> says, it doesn't change anything anyway.
It changes what's in .config and corresponds 1:1 to what is used during
the build.
Now, I have a .config from "make oldconfig". But "make all" will read it
and flips the value. (That is IMHO misleading, but I can live with that,
provided I now know. Not sure about others though.)
thanks,
--
js
suse labs