[PATCH 0/7] sha1 library cleanup

[PATCH 0/7] sha1 library cleanup

[Eric Biggers]

<linux/cryptohash.h> sounds very generic and important, like it's the
header to include if you're doing cryptographic hashing in the kernel.
But actually it only includes the library implementation of the SHA-1
compression function (not even the full SHA-1).  This should basically
never be used anymore; SHA-1 is no longer considered secure, and there
are much better ways to do cryptographic hashing in the kernel.

Also the function is named just "sha_transform()", which makes it
unclear which version of SHA is meant.

Therefore, this series cleans things up by moving these SHA-1
declarations into <crypto/sha.h> where they better belong, and changing
the names to say SHA-1 rather than just SHA.

As future work, we should split sha.h into sha1.h and sha2.h and try to
remove the remaining uses of SHA-1.  For example, the remaining use in
drivers/char/random.c is probably one that can be gotten rid of.

This patch series applies to cryptodev/master.

Eric Biggers (7):
  mptcp: use SHA256_BLOCK_SIZE, not SHA_MESSAGE_BYTES
  crypto: powerpc/sha1 - remove unused temporary workspace
  crypto: powerpc/sha1 - prefix the "sha1_" functions
  crypto: s390/sha1 - prefix the "sha1_" functions
  crypto: lib/sha1 - rename "sha" to "sha1"
  crypto: lib/sha1 - remove unnecessary includes of linux/cryptohash.h
  crypto: lib/sha1 - fold linux/cryptohash.h into crypto/sha.h

 Documentation/security/siphash.rst          |  2 +-
 arch/arm/crypto/sha1_glue.c                 |  1 -
 arch/arm/crypto/sha1_neon_glue.c            |  1 -
 arch/arm/crypto/sha256_glue.c               |  1 -
 arch/arm/crypto/sha256_neon_glue.c          |  1 -
 arch/arm/kernel/armksyms.c                  |  1 -
 arch/arm64/crypto/sha256-glue.c             |  1 -
 arch/arm64/crypto/sha512-glue.c             |  1 -
 arch/microblaze/kernel/microblaze_ksyms.c   |  1 -
 arch/mips/cavium-octeon/crypto/octeon-md5.c |  1 -
 arch/powerpc/crypto/md5-glue.c              |  1 -
 arch/powerpc/crypto/sha1-spe-glue.c         |  1 -
 arch/powerpc/crypto/sha1.c                  | 33 ++++++++++-----------
 arch/powerpc/crypto/sha256-spe-glue.c       |  1 -
 arch/s390/crypto/sha1_s390.c                | 12 ++++----
 arch/sparc/crypto/md5_glue.c                |  1 -
 arch/sparc/crypto/sha1_glue.c               |  1 -
 arch/sparc/crypto/sha256_glue.c             |  1 -
 arch/sparc/crypto/sha512_glue.c             |  1 -
 arch/unicore32/kernel/ksyms.c               |  1 -
 arch/x86/crypto/sha1_ssse3_glue.c           |  1 -
 arch/x86/crypto/sha256_ssse3_glue.c         |  1 -
 arch/x86/crypto/sha512_ssse3_glue.c         |  1 -
 crypto/sha1_generic.c                       |  5 ++--
 drivers/char/random.c                       |  8 ++---
 drivers/crypto/atmel-sha.c                  |  1 -
 drivers/crypto/chelsio/chcr_algo.c          |  1 -
 drivers/crypto/chelsio/chcr_ipsec.c         |  1 -
 drivers/crypto/omap-sham.c                  |  1 -
 fs/f2fs/hash.c                              |  1 -
 include/crypto/sha.h                        | 10 +++++++
 include/linux/cryptohash.h                  | 14 ---------
 include/linux/filter.h                      |  4 +--
 include/net/tcp.h                           |  1 -
 kernel/bpf/core.c                           | 18 +++++------
 lib/crypto/chacha.c                         |  1 -
 lib/sha1.c                                  | 24 ++++++++-------
 net/core/secure_seq.c                       |  1 -
 net/ipv6/addrconf.c                         | 10 +++----
 net/ipv6/seg6_hmac.c                        |  1 -
 net/mptcp/crypto.c                          |  4 +--
 41 files changed, 69 insertions(+), 104 deletions(-)
 delete mode 100644 include/linux/cryptohash.h


