[GIT,PULL] Crypto Fixes for 5.6

1208802 mbox series

Hi Linus:

This push fixes a build problem with x86/curve25519.

The following changes since commit c9cc0517bba9f0213f1e55172feceb99e5512daf:

  crypto: chacha20poly1305 - prevent integer overflow on large input (2020-02-14 14:48:37 +0800)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git linus 

for you to fetch changes up to 1579f1bc3b753d17a44de3457d5c6f4a5b14c752:

  crypto: x86/curve25519 - support assemblers with no adx support (2020-03-05 18:28:09 +1100)

----------------------------------------------------------------
Jason A. Donenfeld (1):
      crypto: x86/curve25519 - support assemblers with no adx support

 arch/x86/Makefile           | 5 +++--
 arch/x86/crypto/Makefile    | 7 ++++++-
 include/crypto/curve25519.h | 6 ++++--
 3 files changed, 13 insertions(+), 5 deletions(-)

Thanks,

On Thu, Mar 12, 2020 at 4:57 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> This push fixes a build problem with x86/curve25519.

Pulled.

I do have a comment, though: this fix matches the existing pattern of
checking for assembler support, but that existing pattern is
absolutely horrible.

Would some enterprising individual please look at making the
CONFIG_AS_xyz flags use the _real_ config subsystem rather than ad-hoc
Makefile rules?

IOW, instead of having

  adx_instr := $(call as-instr,adox %r10$(comma)%r10,-DCONFIG_AS_ADX=1)
  ..
  adx_supported := $(call as-instr,adox %r10$(comma)%r10,yes,no)

in the makefiles, and silently changing how the Kconfig variables work
depending on those flags, make that DCONFIG_AS_ADX be a real config
variable:

   config AS_ADX
           def_bool $(success,$(srctree)/scripts/as-instr.sh "adox %r10,%r10")

or something like that?

And then we can make that CRYPTO_CURVE25519_X86 config variable simply have a

        depends on AS_ADX

in it, and the Kconfig system just takes care of these dependencies on its own.

Anyway, the crypto change isn't _wrong_, but it does point out an ugly
little horror in how the crypto layer silently basically changes the
configuration depending on other things.

For an example of why this is problematic: it means that if somebody
sends you their config file, the actual configuration you get may be
*completely* different from what they actually had, depending on
tools.

Added Masahiro to the cc, since he's used to the 'def_bool' model, and
also is familiar with our existing 'as-instr' Makefile macro.

So this is basically me throwing out a "I wish somebody would look at
this". Not meant as a criticism of the commit in question.

            Linus

The pull request you sent on Thu, 12 Mar 2020 22:57:14 +1100:

> git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git linus

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/2644bc8569baa735ae9c0a92432d6a30c20c1694

Thank you!

Hi Linus,

On Fri, Mar 13, 2020 at 1:41 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, Mar 12, 2020 at 4:57 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> >
> > This push fixes a build problem with x86/curve25519.
>
> Pulled.
>
> I do have a comment, though: this fix matches the existing pattern of
> checking for assembler support, but that existing pattern is
> absolutely horrible.
>
> Would some enterprising individual please look at making the
> CONFIG_AS_xyz flags use the _real_ config subsystem rather than ad-hoc
> Makefile rules?
>
> IOW, instead of having
>
>   adx_instr := $(call as-instr,adox %r10$(comma)%r10,-DCONFIG_AS_ADX=1)
>   ..
>   adx_supported := $(call as-instr,adox %r10$(comma)%r10,yes,no)
>
> in the makefiles, and silently changing how the Kconfig variables work
> depending on those flags, make that DCONFIG_AS_ADX be a real config
> variable:
>
>    config AS_ADX
>            def_bool $(success,$(srctree)/scripts/as-instr.sh "adox %r10,%r10")
>
> or something like that?
>
> And then we can make that CRYPTO_CURVE25519_X86 config variable simply have a
>
>         depends on AS_ADX
>
> in it, and the Kconfig system just takes care of these dependencies on its own.
>
> Anyway, the crypto change isn't _wrong_, but it does point out an ugly
> little horror in how the crypto layer silently basically changes the
> configuration depending on other things.
>
> For an example of why this is problematic: it means that if somebody
> sends you their config file, the actual configuration you get may be
> *completely* different from what they actually had, depending on
> tools.
>
> Added Masahiro to the cc, since he's used to the 'def_bool' model, and
> also is familiar with our existing 'as-instr' Makefile macro.


