[PATCH v2 0/3] crypto: lib - create utils module

[PATCH v2 0/3] crypto: lib - create utils module

[Eric Biggers]

Create a utils module in lib/crypto/, and move __crypto_memneq,
__crypto_xor, and crypto_simd_disabled_for_test into it.

This supersedes "crypto: xor - move __crypto_xor into lib/"
(https://lore.kernel.org/linux-crypto/20220709215453.262237-1-ebiggers@kernel.org/T/#u).

Eric Biggers (3):
  crypto: lib - create utils module and move __crypto_memneq into it
  crypto: lib - move __crypto_xor into utils
  crypto: lib - move crypto_simd_disabled_for_test into utils

 crypto/Kconfig            |  2 +-
 crypto/algapi.c           | 77 ---------------------------------
 lib/Kconfig               |  3 --
 lib/Makefile              |  1 -
 lib/crypto/Kconfig        |  8 ++--
 lib/crypto/Makefile       |  3 ++
 lib/{ => crypto}/memneq.c |  0
 lib/crypto/utils.c        | 91 +++++++++++++++++++++++++++++++++++++++
 8 files changed, 100 insertions(+), 85 deletions(-)
 rename lib/{ => crypto}/memneq.c (100%)
 create mode 100644 lib/crypto/utils.c


base-commit: ec8f7f4821d5e70d71601519bc2325b311324a96
-- 
2.37.0

[PATCH v2 1/3] crypto: lib - create utils module and move __crypto_memneq into it

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

As requested at
https://lore.kernel.org/r/YtEgzHuuMts0YBCz@gondor.apana.org.au, move
__crypto_memneq into lib/crypto/ and put it under a new tristate.  The
tristate is CRYPTO_LIB_UTILS, and it builds a module libcryptoutils.  As
more crypto library utilities are being added, this creates a single
place for them to go without cluttering up the main lib directory.

The module's main file will be lib/crypto/utils.c.  However, leave
memneq.c as its own file because of its nonstandard license.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/Kconfig            | 2 +-
 lib/Kconfig               | 3 ---
 lib/Makefile              | 1 -
 lib/crypto/Kconfig        | 5 ++++-
 lib/crypto/Makefile       | 3 +++
 lib/{ => crypto}/memneq.c | 0
 6 files changed, 8 insertions(+), 6 deletions(-)
 rename lib/{ => crypto}/memneq.c (100%)

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 75c71d9a5ffb7e..dc7c33f4ee6d55 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -15,7 +15,7 @@ source "crypto/async_tx/Kconfig"
 #
 menuconfig CRYPTO
 	tristate "Cryptographic API"
-	select LIB_MEMNEQ
+	select CRYPTO_LIB_UTILS
 	help
 	  This option provides the core Cryptographic API.
 
diff --git a/lib/Kconfig b/lib/Kconfig
index eaaad4d85bf24b..6a843639814fbf 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -120,9 +120,6 @@ config INDIRECT_IOMEM_FALLBACK
 
 source "lib/crypto/Kconfig"
 
-config LIB_MEMNEQ
-	bool
-
 config CRC_CCITT
 	tristate "CRC-CCITT functions"
 	help
diff --git a/lib/Makefile b/lib/Makefile
index 67482f5ec0e899..0557be76c2565f 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -251,7 +251,6 @@ obj-$(CONFIG_DIMLIB) += dim/
 obj-$(CONFIG_SIGNATURE) += digsig.o
 
 lib-$(CONFIG_CLZ_TAB) += clz_tab.o
-lib-$(CONFIG_LIB_MEMNEQ) += memneq.o
 
 obj-$(CONFIG_GENERIC_STRNCPY_FROM_USER) += strncpy_from_user.o
 obj-$(CONFIG_GENERIC_STRNLEN_USER) += strnlen_user.o
diff --git a/lib/crypto/Kconfig b/lib/crypto/Kconfig
index 9ff549f63540fa..b09d9d6546cbc3 100644
--- a/lib/crypto/Kconfig
+++ b/lib/crypto/Kconfig
@@ -2,6 +2,9 @@
 
 menu "Crypto library routines"
 
+config CRYPTO_LIB_UTILS
+	tristate
+
 config CRYPTO_LIB_AES
 	tristate
 
@@ -71,7 +74,7 @@ config CRYPTO_LIB_CURVE25519
 	tristate "Curve25519 scalar multiplication library"
 	depends on CRYPTO_ARCH_HAVE_LIB_CURVE25519 || !CRYPTO_ARCH_HAVE_LIB_CURVE25519
 	select CRYPTO_LIB_CURVE25519_GENERIC if CRYPTO_ARCH_HAVE_LIB_CURVE25519=n
-	select LIB_MEMNEQ
+	select CRYPTO_LIB_UTILS
 	help
 	  Enable the Curve25519 library interface. This interface may be
 	  fulfilled by either the generic implementation or an arch-specific
diff --git a/lib/crypto/Makefile b/lib/crypto/Makefile
index 919cbb2c220d61..b956b3bae26aaf 100644
--- a/lib/crypto/Makefile
+++ b/lib/crypto/Makefile
@@ -1,5 +1,8 @@
 # SPDX-License-Identifier: GPL-2.0
 
+obj-$(CONFIG_CRYPTO_LIB_UTILS)			+= libcryptoutils.o
+libcryptoutils-y				:= memneq.o
+
 # chacha is used by the /dev/random driver which is always builtin
 obj-y						+= chacha.o
 obj-$(CONFIG_CRYPTO_LIB_CHACHA_GENERIC)		+= libchacha.o
diff --git a/lib/memneq.c b/lib/crypto/memneq.c
similarity index 100%
rename from lib/memneq.c
rename to lib/crypto/memneq.c
-- 
2.37.0

[PATCH v2 2/3] crypto: lib - move __crypto_xor into utils

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

CRYPTO_LIB_CHACHA depends on CRYPTO for __crypto_xor, defined in
crypto/algapi.c.  This is a layering violation because the dependencies
should only go in the other direction (crypto/ => lib/crypto/).  Also
the correct dependency would be CRYPTO_ALGAPI, not CRYPTO.  Fix this by
moving __crypto_xor into the utils module in lib/crypto/.

Note that CRYPTO_LIB_CHACHA_GENERIC selected XOR_BLOCKS, which is
unrelated and unnecessary.  It was perhaps thought that XOR_BLOCKS was
needed for __crypto_xor, but that's not the case.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/algapi.c     | 71 -------------------------------------
 lib/crypto/Kconfig  |  3 +-
 lib/crypto/Makefile |  2 +-
 lib/crypto/utils.c  | 85 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 87 insertions(+), 74 deletions(-)
 create mode 100644 lib/crypto/utils.c

diff --git a/crypto/algapi.c b/crypto/algapi.c
index d1c99288af3e0d..5c69ff8e8fa5c1 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -997,77 +997,6 @@ void crypto_inc(u8 *a, unsigned int size)
 }
 EXPORT_SYMBOL_GPL(crypto_inc);
 
