[PATCH] crypto/testmgr: don't copy from source IV too much

[PATCH] crypto/testmgr: don't copy from source IV too much

[Andrey Ryabinin]

While the destination buffer 'iv' is MAX_IVLEN size,
the source 'template[i].iv' could be smaller. Thus
copying it via memcpy() leads to invalid memory access.
Use strlcpy() instead.

Signed-off-by: Andrey Ryabinin <aryabinin@odin.com>
---
 crypto/testmgr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index d0a42bd..d85221c 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -974,7 +974,7 @@ static int __test_skcipher(struct crypto_ablkcipher *tfm, int enc,
 			continue;
 
 		if (template[i].iv)
-			memcpy(iv, template[i].iv, MAX_IVLEN);
+			strlcpy(iv, template[i].iv, MAX_IVLEN);
 		else
 			memset(iv, 0, MAX_IVLEN);
 
@@ -1049,7 +1049,7 @@ static int __test_skcipher(struct crypto_ablkcipher *tfm, int enc,
 			continue;
 
 		if (template[i].iv)
-			memcpy(iv, template[i].iv, MAX_IVLEN);
+			strlcpy(iv, template[i].iv, MAX_IVLEN);
 		else
 			memset(iv, 0, MAX_IVLEN);
 
-- 
2.4.6

[PATCH] x86/crypto/ghash-intel: specify context size for ghash async algorithm

[Andrey Ryabinin]

Currently context size (cra_ctxsize) doesn't specified for
ghash_async_alg. Which means it's zero. Thus crypto_create_tfm()
doesn't allocate needed space for ghash_async_ctx, so any
read/write to ctx (e.g. in ghash_async_init_tfm()) is not valid.

Signed-off-by: Andrey Ryabinin <aryabinin@odin.com>
---
 arch/x86/crypto/ghash-clmulni-intel_glue.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/crypto/ghash-clmulni-intel_glue.c b/arch/x86/crypto/ghash-clmulni-intel_glue.c
index 64d7cf1..440df0c 100644
--- a/arch/x86/crypto/ghash-clmulni-intel_glue.c
+++ b/arch/x86/crypto/ghash-clmulni-intel_glue.c
@@ -294,6 +294,7 @@ static struct ahash_alg ghash_async_alg = {
 			.cra_name		= "ghash",
 			.cra_driver_name	= "ghash-clmulni",
 			.cra_priority		= 400,
+			.cra_ctxsize		= sizeof(struct ghash_async_ctx),
 			.cra_flags		= CRYPTO_ALG_TYPE_AHASH | CRYPTO_ALG_ASYNC,
 			.cra_blocksize		= GHASH_BLOCK_SIZE,
 			.cra_type		= &crypto_ahash_type,
-- 
2.4.6

Re: [PATCH] x86/crypto/ghash-intel: specify context size for ghash async algorithm

[Herbert Xu]

On Thu, Sep 03, 2015 at 02:32:01PM +0300, Andrey Ryabinin wrote:
> Currently context size (cra_ctxsize) doesn't specified for
> ghash_async_alg. Which means it's zero. Thus crypto_create_tfm()
> doesn't allocate needed space for ghash_async_ctx, so any
> read/write to ctx (e.g. in ghash_async_init_tfm()) is not valid.
> 
> Signed-off-by: Andrey Ryabinin <aryabinin@odin.com>

Patch 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

Re: [PATCH] crypto/testmgr: don't copy from source IV too much

[Herbert Xu]

On Thu, Sep 03, 2015 at 02:32:00PM +0300, Andrey Ryabinin wrote:
> While the destination buffer 'iv' is MAX_IVLEN size,
> the source 'template[i].iv' could be smaller. Thus
> copying it via memcpy() leads to invalid memory access.
> Use strlcpy() instead.
> 
> Signed-off-by: Andrey Ryabinin <aryabinin@odin.com>

Thanks for the patch.  Unfortunately it's broken because the IV
is not a string and can contain NULs.  So either fix it by using
the real ivsize, or change template[i].iv to a char array.

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] crypto/testmgr: don't copy from source IV too much

[Andrey Ryabinin]

On 09/03/2015 04:20 PM, Herbert Xu wrote:
> On Thu, Sep 03, 2015 at 02:32:00PM +0300, Andrey Ryabinin wrote:
>> While the destination buffer 'iv' is MAX_IVLEN size,
>> the source 'template[i].iv' could be smaller. Thus
>> copying it via memcpy() leads to invalid memory access.
>> Use strlcpy() instead.
>>
>> Signed-off-by: Andrey Ryabinin <aryabinin@odin.com>
> 
> Thanks for the patch.  Unfortunately it's broken because the IV
> is not a string and can contain NULs.  So either fix it by using
> the real ivsize,

So I've tried to use crypto_ablkcipher_ivsize(tfm) for that.

But noticed that some algs don't set ivsize (which makes it zero).
E.g. "ecb-cast6-avx" doesn't set it, but test vectors (cast6_enc_tv_template[], cast6_dec_tv_template[])
have .iv of 16bytes.

So I'm not sure what part is wrong here.
Is it wrong to use crypto_ablkcipher_ivsize(tfm) to get ivsize here?
Is it bug in 'ecb-cast6-avx'?
Or maybe something else?


> or change template[i].iv to a char array.
> 
> Cheers,
> 

Re: [PATCH] crypto/testmgr: don't copy from source IV too much

[Herbert Xu]

On Fri, Sep 04, 2015 at 07:42:26PM +0300, Andrey Ryabinin wrote:
> 
> But noticed that some algs don't set ivsize (which makes it zero).
> E.g. "ecb-cast6-avx" doesn't set it, but test vectors (cast6_enc_tv_template[], cast6_dec_tv_template[])
> have .iv of 16bytes.

ECB should always have an IV size of zero so this is correct.
If the test vectors contain an IV it'll simply be ignored.

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] crypto/testmgr: don't copy from source IV too much

[Andrey Ryabinin]

While the destination buffer 'iv' is MAX_IVLEN size,
the source 'template[i].iv' could be smaller, thus
memcpy may read read invalid memory.
Use crypto_skcipher_ivsize() to get real ivsize
and pass it to memcpy.

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 crypto/testmgr.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 35c2de1..fa18753 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -940,6 +940,7 @@ static int __test_skcipher(struct crypto_skcipher *tfm, int enc,
 	char *xbuf[XBUFSIZE];
 	char *xoutbuf[XBUFSIZE];
 	int ret = -ENOMEM;
+	unsigned int ivsize = crypto_skcipher_ivsize(tfm);
 
 	if (testmgr_alloc_buf(xbuf))
 		goto out_nobuf;
@@ -975,7 +976,7 @@ static int __test_skcipher(struct crypto_skcipher *tfm, int enc,
 			continue;
 
 		if (template[i].iv)
-			memcpy(iv, template[i].iv, MAX_IVLEN);
+			memcpy(iv, template[i].iv, ivsize);
 		else
 			memset(iv, 0, MAX_IVLEN);
 
@@ -1051,7 +1052,7 @@ static int __test_skcipher(struct crypto_skcipher *tfm, int enc,
 			continue;
 
 		if (template[i].iv)
-			memcpy(iv, template[i].iv, MAX_IVLEN);
+			memcpy(iv, template[i].iv, ivsize);
 		else
 			memset(iv, 0, MAX_IVLEN);
 
-- 
2.4.6

Re: [PATCH v2] crypto/testmgr: don't copy from source IV too much

[Herbert Xu]

On Thu, Sep 10, 2015 at 01:11:55PM +0300, Andrey Ryabinin wrote:
> While the destination buffer 'iv' is MAX_IVLEN size,
> the source 'template[i].iv' could be smaller, thus
> memcpy may read read invalid memory.
> Use crypto_skcipher_ivsize() to get real ivsize
> and pass it to memcpy.
> 
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>

Patch 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