Thanks for the heads-up.

In fact, as-instr is already used in Kconfig.
arch/arm64/Kconfig: line 1396


arm / arm64 are simple cases because
32, 64-bit is separated by directory.

There is one thing we need to be careful about.
The x86 GCC is usually biarch.
So, when evaluating 64-bit assembly code
with a default 32-bit compiler,
-m64 must be passed.

I will keep this conversion in my mind.

Thanks.

> So this is basically me throwing out a "I wish somebody would look at
> this". Not meant as a criticism of the commit in question.
>
>             Linus

Funny, I always thought it was like that "for a good reason" that I
just didn't know about -- assumedly something having to do with a
difference between config time and compile time. I agree with you that
everything gets so much cleaner if we can do this in Kconfig. I've put
together the patch pasted below, which appears to work well. I'll work
on replumbing the other stuff and will send a series off to the list
hopefully not before too long.

From 12375354ddb4c8b1c75663312a9b6d9b9bc5f520 Mon Sep 17 00:00:00 2001
From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Fri, 20 Mar 2020 17:49:36 -0600
Subject: [PATCH] x86: probe assembler instead of kconfig instead of makefile

Doing this probing inside of the Makefiles means we have a maze of
ifdefs inside the source code and child Makefiles that need to make
proper decisions on this too. Instead, we do it at Kconfig time, like
many other compiler and assembler options, which allows us to set up the
dependencies normally for full compilation units.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 arch/x86/Kconfig           |  2 ++
 arch/x86/Kconfig.assembler | 33 +++++++++++++++++++++++++++++++++
 arch/x86/Makefile          | 22 ----------------------
 3 files changed, 35 insertions(+), 22 deletions(-)
 create mode 100644 arch/x86/Kconfig.assembler

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index beea77046f9b..707673227837 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2935,3 +2935,5 @@ config HAVE_ATOMIC_IOMAP
 source "drivers/firmware/Kconfig"

 source "arch/x86/kvm/Kconfig"
+
+source "arch/x86/Kconfig.assembler"
diff --git a/arch/x86/Kconfig.assembler b/arch/x86/Kconfig.assembler
new file mode 100644
index 000000000000..809adcf6f7c3
--- /dev/null
+++ b/arch/x86/Kconfig.assembler
@@ -0,0 +1,33 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2020 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
+
+config AS_CFI
+ def_bool $(as-instr,.cfi_startproc\n.cfi_rel_offset
rsp$(comma)0\n.cfi_endproc) if 64BIT
+ def_bool $(as-instr,.cfi_startproc\n.cfi_rel_offset
esp$(comma)0\n.cfi_endproc) if !64BIT
+
+config AS_CFI_SIGNAL_FRAME
+ def_bool $(as-instr,.cfi_startproc\n.cfi_signal_frame\n.cfi_endproc)
+
+config AS_CFI_SECTIONS
+ def_bool $(as-instr,.cfi_sections .debug_frame)
+
+config AS_SSSE3
+ def_bool $(as-instr,pshufb %xmm0$(comma)%xmm0)
+
+config AS_AVX
+ def_bool $(as-instr,vxorps %ymm0$(comma)%ymm1$(comma)%ymm2)
+
+config AS_AVX2
+ def_bool $(as-instr,vpbroadcastb %xmm0$(comma)%ymm1)
+
+config AS_AVX512
+ def_bool $(as-instr,vpmovm2b %k1$(comma)%zmm5)
+
+config AS_SHA1_NI
+ def_bool $(as-instr,sha1msg1 %xmm0$(comma)%xmm1)
+
+config AS_SHA256_NI
+ def_bool $(as-instr,sha256msg1 %xmm0$(comma)%xmm1)
+
+config AS_ADX
+ def_bool $(as-instr,adox %r10$(comma)%r10)
diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 513a55562d75..b65ec63c7db7 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -177,28 +177,6 @@ ifeq ($(ACCUMULATE_OUTGOING_ARGS), 1)
  KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args,)
 endif

