[PATCH v2 0/7] crypto: fixes in preparation of new fuzz tests

[PATCH v2 0/7] crypto: fixes in preparation of new fuzz tests

[Eric Biggers]

This series contains the remaining fixes in preparation of the
comparison fuzz tests I've proposed in the series
"crypto: fuzz algorithms against their generic implementation".

I will send a new version of the testmgr patches separately.

These fixes and my latest tests can also be retrieved from git at:

	Repository: https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git
	Branch: cryptofuzz-vs-generic

Eric Biggers (7):
  crypto: lrw - don't access already-freed walk.iv
  crypto: salsa20 - don't access already-freed walk.iv
  crypto: arm/aes-neonbs - don't access already-freed walk.iv
  crypto: arm64/aes-neonbs - don't access already-freed walk.iv
  crypto: gcm - fix incompatibility between "gcm" and "gcm_base"
  crypto: ccm - fix incompatibility between "ccm" and "ccm_base"
  crypto: vmx - return correct error code on failed setkey

 arch/arm/crypto/aes-neonbs-glue.c   |  2 ++
 arch/arm64/crypto/aes-neonbs-glue.c |  2 ++
 crypto/ccm.c                        | 43 ++++++++++++-----------------
 crypto/gcm.c                        | 32 ++++++---------------
 crypto/lrw.c                        |  4 ++-
 crypto/salsa20_generic.c            |  2 +-
 drivers/crypto/vmx/aes.c            |  7 +++--
 drivers/crypto/vmx/aes_cbc.c        |  7 +++--
 drivers/crypto/vmx/aes_ctr.c        |  5 ++--
 drivers/crypto/vmx/aes_xts.c        |  9 +++---
 10 files changed, 50 insertions(+), 63 deletions(-)

-- 
2.21.0

[PATCH v2 1/7] crypto: lrw - don't access already-freed walk.iv

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

If the user-provided IV needs to be aligned to the algorithm's
alignmask, then skcipher_walk_virt() copies the IV into a new aligned
buffer walk.iv.  But skcipher_walk_virt() can fail afterwards, and then
if the caller unconditionally accesses walk.iv, it's a use-after-free.

Fix this in the LRW template by checking the return value of
skcipher_walk_virt().

This bug was detected by my patches that improve testmgr to fuzz
algorithms against their generic implementation.  When the extra
self-tests were run on a KASAN-enabled kernel, a KASAN use-after-free
splat occured during lrw(aes) testing.

Fixes: c778f96bf347 ("crypto: lrw - Optimize tweak computation")
Cc: <stable@vger.kernel.org> # v4.20+
Cc: Ondrej Mosnacek <omosnace@redhat.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/lrw.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/crypto/lrw.c b/crypto/lrw.c
index 0430ccd087286..b6666c595a686 100644
--- a/crypto/lrw.c
+++ b/crypto/lrw.c
@@ -162,8 +162,10 @@ static int xor_tweak(struct skcipher_request *req, bool second_pass)
 	}
 
 	err = skcipher_walk_virt(&w, req, false);
-	iv = (__be32 *)w.iv;
+	if (err)
+		return err;
 
+	iv = (__be32 *)w.iv;
 	counter[0] = be32_to_cpu(iv[3]);
 	counter[1] = be32_to_cpu(iv[2]);
 	counter[2] = be32_to_cpu(iv[1]);
-- 
2.21.0

[PATCH v2 2/7] crypto: salsa20 - don't access already-freed walk.iv

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

If the user-provided IV needs to be aligned to the algorithm's
alignmask, then skcipher_walk_virt() copies the IV into a new aligned
buffer walk.iv.  But skcipher_walk_virt() can fail afterwards, and then
if the caller unconditionally accesses walk.iv, it's a use-after-free.