-void __crypto_xor(u8 *dst, const u8 *src1, const u8 *src2, unsigned int len)
-{
-	int relalign = 0;
-
-	if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) {
-		int size = sizeof(unsigned long);
-		int d = (((unsigned long)dst ^ (unsigned long)src1) |
-			 ((unsigned long)dst ^ (unsigned long)src2)) &
-			(size - 1);
-
-		relalign = d ? 1 << __ffs(d) : size;
-
-		/*
-		 * If we care about alignment, process as many bytes as
-		 * needed to advance dst and src to values whose alignments
-		 * equal their relative alignment. This will allow us to
-		 * process the remainder of the input using optimal strides.
-		 */
-		while (((unsigned long)dst & (relalign - 1)) && len > 0) {
-			*dst++ = *src1++ ^ *src2++;
-			len--;
-		}
-	}
-
-	while (IS_ENABLED(CONFIG_64BIT) && len >= 8 && !(relalign & 7)) {
-		if (IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) {
-			u64 l = get_unaligned((u64 *)src1) ^
-				get_unaligned((u64 *)src2);
-			put_unaligned(l, (u64 *)dst);
-		} else {
-			*(u64 *)dst = *(u64 *)src1 ^ *(u64 *)src2;
-		}
-		dst += 8;
-		src1 += 8;
-		src2 += 8;
-		len -= 8;
-	}
-
-	while (len >= 4 && !(relalign & 3)) {
-		if (IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) {
-			u32 l = get_unaligned((u32 *)src1) ^
-				get_unaligned((u32 *)src2);
-			put_unaligned(l, (u32 *)dst);
-		} else {
-			*(u32 *)dst = *(u32 *)src1 ^ *(u32 *)src2;
-		}
-		dst += 4;
-		src1 += 4;
-		src2 += 4;
-		len -= 4;
-	}
-
-	while (len >= 2 && !(relalign & 1)) {
-		if (IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) {
-			u16 l = get_unaligned((u16 *)src1) ^
-				get_unaligned((u16 *)src2);
-			put_unaligned(l, (u16 *)dst);
-		} else {
-			*(u16 *)dst = *(u16 *)src1 ^ *(u16 *)src2;
-		}
-		dst += 2;
-		src1 += 2;
-		src2 += 2;
-		len -= 2;
-	}
-
-	while (len--)
-		*dst++ = *src1++ ^ *src2++;
-}
-EXPORT_SYMBOL_GPL(__crypto_xor);
-
 unsigned int crypto_alg_extsize(struct crypto_alg *alg)
 {
 	return alg->cra_ctxsize +
diff --git a/lib/crypto/Kconfig b/lib/crypto/Kconfig
index b09d9d6546cbc3..7e9683e9f5c636 100644
--- a/lib/crypto/Kconfig
+++ b/lib/crypto/Kconfig
@@ -36,7 +36,7 @@ config CRYPTO_ARCH_HAVE_LIB_CHACHA
 
 config CRYPTO_LIB_CHACHA_GENERIC
 	tristate
-	select XOR_BLOCKS
+	select CRYPTO_LIB_UTILS
 	help
 	  This symbol can be depended upon by arch implementations of the
 	  ChaCha library interface that require the generic code as a
@@ -46,7 +46,6 @@ config CRYPTO_LIB_CHACHA_GENERIC
 
 config CRYPTO_LIB_CHACHA
 	tristate "ChaCha library interface"
-	depends on CRYPTO
 	depends on CRYPTO_ARCH_HAVE_LIB_CHACHA || !CRYPTO_ARCH_HAVE_LIB_CHACHA
 	select CRYPTO_LIB_CHACHA_GENERIC if CRYPTO_ARCH_HAVE_LIB_CHACHA=n
 	help
diff --git a/lib/crypto/Makefile b/lib/crypto/Makefile
index b956b3bae26aaf..c852f067ab0601 100644
--- a/lib/crypto/Makefile
+++ b/lib/crypto/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_CRYPTO_LIB_UTILS)			+= libcryptoutils.o
-libcryptoutils-y				:= memneq.o
+libcryptoutils-y				:= memneq.o utils.o
 
 # chacha is used by the /dev/random driver which is always builtin
 obj-y						+= chacha.o