-# Stackpointer is addressed different for 32 bit and 64 bit x86
-sp-$(CONFIG_X86_32) := esp
-sp-$(CONFIG_X86_64) := rsp
-
-# do binutils support CFI?
-cfi := $(call as-instr,.cfi_startproc\n.cfi_rel_offset
$(sp-y)$(comma)0\n.cfi_endproc,-DCONFIG_AS_CFI=1)
-# is .cfi_signal_frame supported too?
-cfi-sigframe := $(call
as-instr,.cfi_startproc\n.cfi_signal_frame\n.cfi_endproc,-DCONFIG_AS_CFI_SIGNAL_FRAME=1)
-cfi-sections := $(call as-instr,.cfi_sections
.debug_frame,-DCONFIG_AS_CFI_SECTIONS=1)
-
-# does binutils support specific instructions?
-asinstr += $(call as-instr,pshufb %xmm0$(comma)%xmm0,-DCONFIG_AS_SSSE3=1)
-avx_instr := $(call as-instr,vxorps
%ymm0$(comma)%ymm1$(comma)%ymm2,-DCONFIG_AS_AVX=1)
-avx2_instr :=$(call as-instr,vpbroadcastb
%xmm0$(comma)%ymm1,-DCONFIG_AS_AVX2=1)
-avx512_instr :=$(call as-instr,vpmovm2b %k1$(comma)%zmm5,-DCONFIG_AS_AVX512=1)
-sha1_ni_instr :=$(call as-instr,sha1msg1
%xmm0$(comma)%xmm1,-DCONFIG_AS_SHA1_NI=1)
-sha256_ni_instr :=$(call as-instr,sha256msg1
%xmm0$(comma)%xmm1,-DCONFIG_AS_SHA256_NI=1)
-adx_instr := $(call as-instr,adox %r10$(comma)%r10,-DCONFIG_AS_ADX=1)
-
-KBUILD_AFLAGS += $(cfi) $(cfi-sigframe) $(cfi-sections) $(asinstr)
$(avx_instr) $(avx2_instr) $(avx512_instr) $(sha1_ni_instr)
$(sha256_ni_instr) $(adx_instr)
-KBUILD_CFLAGS += $(cfi) $(cfi-sigframe) $(cfi-sections) $(asinstr)
$(avx_instr) $(avx2_instr) $(avx512_instr) $(sha1_ni_instr)
$(sha256_ni_instr) $(adx_instr)
-
 KBUILD_LDFLAGS := -m elf_$(UTS_MACHINE)

 #

On Fri, Mar 20, 2020 at 4:54 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Funny, I always thought it was like that "for a good reason" that I
> just didn't know about -- assumedly something having to do with a
> difference between config time and compile time.

No, there _is_ a "good reason", but it is simply "hysterical raisins".

All scripting used to be done in Makefiles, for the simple reason that
GNU make supported all those shell escapes. The Kconfig language did
not.

The whole "shell escape in Kconfig" is relatively recent, and so we
still have old code (and people) used to the build-time makefile rules
rather than those newfangled Kconfig things.

Of course "relatively recent" is about two years by now. It's not like
we did it yesterday.

Anyway, your conversion patches look fine to me. I'm obviously not
taking them for 5.6, but if they go into -next and get some testing,
I'd love to have that cleanup in 5.7.

                 Linus