salsa20-generic doesn't set an alignmask, so currently it isn't affected
by this despite unconditionally accessing walk.iv.  However this is more
subtle than desired, and it was actually broken prior to the alignmask
being removed by commit b62b3db76f73 ("crypto: salsa20-generic - cleanup
and convert to skcipher API").

Since salsa20-generic does not update the IV and does not need any IV
alignment, update it to use req->iv instead of walk.iv.

Fixes: 2407d60872dd ("[CRYPTO] salsa20: Salsa20 stream cipher")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/salsa20_generic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/salsa20_generic.c b/crypto/salsa20_generic.c
index 443fba09cbed7..faed244be316f 100644
--- a/crypto/salsa20_generic.c
+++ b/crypto/salsa20_generic.c
@@ -160,7 +160,7 @@ static int salsa20_crypt(struct skcipher_request *req)
 
 	err = skcipher_walk_virt(&walk, req, false);
 
-	salsa20_init(state, ctx, walk.iv);
+	salsa20_init(state, ctx, req->iv);
 
 	while (walk.nbytes > 0) {
 		unsigned int nbytes = walk.nbytes;
-- 
2.21.0

Re: [PATCH v2 2/7] crypto: salsa20 - don't access already-freed walk.iv

[Sasha Levin]

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 2407d60872dd [CRYPTO] salsa20: Salsa20 stream cipher.

The bot has tested the following trees: v5.0.7, v4.19.34, v4.14.111, v4.9.168, v4.4.178, v3.18.138.

v5.0.7: Build OK!
v4.19.34: Build OK!
v4.14.111: Failed to apply! Possible dependencies:
    015a03704df1 ("crypto: salsa20 - Revert "crypto: salsa20 - export generic helpers"")
    b62b3db76f73 ("crypto: salsa20-generic - cleanup and convert to skcipher API")
    eb772f37ae81 ("crypto: salsa20 - export generic helpers")

v4.9.168: Failed to apply! Possible dependencies:
    015a03704df1 ("crypto: salsa20 - Revert "crypto: salsa20 - export generic helpers"")
    b62b3db76f73 ("crypto: salsa20-generic - cleanup and convert to skcipher API")
    eb772f37ae81 ("crypto: salsa20 - export generic helpers")

v4.4.178: Failed to apply! Possible dependencies:
    015a03704df1 ("crypto: salsa20 - Revert "crypto: salsa20 - export generic helpers"")
    b62b3db76f73 ("crypto: salsa20-generic - cleanup and convert to skcipher API")
    eb772f37ae81 ("crypto: salsa20 - export generic helpers")

v3.18.138: Failed to apply! Possible dependencies:
    015a03704df1 ("crypto: salsa20 - Revert "crypto: salsa20 - export generic helpers"")
    b62b3db76f73 ("crypto: salsa20-generic - cleanup and convert to skcipher API")
    eb772f37ae81 ("crypto: salsa20 - export generic helpers")


How should we proceed with this patch?

--
Thanks,
Sasha

[PATCH v2 3/7] crypto: arm/aes-neonbs - don't access already-freed walk.iv

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

If the user-provided IV needs to be aligned to the algorithm's
alignmask, then skcipher_walk_virt() copies the IV into a new aligned
buffer walk.iv.  But skcipher_walk_virt() can fail afterwards, and then
if the caller unconditionally accesses walk.iv, it's a use-after-free.

arm32 xts-aes-neonbs doesn't set an alignmask, so currently it isn't
affected by this despite unconditionally accessing walk.iv.  However
this is more subtle than desired, and it was actually broken prior to
the alignmask being removed by commit cc477bf64573 ("crypto: arm/aes -
replace bit-sliced OpenSSL NEON code").  Thus, update xts-aes-neonbs to
start checking the return value of skcipher_walk_virt().

Fixes: e4e7f10bfc40 ("ARM: add support for bit sliced AES using NEON instructions")
Cc: <stable@vger.kernel.org> # v3.13+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 arch/arm/crypto/aes-neonbs-glue.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/crypto/aes-neonbs-glue.c b/arch/arm/crypto/aes-neonbs-glue.c
index 07e31941dc674..617c2c99ebfb3 100644
--- a/arch/arm/crypto/aes-neonbs-glue.c
+++ b/arch/arm/crypto/aes-neonbs-glue.c
@@ -278,6 +278,8 @@ static int __xts_crypt(struct skcipher_request *req,
 	int err;
 
 	err = skcipher_walk_virt(&walk, req, true);
+	if (err)
+		return err;
 
 	crypto_cipher_encrypt_one(ctx->tweak_tfm, walk.iv, walk.iv);
 
-- 
2.21.0

Re: [PATCH v2 3/7] crypto: arm/aes-neonbs - don't access already-freed walk.iv

[Sasha Levin]

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: e4e7f10bfc40 ARM: add support for bit sliced AES using NEON instructions.

The bot has tested the following trees: v5.0.7, v4.19.34, v4.14.111, v4.9.168, v4.4.178, v3.18.138.

v5.0.7: Build OK!
v4.19.34: Build OK!
v4.14.111: Build OK!
v4.9.168: Failed to apply! Possible dependencies:
    211f41af534a ("crypto: aesbs - Convert to skcipher")
    585b5fa63da9 ("crypto: arm/aes - Select SIMD in Kconfig")
    6fdf436fd854 ("crypto: arm/aes - Add missing SIMD select for aesbs")
    81126d1a8bc2 ("crypto: arm/aesbs - fix brokenness after skcipher conversion")
    81edb4262975 ("crypto: arm/aes - replace scalar AES cipher")
    cc477bf64573 ("crypto: arm/aes - replace bit-sliced OpenSSL NEON code")

v4.4.178: Failed to apply! Possible dependencies:
    211f41af534a ("crypto: aesbs - Convert to skcipher")
    28856a9e52c7 ("crypto: xts - consolidate sanity check for keys")
    49abc0d2e19b ("crypto: xts - fix compile errors")
    585b5fa63da9 ("crypto: arm/aes - Select SIMD in Kconfig")
    6fdf436fd854 ("crypto: arm/aes - Add missing SIMD select for aesbs")
    81126d1a8bc2 ("crypto: arm/aesbs - fix brokenness after skcipher conversion")
    81edb4262975 ("crypto: arm/aes - replace scalar AES cipher")
    cc477bf64573 ("crypto: arm/aes - replace bit-sliced OpenSSL NEON code")

v3.18.138: Failed to apply! Possible dependencies:
    006d0624fa0d ("crypto: arm - add support for SHA-224/256 using ARMv8 Crypto Extensions")
    12ac3efe74f8 ("arm64/crypto: use crypto instructions to generate AES key schedule")
    504c6143c53d ("crypto: powerpc/aes - kernel config")
    585b5fa63da9 ("crypto: arm/aes - Select SIMD in Kconfig")
    652ccae5cc4e ("crypto: arm - move ARM specific Kconfig definitions to a dedicated file")
    6fdf436fd854 ("crypto: arm/aes - Add missing SIMD select for aesbs")
    81edb4262975 ("crypto: arm/aes - replace scalar AES cipher")
    86464859cc77 ("crypto: arm - AES in ECB/CBC/CTR/XTS modes using ARMv8 Crypto Extensions")
    864cbeed4ab2 ("crypto: arm - add support for SHA1 using ARMv8 Crypto Instructions")
    cc477bf64573 ("crypto: arm/aes - replace bit-sliced OpenSSL NEON code")


How should we proceed with this patch?

--
Thanks,
Sasha

[PATCH v2 4/7] crypto: arm64/aes-neonbs - don't access already-freed walk.iv

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

If the user-provided IV needs to be aligned to the algorithm's
alignmask, then skcipher_walk_virt() copies the IV into a new aligned
buffer walk.iv.  But skcipher_walk_virt() can fail afterwards, and then
if the caller unconditionally accesses walk.iv, it's a use-after-free.

xts-aes-neonbs doesn't set an alignmask, so currently it isn't affected
by this despite unconditionally accessing walk.iv.  However this is more
subtle than desired, and unconditionally accessing walk.iv has caused a
real problem in other algorithms.  Thus, update xts-aes-neonbs to start
checking the return value of skcipher_walk_virt().

Fixes: 1abee99eafab ("crypto: arm64/aes - reimplement bit-sliced ARM/NEON implementation for arm64")
Cc: <stable@vger.kernel.org> # v4.11+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 arch/arm64/crypto/aes-neonbs-glue.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/crypto/aes-neonbs-glue.c b/arch/arm64/crypto/aes-neonbs-glue.c
index 4737b6c6c5cf5..5144551177334 100644
--- a/arch/arm64/crypto/aes-neonbs-glue.c
+++ b/arch/arm64/crypto/aes-neonbs-glue.c
@@ -304,6 +304,8 @@ static int __xts_crypt(struct skcipher_request *req,
 	int err;
 
 	err = skcipher_walk_virt(&walk, req, false);
+	if (err)
+		return err;
 
 	kernel_neon_begin();
 	neon_aes_ecb_encrypt(walk.iv, walk.iv, ctx->twkey, ctx->key.rounds, 1);
-- 
2.21.0

Re: [PATCH v2 4/7] crypto: arm64/aes-neonbs - don't access already-freed walk.iv

[Sasha Levin]

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 1abee99eafab crypto: arm64/aes - reimplement bit-sliced ARM/NEON implementation for arm64.

The bot has tested the following trees: v5.0.7, v4.19.34, v4.14.111.

v5.0.7: Build OK!
v4.19.34: Build OK!
v4.14.111: Failed to apply! Possible dependencies:
    683381747270 ("crypto: arm64/aes-blk - move kernel mode neon en/disable into loop")
    78ad7b08d8e0 ("crypto: arm64/aes-bs - move kernel mode neon en/disable into loop")


How should we proceed with this patch?

--
Thanks,
Sasha

[PATCH v2 5/7] crypto: gcm - fix incompatibility between "gcm" and "gcm_base"

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

GCM instances can be created by either the "gcm" template, which only
allows choosing the block cipher, e.g. "gcm(aes)"; or by "gcm_base",
which allows choosing the ctr and ghash implementations, e.g.
"gcm_base(ctr(aes-generic),ghash-generic)".

However, a "gcm_base" instance prevents a "gcm" instance from being
registered using the same implementations.  Nor will the instance be
found by lookups of "gcm".  This can be used as a denial of service.
Moreover, "gcm_base" instances are never tested by the crypto
self-tests, even if there are compatible "gcm" tests.

The root cause of these problems is that instances of the two templates
use different cra_names.  Therefore, fix these problems by making
"gcm_base" instances set the same cra_name as "gcm" instances, e.g.
"gcm(aes)" instead of "gcm_base(ctr(aes-generic),ghash-generic)".

This requires extracting the block cipher name from the name of the ctr
algorithm.  It also requires starting to verify that the algorithms are
really ctr and ghash, not something else entirely.  But it would be
bizarre if anyone were actually using non-gcm-compatible algorithms with
gcm_base, so this shouldn't break anyone in practice.

Fixes: d00aa19b507b ("[CRYPTO] gcm: Allow block cipher parameter")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/gcm.c | 32 +++++++++-----------------------
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/crypto/gcm.c b/crypto/gcm.c
index e1a11f529d257..c7354ed1fa4d4 100644
--- a/crypto/gcm.c
+++ b/crypto/gcm.c
@@ -597,7 +597,6 @@ static void crypto_gcm_free(struct aead_instance *inst)
 
 static int crypto_gcm_create_common(struct crypto_template *tmpl,
 				    struct rtattr **tb,
-				    const char *full_name,
 				    const char *ctr_name,
 				    const char *ghash_name)
 {
@@ -638,7 +637,7 @@ static int crypto_gcm_create_common(struct crypto_template *tmpl,
 		goto err_free_inst;
 
 	err = -EINVAL;
-	if (ghash->digestsize != 16)
+	if (strcmp(ghash->base.cra_name, "ghash") != 0)
 		goto err_drop_ghash;
 
 	crypto_set_skcipher_spawn(&ctx->ctr, aead_crypto_instance(inst));
@@ -650,24 +649,23 @@ static int crypto_gcm_create_common(struct crypto_template *tmpl,
 
 	ctr = crypto_spawn_skcipher_alg(&ctx->ctr);
 
-	/* We only support 16-byte blocks. */
+	/* The skcipher algorithm must be CTR mode, using 16-byte blocks. */
 	err = -EINVAL;
-	if (crypto_skcipher_alg_ivsize(ctr) != 16)
+	if (strncmp(ctr->base.cra_name, "ctr(", 4) != 0 ||
+	    crypto_skcipher_alg_ivsize(ctr) != 16)
 		goto out_put_ctr;
 
-	/* Not a stream cipher? */
-	if (ctr->base.cra_blocksize != 1)
+	err = -ENAMETOOLONG;
+	if (snprintf(inst->alg.base.cra_name, CRYPTO_MAX_ALG_NAME,
+		     "gcm(%s", ctr->base.cra_name + 4) >= CRYPTO_MAX_ALG_NAME)
 		goto out_put_ctr;
 
-	err = -ENAMETOOLONG;
 	if (snprintf(inst->alg.base.cra_driver_name, CRYPTO_MAX_ALG_NAME,
 		     "gcm_base(%s,%s)", ctr->base.cra_driver_name,
 		     ghash_alg->cra_driver_name) >=
 	    CRYPTO_MAX_ALG_NAME)
 		goto out_put_ctr;
 
-	memcpy(inst->alg.base.cra_name, full_name, CRYPTO_MAX_ALG_NAME);
-
 	inst->alg.base.cra_flags = (ghash->base.cra_flags |
 				    ctr->base.cra_flags) & CRYPTO_ALG_ASYNC;
 	inst->alg.base.cra_priority = (ghash->base.cra_priority +
@@ -709,7 +707,6 @@ static int crypto_gcm_create(struct crypto_template *tmpl, struct rtattr **tb)
 {
 	const char *cipher_name;
 	char ctr_name[CRYPTO_MAX_ALG_NAME];
-	char full_name[CRYPTO_MAX_ALG_NAME];
 
 	cipher_name = crypto_attr_alg_name(tb[1]);
 	if (IS_ERR(cipher_name))
@@ -719,12 +716,7 @@ static int crypto_gcm_create(struct crypto_template *tmpl, struct rtattr **tb)
 	    CRYPTO_MAX_ALG_NAME)
 		return -ENAMETOOLONG;
 
-	if (snprintf(full_name, CRYPTO_MAX_ALG_NAME, "gcm(%s)", cipher_name) >=
-	    CRYPTO_MAX_ALG_NAME)
-		return -ENAMETOOLONG;
-
-	return crypto_gcm_create_common(tmpl, tb, full_name,
-					ctr_name, "ghash");
+	return crypto_gcm_create_common(tmpl, tb, ctr_name, "ghash");
 }
 
 static int crypto_gcm_base_create(struct crypto_template *tmpl,
@@ -732,7 +724,6 @@ static int crypto_gcm_base_create(struct crypto_template *tmpl,
 {
 	const char *ctr_name;
 	const char *ghash_name;
-	char full_name[CRYPTO_MAX_ALG_NAME];
 
 	ctr_name = crypto_attr_alg_name(tb[1]);
 	if (IS_ERR(ctr_name))
@@ -742,12 +733,7 @@ static int crypto_gcm_base_create(struct crypto_template *tmpl,
 	if (IS_ERR(ghash_name))
 		return PTR_ERR(ghash_name);
 
-	if (snprintf(full_name, CRYPTO_MAX_ALG_NAME, "gcm_base(%s,%s)",
-		     ctr_name, ghash_name) >= CRYPTO_MAX_ALG_NAME)
-		return -ENAMETOOLONG;
-
-	return crypto_gcm_create_common(tmpl, tb, full_name,
-					ctr_name, ghash_name);
+	return crypto_gcm_create_common(tmpl, tb, ctr_name, ghash_name);
 }
 
 static int crypto_rfc4106_setkey(struct crypto_aead *parent, const u8 *key,
-- 
2.21.0

Re: [PATCH v2 5/7] crypto: gcm - fix incompatibility between "gcm" and "gcm_base"

[Sasha Levin]

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: d00aa19b507b [CRYPTO] gcm: Allow block cipher parameter.

The bot has tested the following trees: v5.0.7, v4.19.34, v4.14.111, v4.9.168, v4.4.178, v3.18.138.

v5.0.7: Build OK!
v4.19.34: Build OK!
v4.14.111: Build OK!
v4.9.168: Failed to apply! Possible dependencies:
    9b40f79c08e8 ("crypto: gcm - Fix error return code in crypto_gcm_create_common()")

v4.4.178: Failed to apply! Possible dependencies:
    16f37ecdd068 ("crypto: gcm - Use skcipher")
    9b40f79c08e8 ("crypto: gcm - Fix error return code in crypto_gcm_create_common()")

v3.18.138: Failed to apply! Possible dependencies:
    16f37ecdd068 ("crypto: gcm - Use skcipher")
    17db85469970 ("crypto: gcm - Use default null skcipher")
    5d72336f1b2c ("crypto: gcm - Use crypto_aead_set_reqsize helper")
    9b40f79c08e8 ("crypto: gcm - Fix error return code in crypto_gcm_create_common()")
    adcbc688fe2f ("crypto: gcm - Convert to new AEAD interface")


How should we proceed with this patch?

--
Thanks,
Sasha

Re: [PATCH v2 5/7] crypto: gcm - fix incompatibility between "gcm" and "gcm_base"

[Herbert Xu]

Eric Biggers <ebiggers@kernel.org> wrote:
>
> @@ -638,7 +637,7 @@ static int crypto_gcm_create_common(struct crypto_template *tmpl,
>                goto err_free_inst;
> 
>        err = -EINVAL;
> -       if (ghash->digestsize != 16)
> +       if (strcmp(ghash->base.cra_name, "ghash") != 0)
>                goto err_drop_ghash;

We should keep both tests because the self-tests can be compiled
out so there is no guarantee that something claiming to be ghash
actually is ghash.

Cheers,
-- 
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

Re: [PATCH v2 5/7] crypto: gcm - fix incompatibility between "gcm" and "gcm_base"

[Eric Biggers]

On Thu, Apr 18, 2019 at 10:00:06PM +0800, Herbert Xu wrote:
> Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > @@ -638,7 +637,7 @@ static int crypto_gcm_create_common(struct crypto_template *tmpl,
> >                goto err_free_inst;
> > 
> >        err = -EINVAL;
> > -       if (ghash->digestsize != 16)
> > +       if (strcmp(ghash->base.cra_name, "ghash") != 0)
> >                goto err_drop_ghash;
> 
> We should keep both tests because the self-tests can be compiled
> out so there is no guarantee that something claiming to be ghash
> actually is ghash.
> 

I'm not necessarily opposed to doing this, but if we're assuming that untested,
arbitrarily broken algorithms may be registered with the crypto API under any
name, "ghash" could easily still be broken even if it declares a 16-byte digest
size.  Verifying the digest size is just an extra sanity check; we're really
still relying on the implementation to be correct.  (Same for the ccm patch.)

- Eric

Re: [PATCH v2 5/7] crypto: gcm - fix incompatibility between "gcm" and "gcm_base"

[Herbert Xu]

On Thu, Apr 18, 2019 at 11:41:46AM -0700, Eric Biggers wrote:
>
> I'm not necessarily opposed to doing this, but if we're assuming that untested,
> arbitrarily broken algorithms may be registered with the crypto API under any
> name, "ghash" could easily still be broken even if it declares a 16-byte digest
> size.  Verifying the digest size is just an extra sanity check; we're really
> still relying on the implementation to be correct.  (Same for the ccm patch.)

The check is not so much for the purpose of producing the correct
output for the given algorithm, since as you correctly stated that
would rely on the correctness of the underlying algorithm.  It is
more about not crashing when the underlying algorithm has a geometry
that is completely different (e.g., by overwriting the digest
buffer).

Come to think of it, perhaps we should check the geometry of
algorithms even when self-tests are compiled out.  That way these
checks could be replaced by a simple name check.

This would be good anyway because the self-tests may indeed crash
if an algorithm with an incorrect geometry is used.

Cheers,
-- 
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

[PATCH v2 6/7] crypto: ccm - fix incompatibility between "ccm" and "ccm_base"

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

CCM instances can be created by either the "ccm" template, which only
allows choosing the block cipher, e.g. "ccm(aes)"; or by "ccm_base",
which allows choosing the ctr and cbcmac implementations, e.g.
"ccm_base(ctr(aes-generic),cbcmac(aes-generic))".

However, a "ccm_base" instance prevents a "ccm" instance from being
registered using the same implementations.  Nor will the instance be
found by lookups of "ccm".  This can be used as a denial of service.
Moreover, "ccm_base" instances are never tested by the crypto
self-tests, even if there are compatible "ccm" tests.

The root cause of these problems is that instances of the two templates
use different cra_names.  Therefore, fix these problems by making
"ccm_base" instances set the same cra_name as "ccm" instances, e.g.
"ccm(aes)" instead of "ccm_base(ctr(aes-generic),cbcmac(aes-generic))".

This requires extracting the block cipher name from the name of the ctr
and cbcmac algorithms.  It also requires starting to verify that the
algorithms are really ctr and cbcmac using the same block cipher, not
something else entirely.  But it would be bizarre if anyone were
actually using non-ccm-compatible algorithms with ccm_base, so this
shouldn't break anyone in practice.

Fixes: 4a49b499dfa0 ("[CRYPTO] ccm: Added CCM mode")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/ccm.c | 43 +++++++++++++++++--------------------------
 1 file changed, 17 insertions(+), 26 deletions(-)

diff --git a/crypto/ccm.c b/crypto/ccm.c
index 50df8f001c1c9..dbb3b6d8e6136 100644
--- a/crypto/ccm.c
+++ b/crypto/ccm.c
@@ -458,7 +458,6 @@ static void crypto_ccm_free(struct aead_instance *inst)
 
 static int crypto_ccm_create_common(struct crypto_template *tmpl,
 				    struct rtattr **tb,
-				    const char *full_name,
 				    const char *ctr_name,
 				    const char *mac_name)
 {
@@ -486,7 +485,8 @@ static int crypto_ccm_create_common(struct crypto_template *tmpl,
 
 	mac = __crypto_hash_alg_common(mac_alg);
 	err = -EINVAL;
-	if (mac->digestsize != 16)
+	if (strncmp(mac->base.cra_name, "cbcmac(", 7) != 0 ||
+	    mac->digestsize != 16)
 		goto out_put_mac;
 
 	inst = kzalloc(sizeof(*inst) + sizeof(*ictx), GFP_KERNEL);
@@ -509,23 +509,26 @@ static int crypto_ccm_create_common(struct crypto_template *tmpl,
 
 	ctr = crypto_spawn_skcipher_alg(&ictx->ctr);
 
-	/* Not a stream cipher? */
+	/* The skcipher algorithm must be CTR mode, using 16-byte blocks. */
 	err = -EINVAL;
-	if (ctr->base.cra_blocksize != 1)
+	if (strncmp(ctr->base.cra_name, "ctr(", 4) != 0 ||
+	    crypto_skcipher_alg_ivsize(ctr) != 16)
 		goto err_drop_ctr;
 
-	/* We want the real thing! */
-	if (crypto_skcipher_alg_ivsize(ctr) != 16)
+	/* ctr and cbcmac must use the same underlying block cipher. */
+	if (strcmp(ctr->base.cra_name + 4, mac->base.cra_name + 7) != 0)
 		goto err_drop_ctr;
 
 	err = -ENAMETOOLONG;
+	if (snprintf(inst->alg.base.cra_name, CRYPTO_MAX_ALG_NAME,
+		     "ccm(%s", ctr->base.cra_name + 4) >= CRYPTO_MAX_ALG_NAME)
+		goto err_drop_ctr;
+
 	if (snprintf(inst->alg.base.cra_driver_name, CRYPTO_MAX_ALG_NAME,
 		     "ccm_base(%s,%s)", ctr->base.cra_driver_name,
 		     mac->base.cra_driver_name) >= CRYPTO_MAX_ALG_NAME)
 		goto err_drop_ctr;
 
-	memcpy(inst->alg.base.cra_name, full_name, CRYPTO_MAX_ALG_NAME);
-
 	inst->alg.base.cra_flags = ctr->base.cra_flags & CRYPTO_ALG_ASYNC;
 	inst->alg.base.cra_priority = (mac->base.cra_priority +
 				       ctr->base.cra_priority) / 2;
@@ -567,7 +570,6 @@ static int crypto_ccm_create(struct crypto_template *tmpl, struct rtattr **tb)
 	const char *cipher_name;
 	char ctr_name[CRYPTO_MAX_ALG_NAME];
 	char mac_name[CRYPTO_MAX_ALG_NAME];
-	char full_name[CRYPTO_MAX_ALG_NAME];
 
 	cipher_name = crypto_attr_alg_name(tb[1]);
 	if (IS_ERR(cipher_name))
@@ -581,35 +583,24 @@ static int crypto_ccm_create(struct crypto_template *tmpl, struct rtattr **tb)
 		     cipher_name) >= CRYPTO_MAX_ALG_NAME)
 		return -ENAMETOOLONG;
 
-	if (snprintf(full_name, CRYPTO_MAX_ALG_NAME, "ccm(%s)", cipher_name) >=
-	    CRYPTO_MAX_ALG_NAME)
-		return -ENAMETOOLONG;
-
-	return crypto_ccm_create_common(tmpl, tb, full_name, ctr_name,
-					mac_name);
+	return crypto_ccm_create_common(tmpl, tb, ctr_name, mac_name);
 }
 
 static int crypto_ccm_base_create(struct crypto_template *tmpl,
 				  struct rtattr **tb)
 {
 	const char *ctr_name;
-	const char *cipher_name;
-	char full_name[CRYPTO_MAX_ALG_NAME];
+	const char *mac_name;
 
 	ctr_name = crypto_attr_alg_name(tb[1]);
 	if (IS_ERR(ctr_name))
 		return PTR_ERR(ctr_name);
 
-	cipher_name = crypto_attr_alg_name(tb[2]);
-	if (IS_ERR(cipher_name))
-		return PTR_ERR(cipher_name);
-
-	if (snprintf(full_name, CRYPTO_MAX_ALG_NAME, "ccm_base(%s,%s)",
-		     ctr_name, cipher_name) >= CRYPTO_MAX_ALG_NAME)
-		return -ENAMETOOLONG;
+	mac_name = crypto_attr_alg_name(tb[2]);
+	if (IS_ERR(mac_name))
+		return PTR_ERR(mac_name);
 
-	return crypto_ccm_create_common(tmpl, tb, full_name, ctr_name,
-					cipher_name);
+	return crypto_ccm_create_common(tmpl, tb, ctr_name, mac_name);
 }
 
 static int crypto_rfc4309_setkey(struct crypto_aead *parent, const u8 *key,
-- 
2.21.0

Re: [PATCH v2 6/7] crypto: ccm - fix incompatibility between "ccm" and "ccm_base"

[Sasha Levin]

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 4a49b499dfa0 [CRYPTO] ccm: Added CCM mode.

The bot has tested the following trees: v5.0.7, v4.19.34, v4.14.111, v4.9.168, v4.4.178, v3.18.138.

v5.0.7: Build OK!
v4.19.34: Build OK!
v4.14.111: Build OK!
v4.9.168: Failed to apply! Possible dependencies:
    f15f05b0a5de ("crypto: ccm - switch to separate cbcmac driver")

v4.4.178: Failed to apply! Possible dependencies:
    464b93a3c7d1 ("crypto: ccm - Use skcipher")
    f15f05b0a5de ("crypto: ccm - switch to separate cbcmac driver")

v3.18.138: Failed to apply! Possible dependencies:
    2c221ad39424 ("crypto: ccm - Use crypto_aead_set_reqsize helper")
    464b93a3c7d1 ("crypto: ccm - Use skcipher")
    81c4c35eb61a ("crypto: ccm - Convert to new AEAD interface")
    f15f05b0a5de ("crypto: ccm - switch to separate cbcmac driver")


How should we proceed with this patch?

--
Thanks,
Sasha

Re: [PATCH v2 6/7] crypto: ccm - fix incompatibility between "ccm" and "ccm_base"

[Herbert Xu]

Eric Biggers <ebiggers@kernel.org> wrote:
>
> @@ -486,7 +485,8 @@ static int crypto_ccm_create_common(struct crypto_template *tmpl,
> 
>        mac = __crypto_hash_alg_common(mac_alg);
>        err = -EINVAL;
> -       if (mac->digestsize != 16)
> +       if (strncmp(mac->base.cra_name, "cbcmac(", 7) != 0 ||
> +           mac->digestsize != 16)
>                goto out_put_mac;

Keeping the digestsize check is good because names don't mean
much when self-tests are disabled.

> @@ -509,23 +509,26 @@ static int crypto_ccm_create_common(struct crypto_template *tmpl,
> 
>        ctr = crypto_spawn_skcipher_alg(&ictx->ctr);
> 
> -       /* Not a stream cipher? */
> +       /* The skcipher algorithm must be CTR mode, using 16-byte blocks. */
>        err = -EINVAL;
> -       if (ctr->base.cra_blocksize != 1)
> +       if (strncmp(ctr->base.cra_name, "ctr(", 4) != 0 ||
> +           crypto_skcipher_alg_ivsize(ctr) != 16)
>                goto err_drop_ctr;

So we should keep the cra_blocksize test here as well.

Cheers,
-- 
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

[PATCH v2 7/7] crypto: vmx - return correct error code on failed setkey

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

In the VMX implementations of AES and AES modes, return -EINVAL when an
invalid key length is provided, rather than some unusual error code
determined via a series of additions.  This makes the behavior match the
other AES implementations in the kernel's crypto API.

Cc: Daniel Axtens <dja@axtens.net>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 drivers/crypto/vmx/aes.c     | 7 ++++---
 drivers/crypto/vmx/aes_cbc.c | 7 ++++---
 drivers/crypto/vmx/aes_ctr.c | 5 +++--
 drivers/crypto/vmx/aes_xts.c | 9 +++++----
 4 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/crypto/vmx/aes.c b/drivers/crypto/vmx/aes.c
index d7316f7a3a696..b00d6947e02f4 100644
--- a/drivers/crypto/vmx/aes.c
+++ b/drivers/crypto/vmx/aes.c
@@ -78,13 +78,14 @@ static int p8_aes_setkey(struct crypto_tfm *tfm, const u8 *key,
 	pagefault_disable();
 	enable_kernel_vsx();
 	ret = aes_p8_set_encrypt_key(key, keylen * 8, &ctx->enc_key);
-	ret += aes_p8_set_decrypt_key(key, keylen * 8, &ctx->dec_key);
+	ret |= aes_p8_set_decrypt_key(key, keylen * 8, &ctx->dec_key);
 	disable_kernel_vsx();
 	pagefault_enable();
 	preempt_enable();
 
-	ret += crypto_cipher_setkey(ctx->fallback, key, keylen);
-	return ret;
+	ret |= crypto_cipher_setkey(ctx->fallback, key, keylen);
+
+	return ret ? -EINVAL : 0;
 }
 
 static void p8_aes_encrypt(struct crypto_tfm *tfm, u8 *dst, const u8 *src)
diff --git a/drivers/crypto/vmx/aes_cbc.c b/drivers/crypto/vmx/aes_cbc.c
index c5c5ff82b52e0..fbe882ef1bc5d 100644
--- a/drivers/crypto/vmx/aes_cbc.c
+++ b/drivers/crypto/vmx/aes_cbc.c
@@ -81,13 +81,14 @@ static int p8_aes_cbc_setkey(struct crypto_tfm *tfm, const u8 *key,
 	pagefault_disable();
 	enable_kernel_vsx();
 	ret = aes_p8_set_encrypt_key(key, keylen * 8, &ctx->enc_key);
-	ret += aes_p8_set_decrypt_key(key, keylen * 8, &ctx->dec_key);
+	ret |= aes_p8_set_decrypt_key(key, keylen * 8, &ctx->dec_key);
 	disable_kernel_vsx();
 	pagefault_enable();
 	preempt_enable();
 
-	ret += crypto_sync_skcipher_setkey(ctx->fallback, key, keylen);
-	return ret;
+	ret |= crypto_sync_skcipher_setkey(ctx->fallback, key, keylen);
+
+	return ret ? -EINVAL : 0;
 }
 
 static int p8_aes_cbc_encrypt(struct blkcipher_desc *desc,
diff --git a/drivers/crypto/vmx/aes_ctr.c b/drivers/crypto/vmx/aes_ctr.c
index 8a2fe092cb8e0..214c69db9ebdf 100644
--- a/drivers/crypto/vmx/aes_ctr.c
+++ b/drivers/crypto/vmx/aes_ctr.c
@@ -83,8 +83,9 @@ static int p8_aes_ctr_setkey(struct crypto_tfm *tfm, const u8 *key,
 	pagefault_enable();
 	preempt_enable();
 
-	ret += crypto_sync_skcipher_setkey(ctx->fallback, key, keylen);
-	return ret;
+	ret |= crypto_sync_skcipher_setkey(ctx->fallback, key, keylen);
+
+	return ret ? -EINVAL : 0;
 }
 
 static void p8_aes_ctr_final(struct p8_aes_ctr_ctx *ctx,
diff --git a/drivers/crypto/vmx/aes_xts.c b/drivers/crypto/vmx/aes_xts.c
index ecd64e5cc5bbb..5bf4c38566502 100644
--- a/drivers/crypto/vmx/aes_xts.c
+++ b/drivers/crypto/vmx/aes_xts.c
@@ -86,14 +86,15 @@ static int p8_aes_xts_setkey(struct crypto_tfm *tfm, const u8 *key,
 	pagefault_disable();
 	enable_kernel_vsx();
 	ret = aes_p8_set_encrypt_key(key + keylen/2, (keylen/2) * 8, &ctx->tweak_key);
-	ret += aes_p8_set_encrypt_key(key, (keylen/2) * 8, &ctx->enc_key);
-	ret += aes_p8_set_decrypt_key(key, (keylen/2) * 8, &ctx->dec_key);
+	ret |= aes_p8_set_encrypt_key(key, (keylen/2) * 8, &ctx->enc_key);
+	ret |= aes_p8_set_decrypt_key(key, (keylen/2) * 8, &ctx->dec_key);
 	disable_kernel_vsx();
 	pagefault_enable();
 	preempt_enable();
 
-	ret += crypto_sync_skcipher_setkey(ctx->fallback, key, keylen);
-	return ret;
+	ret |= crypto_sync_skcipher_setkey(ctx->fallback, key, keylen);
+
+	return ret ? -EINVAL : 0;
 }
 
 static int p8_aes_xts_crypt(struct blkcipher_desc *desc,
-- 
2.21.0

Re: [PATCH v2 0/7] crypto: fixes in preparation of new fuzz tests

[Ard Biesheuvel]

On Tue, 9 Apr 2019 at 23:48, Eric Biggers <ebiggers@kernel.org> wrote:
>
> This series contains the remaining fixes in preparation of the
> comparison fuzz tests I've proposed in the series
> "crypto: fuzz algorithms against their generic implementation".
>
> I will send a new version of the testmgr patches separately.
>
> These fixes and my latest tests can also be retrieved from git at:
>
>         Repository: https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git
>         Branch: cryptofuzz-vs-generic
>
> Eric Biggers (7):
>   crypto: lrw - don't access already-freed walk.iv
>   crypto: salsa20 - don't access already-freed walk.iv
>   crypto: arm/aes-neonbs - don't access already-freed walk.iv
>   crypto: arm64/aes-neonbs - don't access already-freed walk.iv
>   crypto: gcm - fix incompatibility between "gcm" and "gcm_base"
>   crypto: ccm - fix incompatibility between "ccm" and "ccm_base"
>   crypto: vmx - return correct error code on failed setkey
>

For the series

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

>  arch/arm/crypto/aes-neonbs-glue.c   |  2 ++
>  arch/arm64/crypto/aes-neonbs-glue.c |  2 ++
>  crypto/ccm.c                        | 43 ++++++++++++-----------------
>  crypto/gcm.c                        | 32 ++++++---------------
>  crypto/lrw.c                        |  4 ++-
>  crypto/salsa20_generic.c            |  2 +-
>  drivers/crypto/vmx/aes.c            |  7 +++--
>  drivers/crypto/vmx/aes_cbc.c        |  7 +++--
>  drivers/crypto/vmx/aes_ctr.c        |  5 ++--
>  drivers/crypto/vmx/aes_xts.c        |  9 +++---
>  10 files changed, 50 insertions(+), 63 deletions(-)
>
> --
> 2.21.0
>

Re: [PATCH v2 0/7] crypto: fixes in preparation of new fuzz tests

[Herbert Xu]

Eric Biggers <ebiggers@kernel.org> wrote:
> This series contains the remaining fixes in preparation of the
> comparison fuzz tests I've proposed in the series
> "crypto: fuzz algorithms against their generic implementation".
> 
> I will send a new version of the testmgr patches separately.
> 
> These fixes and my latest tests can also be retrieved from git at:
> 
>        Repository: https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git
>        Branch: cryptofuzz-vs-generic
> 
> Eric Biggers (7):
>  crypto: lrw - don't access already-freed walk.iv
>  crypto: salsa20 - don't access already-freed walk.iv
>  crypto: arm/aes-neonbs - don't access already-freed walk.iv
>  crypto: arm64/aes-neonbs - don't access already-freed walk.iv
>  crypto: gcm - fix incompatibility between "gcm" and "gcm_base"
>  crypto: ccm - fix incompatibility between "ccm" and "ccm_base"
>  crypto: vmx - return correct error code on failed setkey
> 
> arch/arm/crypto/aes-neonbs-glue.c   |  2 ++
> arch/arm64/crypto/aes-neonbs-glue.c |  2 ++
> crypto/ccm.c                        | 43 ++++++++++++-----------------
> crypto/gcm.c                        | 32 ++++++---------------
> crypto/lrw.c                        |  4 ++-
> crypto/salsa20_generic.c            |  2 +-
> drivers/crypto/vmx/aes.c            |  7 +++--
> drivers/crypto/vmx/aes_cbc.c        |  7 +++--
> drivers/crypto/vmx/aes_ctr.c        |  5 ++--
> drivers/crypto/vmx/aes_xts.c        |  9 +++---
> 10 files changed, 50 insertions(+), 63 deletions(-)

Patches 1-4 and 7 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