diff --git a/lib/crypto/utils.c b/lib/crypto/utils.c
new file mode 100644
index 00000000000000..f20bdb2ae88771
--- /dev/null
+++ b/lib/crypto/utils.c
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Crypto library utility functions
+ *
+ * Copyright (c) 2006 Herbert Xu <herbert@gondor.apana.org.au>
+ */
+
+#include <crypto/algapi.h>
+#include <asm/unaligned.h>
+
+/*
+ * XOR @len bytes from @src1 and @src2 together, writing the result to @dst
+ * (which may alias one of the sources).  Don't call this directly; call
+ * crypto_xor() or crypto_xor_cpy() instead.
+ */
+void __crypto_xor(u8 *dst, const u8 *src1, const u8 *src2, unsigned int len)
+{
+	int relalign = 0;
+
+	if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) {
+		int size = sizeof(unsigned long);
+		int d = (((unsigned long)dst ^ (unsigned long)src1) |
+			 ((unsigned long)dst ^ (unsigned long)src2)) &
+			(size - 1);
+
+		relalign = d ? 1 << __ffs(d) : size;
+
+		/*
+		 * If we care about alignment, process as many bytes as
+		 * needed to advance dst and src to values whose alignments
+		 * equal their relative alignment. This will allow us to
+		 * process the remainder of the input using optimal strides.
+		 */
+		while (((unsigned long)dst & (relalign - 1)) && len > 0) {
+			*dst++ = *src1++ ^ *src2++;
+			len--;
+		}
+	}
+
+	while (IS_ENABLED(CONFIG_64BIT) && len >= 8 && !(relalign & 7)) {
+		if (IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) {
+			u64 l = get_unaligned((u64 *)src1) ^
+				get_unaligned((u64 *)src2);
+			put_unaligned(l, (u64 *)dst);
+		} else {
+			*(u64 *)dst = *(u64 *)src1 ^ *(u64 *)src2;
+		}
+		dst += 8;
+		src1 += 8;
+		src2 += 8;
+		len -= 8;
+	}
+
+	while (len >= 4 && !(relalign & 3)) {
+		if (IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) {
+			u32 l = get_unaligned((u32 *)src1) ^
+				get_unaligned((u32 *)src2);
+			put_unaligned(l, (u32 *)dst);
+		} else {
+			*(u32 *)dst = *(u32 *)src1 ^ *(u32 *)src2;
+		}
+		dst += 4;
+		src1 += 4;
+		src2 += 4;
+		len -= 4;
+	}
+
+	while (len >= 2 && !(relalign & 1)) {
+		if (IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) {
+			u16 l = get_unaligned((u16 *)src1) ^
+				get_unaligned((u16 *)src2);
+			put_unaligned(l, (u16 *)dst);
+		} else {
+			*(u16 *)dst = *(u16 *)src1 ^ *(u16 *)src2;
+		}
+		dst += 2;
+		src1 += 2;
+		src2 += 2;
+		len -= 2;
+	}
+
+	while (len--)
+		*dst++ = *src1++ ^ *src2++;
+}
+EXPORT_SYMBOL_GPL(__crypto_xor);
-- 
2.37.0

