| Message ID | 20150113202753.32216.26250.stgit@tstruk-mobl1 (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
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,
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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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++) {
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(+) -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html