base-commit: 12b3cf9093542d9f752a4968815ece836159013f
-- 
2.26.2

[PATCH 2/7] crypto: powerpc/sha1 - remove unused temporary workspace

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

The PowerPC implementation of SHA-1 doesn't actually use the 16-word
temporary array that's passed to the assembly code.  This was probably
meant to correspond to the 'W' array that lib/sha1.c uses.  However, in
sha1-powerpc-asm.S these values are actually stored in GPRs 16-31.

Referencing SHA_WORKSPACE_WORDS from this code also isn't appropriate,
since it's an implementation detail of lib/sha1.c.

Therefore, just remove this unneeded array.

Tested with:

	export ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu-
	make mpc85xx_defconfig
	cat >> .config << EOF
	# CONFIG_MODULES is not set
	# CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not set
	CONFIG_DEBUG_KERNEL=y
	CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y
	CONFIG_CRYPTO_SHA1_PPC=y
	EOF
	make olddefconfig
	make -j32
	qemu-system-ppc -M mpc8544ds -cpu e500 -nographic \
		-kernel arch/powerpc/boot/zImage \
		-append "cryptomgr.fuzz_iterations=1000 cryptomgr.panic_on_fail=1"

Cc: linuxppc-dev@lists.ozlabs.org
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Paul Mackerras <paulus@samba.org>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 arch/powerpc/crypto/sha1.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/crypto/sha1.c b/arch/powerpc/crypto/sha1.c
index 7b43fc352089b1..db46b6130a9642 100644
--- a/arch/powerpc/crypto/sha1.c
+++ b/arch/powerpc/crypto/sha1.c
@@ -16,12 +16,11 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/mm.h>
-#include <linux/cryptohash.h>
 #include <linux/types.h>
 #include <crypto/sha.h>
 #include <asm/byteorder.h>
 
-extern void powerpc_sha_transform(u32 *state, const u8 *src, u32 *temp);
+void powerpc_sha_transform(u32 *state, const u8 *src);
 
 static int sha1_init(struct shash_desc *desc)
 {
@@ -47,7 +46,6 @@ static int sha1_update(struct shash_desc *desc, const u8 *data,
 	src = data;
 
 	if ((partial + len) > 63) {
-		u32 temp[SHA_WORKSPACE_WORDS];
 
 		if (partial) {
 			done = -partial;
@@ -56,12 +54,11 @@ static int sha1_update(struct shash_desc *desc, const u8 *data,
 		}
 
 		do {
-			powerpc_sha_transform(sctx->state, src, temp);
+			powerpc_sha_transform(sctx->state, src);
 			done += 64;
 			src = data + done;
 		} while (done + 63 < len);
 
-		memzero_explicit(temp, sizeof(temp));
 		partial = 0;
 	}
 	memcpy(sctx->buffer + partial, src, len - done);
-- 
2.26.2

[PATCH 3/7] crypto: powerpc/sha1 - prefix the "sha1_" functions

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Prefix the PowerPC SHA-1 functions with "powerpc_sha1_" rather than
"sha1_".  This allows us to rename the library function sha_init() to
sha1_init() without causing a naming collision.

Cc: linuxppc-dev@lists.ozlabs.org
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Paul Mackerras <paulus@samba.org>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 arch/powerpc/crypto/sha1.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/crypto/sha1.c b/arch/powerpc/crypto/sha1.c
index db46b6130a9642..b40dc50a6908ae 100644
--- a/arch/powerpc/crypto/sha1.c
+++ b/arch/powerpc/crypto/sha1.c
@@ -22,7 +22,7 @@
 
 void powerpc_sha_transform(u32 *state, const u8 *src);
 
-static int sha1_init(struct shash_desc *desc)
+static int powerpc_sha1_init(struct shash_desc *desc)
 {
 	struct sha1_state *sctx = shash_desc_ctx(desc);
 
@@ -33,8 +33,8 @@ static int sha1_init(struct shash_desc *desc)
 	return 0;
 }
 
-static int sha1_update(struct shash_desc *desc, const u8 *data,
-			unsigned int len)
+static int powerpc_sha1_update(struct shash_desc *desc, const u8 *data,
+			       unsigned int len)
 {
 	struct sha1_state *sctx = shash_desc_ctx(desc);
 	unsigned int partial, done;
@@ -68,7 +68,7 @@ static int sha1_update(struct shash_desc *desc, const u8 *data,
 
 
 /* Add padding and return the message digest. */
-static int sha1_final(struct shash_desc *desc, u8 *out)
+static int powerpc_sha1_final(struct shash_desc *desc, u8 *out)
 {
 	struct sha1_state *sctx = shash_desc_ctx(desc);
 	__be32 *dst = (__be32 *)out;
@@ -81,10 +81,10 @@ static int sha1_final(struct shash_desc *desc, u8 *out)
 	/* Pad out to 56 mod 64 */
 	index = sctx->count & 0x3f;
 	padlen = (index < 56) ? (56 - index) : ((64+56) - index);
-	sha1_update(desc, padding, padlen);
+	powerpc_sha1_update(desc, padding, padlen);
 
 	/* Append length */
-	sha1_update(desc, (const u8 *)&bits, sizeof(bits));
+	powerpc_sha1_update(desc, (const u8 *)&bits, sizeof(bits));
 
 	/* Store state in digest */
 	for (i = 0; i < 5; i++)
@@ -96,7 +96,7 @@ static int sha1_final(struct shash_desc *desc, u8 *out)
 	return 0;
 }
 
-static int sha1_export(struct shash_desc *desc, void *out)
+static int powerpc_sha1_export(struct shash_desc *desc, void *out)
 {
 	struct sha1_state *sctx = shash_desc_ctx(desc);
 
@@ -104,7 +104,7 @@ static int sha1_export(struct shash_desc *desc, void *out)
 	return 0;
 }
 
-static int sha1_import(struct shash_desc *desc, const void *in)
+static int powerpc_sha1_import(struct shash_desc *desc, const void *in)
 {
 	struct sha1_state *sctx = shash_desc_ctx(desc);
 
@@ -114,11 +114,11 @@ static int sha1_import(struct shash_desc *desc, const void *in)
 
 static struct shash_alg alg = {
 	.digestsize	=	SHA1_DIGEST_SIZE,
-	.init		=	sha1_init,
-	.update		=	sha1_update,
-	.final		=	sha1_final,
-	.export		=	sha1_export,
-	.import		=	sha1_import,
+	.init		=	powerpc_sha1_init,
+	.update		=	powerpc_sha1_update,
+	.final		=	powerpc_sha1_final,
+	.export		=	powerpc_sha1_export,
+	.import		=	powerpc_sha1_import,
 	.descsize	=	sizeof(struct sha1_state),
 	.statesize	=	sizeof(struct sha1_state),
 	.base		=	{
-- 
2.26.2

Re: [PATCH 0/7] sha1 library cleanup

[Jason A. Donenfeld]

Thanks for this series. I like the general idea. I think it might make
sense, though, to separate things out into sha1.h and sha256.h. That
will be nice preparation work for when we eventually move obsolete
primitives into some <crypto/dangerous/> subdirectory.

Re: [PATCH 0/7] sha1 library cleanup

[Ard Biesheuvel]

On Sat, 2 May 2020 at 20:28, Eric Biggers <ebiggers@kernel.org> wrote:
>
> <linux/cryptohash.h> sounds very generic and important, like it's the
> header to include if you're doing cryptographic hashing in the kernel.
> But actually it only includes the library implementation of the SHA-1
> compression function (not even the full SHA-1).  This should basically
> never be used anymore; SHA-1 is no longer considered secure, and there
> are much better ways to do cryptographic hashing in the kernel.
>
> Also the function is named just "sha_transform()", which makes it
> unclear which version of SHA is meant.
>
> Therefore, this series cleans things up by moving these SHA-1
> declarations into <crypto/sha.h> where they better belong, and changing
> the names to say SHA-1 rather than just SHA.
>
> As future work, we should split sha.h into sha1.h and sha2.h and try to
> remove the remaining uses of SHA-1.  For example, the remaining use in
> drivers/char/random.c is probably one that can be gotten rid of.
>
> This patch series applies to cryptodev/master.
>
> Eric Biggers (7):
>   mptcp: use SHA256_BLOCK_SIZE, not SHA_MESSAGE_BYTES
>   crypto: powerpc/sha1 - remove unused temporary workspace
>   crypto: powerpc/sha1 - prefix the "sha1_" functions
>   crypto: s390/sha1 - prefix the "sha1_" functions
>   crypto: lib/sha1 - rename "sha" to "sha1"
>   crypto: lib/sha1 - remove unnecessary includes of linux/cryptohash.h
>   crypto: lib/sha1 - fold linux/cryptohash.h into crypto/sha.h
>

For the series,

Acked-by: Ard Biesheuvel <ardb@kernel.org>

>  Documentation/security/siphash.rst          |  2 +-
>  arch/arm/crypto/sha1_glue.c                 |  1 -
>  arch/arm/crypto/sha1_neon_glue.c            |  1 -
>  arch/arm/crypto/sha256_glue.c               |  1 -
>  arch/arm/crypto/sha256_neon_glue.c          |  1 -
>  arch/arm/kernel/armksyms.c                  |  1 -
>  arch/arm64/crypto/sha256-glue.c             |  1 -
>  arch/arm64/crypto/sha512-glue.c             |  1 -
>  arch/microblaze/kernel/microblaze_ksyms.c   |  1 -
>  arch/mips/cavium-octeon/crypto/octeon-md5.c |  1 -
>  arch/powerpc/crypto/md5-glue.c              |  1 -
>  arch/powerpc/crypto/sha1-spe-glue.c         |  1 -
>  arch/powerpc/crypto/sha1.c                  | 33 ++++++++++-----------
>  arch/powerpc/crypto/sha256-spe-glue.c       |  1 -
>  arch/s390/crypto/sha1_s390.c                | 12 ++++----
>  arch/sparc/crypto/md5_glue.c                |  1 -
>  arch/sparc/crypto/sha1_glue.c               |  1 -
>  arch/sparc/crypto/sha256_glue.c             |  1 -
>  arch/sparc/crypto/sha512_glue.c             |  1 -
>  arch/unicore32/kernel/ksyms.c               |  1 -
>  arch/x86/crypto/sha1_ssse3_glue.c           |  1 -
>  arch/x86/crypto/sha256_ssse3_glue.c         |  1 -
>  arch/x86/crypto/sha512_ssse3_glue.c         |  1 -
>  crypto/sha1_generic.c                       |  5 ++--
>  drivers/char/random.c                       |  8 ++---
>  drivers/crypto/atmel-sha.c                  |  1 -
>  drivers/crypto/chelsio/chcr_algo.c          |  1 -
>  drivers/crypto/chelsio/chcr_ipsec.c         |  1 -
>  drivers/crypto/omap-sham.c                  |  1 -
>  fs/f2fs/hash.c                              |  1 -
>  include/crypto/sha.h                        | 10 +++++++
>  include/linux/cryptohash.h                  | 14 ---------
>  include/linux/filter.h                      |  4 +--
>  include/net/tcp.h                           |  1 -
>  kernel/bpf/core.c                           | 18 +++++------
>  lib/crypto/chacha.c                         |  1 -
>  lib/sha1.c                                  | 24 ++++++++-------
>  net/core/secure_seq.c                       |  1 -
>  net/ipv6/addrconf.c                         | 10 +++----
>  net/ipv6/seg6_hmac.c                        |  1 -
>  net/mptcp/crypto.c                          |  4 +--
>  41 files changed, 69 insertions(+), 104 deletions(-)
>  delete mode 100644 include/linux/cryptohash.h
>
>
> base-commit: 12b3cf9093542d9f752a4968815ece836159013f
> --
> 2.26.2
>

Re: [PATCH 0/7] sha1 library cleanup

[Eric Biggers]

On Sat, May 02, 2020 at 03:05:46PM -0600, Jason A. Donenfeld wrote:
> Thanks for this series. I like the general idea. I think it might make
> sense, though, to separate things out into sha1.h and sha256.h. That
> will be nice preparation work for when we eventually move obsolete
> primitives into some <crypto/dangerous/> subdirectory.

That's basically what I suggested in the cover letter:

"As future work, we should split sha.h into sha1.h and sha2.h and try to
remove the remaining uses of SHA-1.  For example, the remaining use in
drivers/char/random.c is probably one that can be gotten rid of."

("sha2.h" rather than "sha256.h", since it would include SHA-512 too.
Also, we already have sha3.h, so having sha{1,2,3}.h would be logical.)

But there are 108 files that include <crypto/sha.h>, all of which would need to
be updated, which risks merge conflicts.  So this series seemed like a good
stopping point to get these initial changes in for 5.8.  Then in the next
release we can split up sha.h (and debate whether sha1.h should really be
"<crypto/dangerous/sha1.h>" or whatever).

There are 3 files where I added an include of sha.h, where we could go directly
to sha1.h if we did it now.  But that's not much compared to the 108 files.

- Eric

Re: [PATCH 2/7] crypto: powerpc/sha1 - remove unused temporary workspace

[Michael Ellerman]

Eric Biggers <ebiggers@kernel.org> writes:
> From: Eric Biggers <ebiggers@google.com>
>
> The PowerPC implementation of SHA-1 doesn't actually use the 16-word
> temporary array that's passed to the assembly code.  This was probably
> meant to correspond to the 'W' array that lib/sha1.c uses.  However, in
> sha1-powerpc-asm.S these values are actually stored in GPRs 16-31.
>
> Referencing SHA_WORKSPACE_WORDS from this code also isn't appropriate,
> since it's an implementation detail of lib/sha1.c.
>
> Therefore, just remove this unneeded array.
>
> Tested with:
>
> 	export ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu-
> 	make mpc85xx_defconfig
> 	cat >> .config << EOF
> 	# CONFIG_MODULES is not set
> 	# CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not set
> 	CONFIG_DEBUG_KERNEL=y
> 	CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y
> 	CONFIG_CRYPTO_SHA1_PPC=y
> 	EOF
> 	make olddefconfig
> 	make -j32
> 	qemu-system-ppc -M mpc8544ds -cpu e500 -nographic \
> 		-kernel arch/powerpc/boot/zImage \
> 		-append "cryptomgr.fuzz_iterations=1000 cryptomgr.panic_on_fail=1"

Thanks for testing.

I gave it a quick spin on a Power9 and it showed no issues.

Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)

cheers

Re: [PATCH 0/7] sha1 library cleanup

[Herbert Xu]

Eric Biggers <ebiggers@kernel.org> wrote:
> <linux/cryptohash.h> sounds very generic and important, like it's the
> header to include if you're doing cryptographic hashing in the kernel.
> But actually it only includes the library implementation of the SHA-1
> compression function (not even the full SHA-1).  This should basically
> never be used anymore; SHA-1 is no longer considered secure, and there
> are much better ways to do cryptographic hashing in the kernel.
> 
> Also the function is named just "sha_transform()", which makes it
> unclear which version of SHA is meant.
> 
> Therefore, this series cleans things up by moving these SHA-1
> declarations into <crypto/sha.h> where they better belong, and changing
> the names to say SHA-1 rather than just SHA.
> 
> As future work, we should split sha.h into sha1.h and sha2.h and try to
> remove the remaining uses of SHA-1.  For example, the remaining use in
> drivers/char/random.c is probably one that can be gotten rid of.
> 
> This patch series applies to cryptodev/master.
> 
> Eric Biggers (7):
>  mptcp: use SHA256_BLOCK_SIZE, not SHA_MESSAGE_BYTES
>  crypto: powerpc/sha1 - remove unused temporary workspace
>  crypto: powerpc/sha1 - prefix the "sha1_" functions
>  crypto: s390/sha1 - prefix the "sha1_" functions
>  crypto: lib/sha1 - rename "sha" to "sha1"
>  crypto: lib/sha1 - remove unnecessary includes of linux/cryptohash.h
>  crypto: lib/sha1 - fold linux/cryptohash.h into crypto/sha.h
> 
> Documentation/security/siphash.rst          |  2 +-
> arch/arm/crypto/sha1_glue.c                 |  1 -
> arch/arm/crypto/sha1_neon_glue.c            |  1 -
> arch/arm/crypto/sha256_glue.c               |  1 -
> arch/arm/crypto/sha256_neon_glue.c          |  1 -
> arch/arm/kernel/armksyms.c                  |  1 -
> arch/arm64/crypto/sha256-glue.c             |  1 -
> arch/arm64/crypto/sha512-glue.c             |  1 -
> arch/microblaze/kernel/microblaze_ksyms.c   |  1 -
> arch/mips/cavium-octeon/crypto/octeon-md5.c |  1 -
> arch/powerpc/crypto/md5-glue.c              |  1 -
> arch/powerpc/crypto/sha1-spe-glue.c         |  1 -
> arch/powerpc/crypto/sha1.c                  | 33 ++++++++++-----------
> arch/powerpc/crypto/sha256-spe-glue.c       |  1 -
> arch/s390/crypto/sha1_s390.c                | 12 ++++----
> arch/sparc/crypto/md5_glue.c                |  1 -
> arch/sparc/crypto/sha1_glue.c               |  1 -
> arch/sparc/crypto/sha256_glue.c             |  1 -
> arch/sparc/crypto/sha512_glue.c             |  1 -
> arch/unicore32/kernel/ksyms.c               |  1 -
> arch/x86/crypto/sha1_ssse3_glue.c           |  1 -
> arch/x86/crypto/sha256_ssse3_glue.c         |  1 -
> arch/x86/crypto/sha512_ssse3_glue.c         |  1 -
> crypto/sha1_generic.c                       |  5 ++--
> drivers/char/random.c                       |  8 ++---
> drivers/crypto/atmel-sha.c                  |  1 -
> drivers/crypto/chelsio/chcr_algo.c          |  1 -
> drivers/crypto/chelsio/chcr_ipsec.c         |  1 -
> drivers/crypto/omap-sham.c                  |  1 -
> fs/f2fs/hash.c                              |  1 -
> include/crypto/sha.h                        | 10 +++++++
> include/linux/cryptohash.h                  | 14 ---------
> include/linux/filter.h                      |  4 +--
> include/net/tcp.h                           |  1 -
> kernel/bpf/core.c                           | 18 +++++------
> lib/crypto/chacha.c                         |  1 -
> lib/sha1.c                                  | 24 ++++++++-------
> net/core/secure_seq.c                       |  1 -
> net/ipv6/addrconf.c                         | 10 +++----
> net/ipv6/seg6_hmac.c                        |  1 -
> net/mptcp/crypto.c                          |  4 +--
> 41 files changed, 69 insertions(+), 104 deletions(-)
> delete mode 100644 include/linux/cryptohash.h
> 
> 
> base-commit: 12b3cf9093542d9f752a4968815ece836159013f

All applied.  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