[PATCH v2 3/3] crypto: lib - move crypto_simd_disabled_for_test into utils

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Move the definition of crypto_simd_disabled_for_test into
lib/crypto/utils.c so that it can be accessed by library code.

This is needed when code that is shared between a traditional crypto API
implementation and a library implementation is built-in, but
CRYPTO_ALGAPI=m.  The x86 blake2s previously was an example of this
(https://lore.kernel.org/linux-crypto/20220517033630.1182-1-gaochao49@huawei.com/T/#u).
Although that case was resolved by removing the blake2s shash support,
this problem could easily come back in the future, so let's address it.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/algapi.c    | 6 ------
 lib/crypto/utils.c | 6 ++++++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/crypto/algapi.c b/crypto/algapi.c
index 5c69ff8e8fa5c1..9377dae75b3def 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -6,7 +6,6 @@
  */
 
 #include <crypto/algapi.h>
-#include <crypto/internal/simd.h>
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/fips.h>
@@ -22,11 +21,6 @@
 
 static LIST_HEAD(crypto_template_list);
 
-#ifdef CONFIG_CRYPTO_MANAGER_EXTRA_TESTS
-DEFINE_PER_CPU(bool, crypto_simd_disabled_for_test);
-EXPORT_PER_CPU_SYMBOL_GPL(crypto_simd_disabled_for_test);
-#endif
-
 static inline void crypto_check_module_sig(struct module *mod)
 {
 	if (fips_enabled && mod && !module_sig_ok(mod))
diff --git a/lib/crypto/utils.c b/lib/crypto/utils.c
index f20bdb2ae88771..e7d99f150d4e61 100644
--- a/lib/crypto/utils.c
+++ b/lib/crypto/utils.c
@@ -6,8 +6,14 @@
  */
 
 #include <crypto/algapi.h>
+#include <crypto/internal/simd.h>
 #include <asm/unaligned.h>
 
+#ifdef CONFIG_CRYPTO_MANAGER_EXTRA_TESTS
+DEFINE_PER_CPU(bool, crypto_simd_disabled_for_test);
+EXPORT_PER_CPU_SYMBOL_GPL(crypto_simd_disabled_for_test);
+#endif
+
 /*
  * XOR @len bytes from @src1 and @src2 together, writing the result to @dst
  * (which may alias one of the sources).  Don't call this directly; call
-- 
2.37.0

Re: [PATCH v2 3/3] crypto: lib - move crypto_simd_disabled_for_test into utils

[Jason A. Donenfeld]

Hi Eric,

On Fri, Jul 15, 2022 at 11:29:20PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Move the definition of crypto_simd_disabled_for_test into
> lib/crypto/utils.c so that it can be accessed by library code.
> 
> This is needed when code that is shared between a traditional crypto API
> implementation and a library implementation is built-in, but
> CRYPTO_ALGAPI=m.  The x86 blake2s previously was an example of this
> (https://lore.kernel.org/linux-crypto/20220517033630.1182-1-gaochao49@huawei.com/T/#u).
> Although that case was resolved by removing the blake2s shash support,
> this problem could easily come back in the future, so let's address it.

I'm not sure I see the reason in general for a utility library rather
than doing these piecemeal like the rest of lib functions. Why is crypto
special here? But in particular to this patch: nothing is actually using
crypto_simd_disabled_for_test in lib/crypto, right? So is this
necessary?

Jason

Re: [PATCH v2 3/3] crypto: lib - move crypto_simd_disabled_for_test into utils

[Eric Biggers]

On Sat, Jul 16, 2022 at 08:32:55PM +0200, Jason A. Donenfeld wrote:
> Hi Eric,
> 
> On Fri, Jul 15, 2022 at 11:29:20PM -0700, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Move the definition of crypto_simd_disabled_for_test into
> > lib/crypto/utils.c so that it can be accessed by library code.
> > 
> > This is needed when code that is shared between a traditional crypto API
> > implementation and a library implementation is built-in, but
> > CRYPTO_ALGAPI=m.  The x86 blake2s previously was an example of this
> > (https://lore.kernel.org/linux-crypto/20220517033630.1182-1-gaochao49@huawei.com/T/#u).
> > Although that case was resolved by removing the blake2s shash support,
> > this problem could easily come back in the future, so let's address it.
> 
> I'm not sure I see the reason in general for a utility library rather
> than doing these piecemeal like the rest of lib functions. Why is crypto
> special here? But in particular to this patch: nothing is actually using
> crypto_simd_disabled_for_test in lib/crypto, right? So is this
> necessary?

Well, this is what Herbert wanted:
https://lore.kernel.org/r/YtEgzHuuMts0YBCz@gondor.apana.org.au.  It's
subjective, but for now I think I prefer this approach too, since the utility
functions are so small and are widely used.  A whole module is overkill for just
a few lines of code.

The commit message answers your second and third questions.

- Eric

Re: [PATCH v2 3/3] crypto: lib - move crypto_simd_disabled_for_test into utils

[Eric Biggers]

On Mon, Jul 18, 2022 at 09:34:23PM -0700, Eric Biggers wrote:
> On Sat, Jul 16, 2022 at 08:32:55PM +0200, Jason A. Donenfeld wrote:
> > Hi Eric,
> > 
> > On Fri, Jul 15, 2022 at 11:29:20PM -0700, Eric Biggers wrote:
> > > From: Eric Biggers <ebiggers@google.com>
> > > 
> > > Move the definition of crypto_simd_disabled_for_test into
> > > lib/crypto/utils.c so that it can be accessed by library code.
> > > 
> > > This is needed when code that is shared between a traditional crypto API
> > > implementation and a library implementation is built-in, but
> > > CRYPTO_ALGAPI=m.  The x86 blake2s previously was an example of this
> > > (https://lore.kernel.org/linux-crypto/20220517033630.1182-1-gaochao49@huawei.com/T/#u).
> > > Although that case was resolved by removing the blake2s shash support,
> > > this problem could easily come back in the future, so let's address it.
> > 
> > I'm not sure I see the reason in general for a utility library rather
> > than doing these piecemeal like the rest of lib functions. Why is crypto
> > special here? But in particular to this patch: nothing is actually using
> > crypto_simd_disabled_for_test in lib/crypto, right? So is this
> > necessary?
> 
> Well, this is what Herbert wanted:
> https://lore.kernel.org/r/YtEgzHuuMts0YBCz@gondor.apana.org.au.  It's
> subjective, but for now I think I prefer this approach too, since the utility
> functions are so small and are widely used.  A whole module is overkill for just
> a few lines of code.
> 
> The commit message answers your second and third questions.
> 

Herbert, any thoughts on this?

Note: I forgot to put a MODULE_LICENSE in the new module, so I'll need to resend
this patchset even if there are no other issues.

- Eric

Re: [PATCH v2 3/3] crypto: lib - move crypto_simd_disabled_for_test into utils

[Herbert Xu]

Hi Eric:

On Thu, Jul 21, 2022 at 12:01:47AM -0700, Eric Biggers wrote:
>
> Herbert, any thoughts on this?

I'm happy to drop the simd patch for now as there are no users
under lib/crypto.

> Note: I forgot to put a MODULE_LICENSE in the new module, so I'll need to resend
> this patchset even if there are no other issues.

Thanks!
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt