[PATCH] crypto: qat - Fix for qat_aes_cbc_hmac_sha512

[PATCH] crypto: qat - Fix for qat_aes_cbc_hmac_sha512

[Tadeusz Struk]

After commit ad511e2 the qat_aes_cbc_hmac_sha512 stopped working:

alg: aead: Test 1 failed on encryption for qat_aes_cbc_hmac_sha512.
00000000: e3 53 77 9c 10 79 ae b8 27 08 94 2d be 77 18 1a
00000010: 94 8a 3a b4 70 5d 3b e5 89 f9 35 14 e5 3f dc 9b
00000020: 45 a2 a9 0b 95 eb 23 0a 81 d8 44 5c 0d 30 90 b8
00000030: 1e c6 de 20 23 66 c3 1f 5f 19 ce f2 f8 10 38 66
00000040: fc e7 1c 47 88 cf c3 34 0c 28 16 4e 17 d1 d0 75

We need to explicitly clean the rest of the context buffer
to make it working again.

Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
 drivers/crypto/qat/qat_common/qat_algs.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/crypto/qat/qat_common/qat_algs.c b/drivers/crypto/qat/qat_common/qat_algs.c
index a0d95f3..1c2f259 100644
--- a/drivers/crypto/qat/qat_common/qat_algs.c
+++ b/drivers/crypto/qat/qat_common/qat_algs.c
@@ -186,10 +186,14 @@ static int qat_alg_do_precomputes(struct icp_qat_hw_auth_algo_blk *hash,
 
 		memcpy(ipad, buff, digest_size);
 		memcpy(opad, buff, digest_size);
+		memset(ipad + digest_size, 0, block_size - digest_size);
+		memset(opad + digest_size, 0, block_size - digest_size);
 		memzero_explicit(buff, sizeof(buff));
 	} else {
 		memcpy(ipad, auth_key, auth_keylen);
 		memcpy(opad, auth_key, auth_keylen);
+		memset(ipad + auth_keylen, 0, block_size - auth_keylen);
+		memset(opad + auth_keylen, 0, block_size - auth_keylen);
 	}
 
 	for (i = 0; i < block_size; i++) {

Re: [PATCH] crypto: qat - Fix for qat_aes_cbc_hmac_sha512

[Herbert Xu]

On Tue, Jan 13, 2015 at 12:27:53PM -0800, Tadeusz Struk wrote:
> After commit ad511e2 the qat_aes_cbc_hmac_sha512 stopped working:
> 
> alg: aead: Test 1 failed on encryption for qat_aes_cbc_hmac_sha512.
> 00000000: e3 53 77 9c 10 79 ae b8 27 08 94 2d be 77 18 1a
> 00000010: 94 8a 3a b4 70 5d 3b e5 89 f9 35 14 e5 3f dc 9b
> 00000020: 45 a2 a9 0b 95 eb 23 0a 81 d8 44 5c 0d 30 90 b8
> 00000030: 1e c6 de 20 23 66 c3 1f 5f 19 ce f2 f8 10 38 66
> 00000040: fc e7 1c 47 88 cf c3 34 0c 28 16 4e 17 d1 d0 75
> 
> We need to explicitly clean the rest of the context buffer
> to make it working again.
> 
> Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>

The fact that you need this patch indicates something is seriously
wrong.

> ---
>  drivers/crypto/qat/qat_common/qat_algs.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/crypto/qat/qat_common/qat_algs.c b/drivers/crypto/qat/qat_common/qat_algs.c
> index a0d95f3..1c2f259 100644
> --- a/drivers/crypto/qat/qat_common/qat_algs.c
> +++ b/drivers/crypto/qat/qat_common/qat_algs.c
> @@ -186,10 +186,14 @@ static int qat_alg_do_precomputes(struct icp_qat_hw_auth_algo_blk *hash,
>  
>  		memcpy(ipad, buff, digest_size);
>  		memcpy(opad, buff, digest_size);
> +		memset(ipad + digest_size, 0, block_size - digest_size);
> +		memset(opad + digest_size, 0, block_size - digest_size);
>  		memzero_explicit(buff, sizeof(buff));

The very first thing we do in that function is zero the whole
auth_state.  So why would we need to zero it here? The only thin
I can think of is if auth_state is too small and we're encountering
garbage on the stack which would be a serious bug.

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: qat - Fix for qat_aes_cbc_hmac_sha512

[Tadeusz Struk]

Hi Herbert,
On 01/13/2015 01:25 PM, Herbert Xu wrote:
>>  		memcpy(ipad, buff, digest_size);
>> >  		memcpy(opad, buff, digest_size);
>> > +		memset(ipad + digest_size, 0, block_size - digest_size);
>> > +		memset(opad + digest_size, 0, block_size - digest_size);
>> >  		memzero_explicit(buff, sizeof(buff));
> The very first thing we do in that function is zero the whole
> auth_state.  So why would we need to zero it here? The only thin
> I can think of is if auth_state is too small and we're encountering
> garbage on the stack which would be a serious bug.

Yes, it looks strange, but the issue is we don't really zero the whole
auth_state. Because struct qat_auth_state is no packed on my system

sizeof(MAX_AUTH_STATE_SIZE + 64) = 244

and sizeof(struct qat_auth_state) = 256

if instead of:

memzero_explicit(auth_state.data, MAX_AUTH_STATE_SIZE + 64);

it would be:

memzero_explicit(&auth_state, sizeof(auth_state));

then it would work as well.
I can send another patch that does the second if you like.
Thanks,
Tadeusz

Re: [PATCH] crypto: qat - Fix for qat_aes_cbc_hmac_sha512

[Herbert Xu]

On Tue, Jan 13, 2015 at 02:21:58PM -0800, Tadeusz Struk wrote:
> 
> Yes, it looks strange, but the issue is we don't really zero the whole
> auth_state. Because struct qat_auth_state is no packed on my system
> 
> sizeof(MAX_AUTH_STATE_SIZE + 64) = 244
> 
> and sizeof(struct qat_auth_state) = 256

That's seriously wrong.  What if the compiler decided to not add
that padding? You will be scribbling all over the stack.

Why are you allocating this qat_auth_state anyway? Just do
ipad/opad[block_size] and be done with it.

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: qat - Fix for qat_aes_cbc_hmac_sha512

[Tadeusz Struk]

On 01/13/2015 02:47 PM, Herbert Xu wrote:
> Why are you allocating this qat_auth_state anyway? Just do
> ipad/opad[block_size] and be done with it.

You are right. I don't need qat_auth_state really. Let me send you v2 in
2 mins.
Thanks

crypto: qat - Ensure ipad and opad are zeroed

[Herbert Xu]

On Tue, Jan 13, 2015 at 02:55:24PM -0800, Tadeusz Struk wrote:
> On 01/13/2015 02:47 PM, Herbert Xu wrote:
> > Why are you allocating this qat_auth_state anyway? Just do
> > ipad/opad[block_size] and be done with it.
> 
> You are right. I don't need qat_auth_state really. Let me send you v2 in
> 2 mins.

Don't worry, I fixed it for you.

-- >8 --
The patch ad511e260a27b8e35d273cc0ecfe5a8ff9543181 (crypto: qat -
Fix incorrect uses of memzero_explicit) broke hashing because the
code was in fact overwriting the qat_auth_state variable.

In fact there is no reason for the variable to exist anyway since
all we are using it for is to store ipad and opad.  So we could
simply create ipad and opad directly and avoid this whole mess.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/drivers/crypto/qat/qat_common/qat_algs.c b/drivers/crypto/qat/qat_common/qat_algs.c
index a0d95f3..e2c4b25 100644
--- a/drivers/crypto/qat/qat_common/qat_algs.c
+++ b/drivers/crypto/qat/qat_common/qat_algs.c
@@ -160,33 +160,30 @@ static int qat_alg_do_precomputes(struct icp_qat_hw_auth_algo_blk *hash,
 				  const uint8_t *auth_key,
 				  unsigned int auth_keylen)
 {
-	struct qat_auth_state auth_state;
 	SHASH_DESC_ON_STACK(shash, ctx->hash_tfm);
 	struct sha1_state sha1;
 	struct sha256_state sha256;
 	struct sha512_state sha512;
 	int block_size = crypto_shash_blocksize(ctx->hash_tfm);
 	int digest_size = crypto_shash_digestsize(ctx->hash_tfm);
-	uint8_t *ipad = auth_state.data;
-	uint8_t *opad = ipad + block_size;
+	char ipad[block_size];
+	char opad[block_size];
 	__be32 *hash_state_out;
 	__be64 *hash512_state_out;
 	int i, offset;
 
-	memset(auth_state.data, 0, sizeof(auth_state.data));
+	memset(ipad, 0, block_size);
+	memset(opad, 0, block_size);
 	shash->tfm = ctx->hash_tfm;
 	shash->flags = 0x0;
 
 	if (auth_keylen > block_size) {
-		char buff[SHA512_BLOCK_SIZE];
 		int ret = crypto_shash_digest(shash, auth_key,
-					      auth_keylen, buff);
+					      auth_keylen, ipad);
 		if (ret)
 			return ret;
 
-		memcpy(ipad, buff, digest_size);
-		memcpy(opad, buff, digest_size);
-		memzero_explicit(buff, sizeof(buff));
+		memcpy(opad, ipad, digest_size);
 	} else {
 		memcpy(ipad, auth_key, auth_keylen);
 		memcpy(opad, auth_key, auth_keylen);

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: crypto: qat - Ensure ipad and opad are zeroed

[Tadeusz Struk]

On 01/13/2015 03:07 PM, Herbert Xu wrote:
> The patch ad511e260a27b8e35d273cc0ecfe5a8ff9543181 (crypto: qat -
> Fix incorrect uses of memzero_explicit) broke hashing because the
> code was in fact overwriting the qat_auth_state variable.
> 
> In fact there is no reason for the variable to exist anyway since
> all we are using it for is to store ipad and opad.  So we could
> simply create ipad and opad directly and avoid this whole mess.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Ok that looks better. Thanks. I think we should also remove the
definition of struct qat_auth_state in line 107 and the #def above.
Regards,
Tadeusz