* [PATCH v2] crypto: ccp - memset structure fields to zero before reuse @ 2019-07-10 0:09 Hook, Gary 2019-07-10 1:57 ` Eric Biggers 2019-07-12 10:18 ` Herbert Xu 0 siblings, 2 replies; 9+ messages in thread From: Hook, Gary @ 2019-07-10 0:09 UTC (permalink / raw) To: linux-crypto; +Cc: herbert, davem, Lendacky, Thomas, Hook, Gary The AES GCM function reuses an 'op' data structure, which members contain values that must be cleared for each (re)use. This fix resolves a crypto self-test failure: alg: aead: gcm-aes-ccp encryption test failed (wrong result) on test vector 2, cfg="two even aligned splits" Fixes: 36cf515b9bbe ("crypto: ccp - Enable support for AES GCM on v5 CCPs") Signed-off-by: Gary R Hook <gary.hook@amd.com> --- Changes since v1: - Explain in the commit message that this fix resolves a failed test drivers/crypto/ccp/ccp-ops.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/drivers/crypto/ccp/ccp-ops.c b/drivers/crypto/ccp/ccp-ops.c index a817f2755c58..9ecc1bb4b237 100644 --- a/drivers/crypto/ccp/ccp-ops.c +++ b/drivers/crypto/ccp/ccp-ops.c @@ -622,6 +622,7 @@ static int ccp_run_aes_gcm_cmd(struct ccp_cmd_queue *cmd_q, unsigned long long *final; unsigned int dm_offset; + unsigned int jobid; unsigned int ilen; bool in_place = true; /* Default value */ int ret; @@ -660,9 +661,11 @@ static int ccp_run_aes_gcm_cmd(struct ccp_cmd_queue *cmd_q, p_tag = scatterwalk_ffwd(sg_tag, p_inp, ilen); } + jobid = CCP_NEW_JOBID(cmd_q->ccp); + memset(&op, 0, sizeof(op)); op.cmd_q = cmd_q; - op.jobid = CCP_NEW_JOBID(cmd_q->ccp); + op.jobid = jobid; op.sb_key = cmd_q->sb_key; /* Pre-allocated */ op.sb_ctx = cmd_q->sb_ctx; /* Pre-allocated */ op.init = 1; @@ -813,6 +816,13 @@ static int ccp_run_aes_gcm_cmd(struct ccp_cmd_queue *cmd_q, final[0] = cpu_to_be64(aes->aad_len * 8); final[1] = cpu_to_be64(ilen * 8); + memset(&op, 0, sizeof(op)); + op.cmd_q = cmd_q; + op.jobid = jobid; + op.sb_key = cmd_q->sb_key; /* Pre-allocated */ + op.sb_ctx = cmd_q->sb_ctx; /* Pre-allocated */ + op.init = 1; + op.u.aes.type = aes->type; op.u.aes.mode = CCP_AES_MODE_GHASH; op.u.aes.action = CCP_AES_GHASHFINAL; op.src.type = CCP_MEMTYPE_SYSTEM; -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] crypto: ccp - memset structure fields to zero before reuse 2019-07-10 0:09 [PATCH v2] crypto: ccp - memset structure fields to zero before reuse Hook, Gary @ 2019-07-10 1:57 ` Eric Biggers 2019-07-10 15:59 ` Gary R Hook 2019-07-12 10:18 ` Herbert Xu 1 sibling, 1 reply; 9+ messages in thread From: Eric Biggers @ 2019-07-10 1:57 UTC (permalink / raw) To: Hook, Gary; +Cc: linux-crypto, herbert, davem, Lendacky, Thomas On Wed, Jul 10, 2019 at 12:09:22AM +0000, Hook, Gary wrote: > The AES GCM function reuses an 'op' data structure, which members > contain values that must be cleared for each (re)use. > > This fix resolves a crypto self-test failure: > alg: aead: gcm-aes-ccp encryption test failed (wrong result) on test vector 2, cfg="two even aligned splits" > > Fixes: 36cf515b9bbe ("crypto: ccp - Enable support for AES GCM on v5 CCPs") > > Signed-off-by: Gary R Hook <gary.hook@amd.com> FYI, with this patch applied I'm still seeing another test failure: [ 2.140227] alg: aead: gcm-aes-ccp setauthsize unexpectedly succeeded on test vector "random: alen=264 plen=161 authsize=6 klen=32"; expected_error=-22 Are you aware of that one too, and are you planning to fix it? - Eric ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] crypto: ccp - memset structure fields to zero before reuse 2019-07-10 1:57 ` Eric Biggers @ 2019-07-10 15:59 ` Gary R Hook 2019-07-10 20:34 ` Eric Biggers 0 siblings, 1 reply; 9+ messages in thread From: Gary R Hook @ 2019-07-10 15:59 UTC (permalink / raw) To: Hook, Gary, linux-crypto, herbert, davem, Lendacky, Thomas On 7/9/19 8:57 PM, Eric Biggers wrote: > On Wed, Jul 10, 2019 at 12:09:22AM +0000, Hook, Gary wrote: >> The AES GCM function reuses an 'op' data structure, which members >> contain values that must be cleared for each (re)use. >> >> This fix resolves a crypto self-test failure: >> alg: aead: gcm-aes-ccp encryption test failed (wrong result) on test vector 2, cfg="two even aligned splits" >> >> Fixes: 36cf515b9bbe ("crypto: ccp - Enable support for AES GCM on v5 CCPs") >> >> Signed-off-by: Gary R Hook <gary.hook@amd.com> > > FYI, with this patch applied I'm still seeing another test failure: > > [ 2.140227] alg: aead: gcm-aes-ccp setauthsize unexpectedly succeeded on test vector "random: alen=264 plen=161 authsize=6 klen=32"; expected_error=-22 > > Are you aware of that one too, and are you planning to fix it? > > - Eric > I just pulled the latest on the master branch of cryptodev-2.6, built, booted, and loaded our module. And I don't see that error. It must be new? In any event, if a test failure occurs, it gets fixed. grh ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] crypto: ccp - memset structure fields to zero before reuse 2019-07-10 15:59 ` Gary R Hook @ 2019-07-10 20:34 ` Eric Biggers 2019-07-10 22:50 ` Gary R Hook 0 siblings, 1 reply; 9+ messages in thread From: Eric Biggers @ 2019-07-10 20:34 UTC (permalink / raw) To: Gary R Hook; +Cc: Hook, Gary, linux-crypto, herbert, davem, Lendacky, Thomas On Wed, Jul 10, 2019 at 03:59:05PM +0000, Gary R Hook wrote: > On 7/9/19 8:57 PM, Eric Biggers wrote: > > On Wed, Jul 10, 2019 at 12:09:22AM +0000, Hook, Gary wrote: > >> The AES GCM function reuses an 'op' data structure, which members > >> contain values that must be cleared for each (re)use. > >> > >> This fix resolves a crypto self-test failure: > >> alg: aead: gcm-aes-ccp encryption test failed (wrong result) on test vector 2, cfg="two even aligned splits" > >> > >> Fixes: 36cf515b9bbe ("crypto: ccp - Enable support for AES GCM on v5 CCPs") > >> > >> Signed-off-by: Gary R Hook <gary.hook@amd.com> > > > > FYI, with this patch applied I'm still seeing another test failure: > > > > [ 2.140227] alg: aead: gcm-aes-ccp setauthsize unexpectedly succeeded on test vector "random: alen=264 plen=161 authsize=6 klen=32"; expected_error=-22 > > > > Are you aware of that one too, and are you planning to fix it? > > > > - Eric > > > > I just pulled the latest on the master branch of cryptodev-2.6, built, > booted, and loaded our module. And I don't see that error. It must be new? Did you have CONFIG_CRYPTO_MANAGER_EXTRA_TESTS enabled? This failure was with a test vector that was generated randomly by the fuzz tests, so CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y is needed to reproduce it. You probably just need to update ccp_aes_gcm_setauthsize() to validate the authentication tag size. > > In any event, if a test failure occurs, it gets fixed. > Good to hear. - Eric ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] crypto: ccp - memset structure fields to zero before reuse 2019-07-10 20:34 ` Eric Biggers @ 2019-07-10 22:50 ` Gary R Hook 2019-07-11 0:46 ` Eric Biggers 0 siblings, 1 reply; 9+ messages in thread From: Gary R Hook @ 2019-07-10 22:50 UTC (permalink / raw) To: Hook, Gary, linux-crypto, herbert, davem, Lendacky, Thomas On 7/10/19 3:34 PM, Eric Biggers wrote: > On Wed, Jul 10, 2019 at 03:59:05PM +0000, Gary R Hook wrote: >> On 7/9/19 8:57 PM, Eric Biggers wrote: >>> On Wed, Jul 10, 2019 at 12:09:22AM +0000, Hook, Gary wrote: >>>> The AES GCM function reuses an 'op' data structure, which members >>>> contain values that must be cleared for each (re)use. >>>> >>>> This fix resolves a crypto self-test failure: >>>> alg: aead: gcm-aes-ccp encryption test failed (wrong result) on test vector 2, cfg="two even aligned splits" >>>> >>>> Fixes: 36cf515b9bbe ("crypto: ccp - Enable support for AES GCM on v5 CCPs") >>>> >>>> Signed-off-by: Gary R Hook <gary.hook@amd.com> >>> >>> FYI, with this patch applied I'm still seeing another test failure: >>> >>> [ 2.140227] alg: aead: gcm-aes-ccp setauthsize unexpectedly succeeded on test vector "random: alen=264 plen=161 authsize=6 klen=32"; expected_error=-22 >>> >>> Are you aware of that one too, and are you planning to fix it? >>> >>> - Eric >>> >> >> I just pulled the latest on the master branch of cryptodev-2.6, built, >> booted, and loaded our module. And I don't see that error. It must be new? > > Did you have CONFIG_CRYPTO_MANAGER_EXTRA_TESTS enabled? This failure was with a > test vector that was generated randomly by the fuzz tests, so > CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y is needed to reproduce it. > > You probably just need to update ccp_aes_gcm_setauthsize() to validate the > authentication tag size. Now I'm confused. I did need to fix that function, and AFAIK the tag for GCM is always going to be 16 bytes (our AES_BLOCK_SIZE). So, after making the small change, the above test passes, but now I progress and get this error: [ 1640.820781] alg: aead: gcm-aes-ccp setauthsize failed on test vector "random: alen=29 plen=29 authsize=12 klen=32"; expected_error=0, actual_error=1 Which is wholly unclear. Why would an authsize of 12 be okay for this transformation? The GCM tag is a fixed size. Nothing in the AEAD documentation jumps out at me. As I don't profess to be a crypto expert, I'd appreciate any guidance on this subtle issue that is eluding me... Thanks grh ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] crypto: ccp - memset structure fields to zero before reuse 2019-07-10 22:50 ` Gary R Hook @ 2019-07-11 0:46 ` Eric Biggers 2019-07-11 15:25 ` Gary R Hook 0 siblings, 1 reply; 9+ messages in thread From: Eric Biggers @ 2019-07-11 0:46 UTC (permalink / raw) To: Gary R Hook; +Cc: Hook, Gary, linux-crypto, herbert, davem, Lendacky, Thomas On Wed, Jul 10, 2019 at 10:50:31PM +0000, Gary R Hook wrote: > On 7/10/19 3:34 PM, Eric Biggers wrote: > > On Wed, Jul 10, 2019 at 03:59:05PM +0000, Gary R Hook wrote: > >> On 7/9/19 8:57 PM, Eric Biggers wrote: > >>> On Wed, Jul 10, 2019 at 12:09:22AM +0000, Hook, Gary wrote: > >>>> The AES GCM function reuses an 'op' data structure, which members > >>>> contain values that must be cleared for each (re)use. > >>>> > >>>> This fix resolves a crypto self-test failure: > >>>> alg: aead: gcm-aes-ccp encryption test failed (wrong result) on test vector 2, cfg="two even aligned splits" > >>>> > >>>> Fixes: 36cf515b9bbe ("crypto: ccp - Enable support for AES GCM on v5 CCPs") > >>>> > >>>> Signed-off-by: Gary R Hook <gary.hook@amd.com> > >>> > >>> FYI, with this patch applied I'm still seeing another test failure: > >>> > >>> [ 2.140227] alg: aead: gcm-aes-ccp setauthsize unexpectedly succeeded on test vector "random: alen=264 plen=161 authsize=6 klen=32"; expected_error=-22 > >>> > >>> Are you aware of that one too, and are you planning to fix it? > >>> > >>> - Eric > >>> > >> > >> I just pulled the latest on the master branch of cryptodev-2.6, built, > >> booted, and loaded our module. And I don't see that error. It must be new? > > > > Did you have CONFIG_CRYPTO_MANAGER_EXTRA_TESTS enabled? This failure was with a > > test vector that was generated randomly by the fuzz tests, so > > CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y is needed to reproduce it. > > > > You probably just need to update ccp_aes_gcm_setauthsize() to validate the > > authentication tag size. > > Now I'm confused. I did need to fix that function, and AFAIK the tag for > GCM is always going to be 16 bytes (our AES_BLOCK_SIZE). > > So, after making the small change, the above test passes, but now I > progress and get this error: > > [ 1640.820781] alg: aead: gcm-aes-ccp setauthsize failed on test vector > "random: alen=29 plen=29 authsize=12 klen=32"; expected_error=0, > actual_error=1 > > Which is wholly unclear. Why would an authsize of 12 be okay for this > transformation? The GCM tag is a fixed size. > > Nothing in the AEAD documentation jumps out at me. As I don't profess to > be a crypto expert, I'd appreciate any guidance on this subtle issue > that is eluding me... > The generic implementation allows authentication tags of 4, 8, 12, 13, 14, 15, or 16 bytes. See crypto_gcm_setauthsize() in crypto/gcm.c, and see https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf section 5.2.1.2 "Output Data". If you disagree that this is the correct behavior, then we need to fix the generic implementation too. Also, I just got a kernel panic in the CCP driver on boot with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS enabled. The kernel included this patch. So apparently there's yet another bug that needs to be fixed. This one must occur infrequently enough to not be hit every time with the default fuzz_iterations=100, but still frequently enough for me to have seen it. - Eric ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] crypto: ccp - memset structure fields to zero before reuse 2019-07-11 0:46 ` Eric Biggers @ 2019-07-11 15:25 ` Gary R Hook 2019-07-12 2:20 ` Herbert Xu 0 siblings, 1 reply; 9+ messages in thread From: Gary R Hook @ 2019-07-11 15:25 UTC (permalink / raw) To: Hook, Gary, linux-crypto, herbert, davem, Lendacky, Thomas On 7/10/19 7:46 PM, Eric Biggers wrote: > > The generic implementation allows authentication tags of 4, 8, 12, 13, 14, 15, > or 16 bytes. See crypto_gcm_setauthsize() in crypto/gcm.c, and see > https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf > section 5.2.1.2 "Output Data". If you disagree that this is the correct > behavior, then we need to fix the generic implementation too. It's been a while, and the refresher was needed, and is appreciated. Our device only allows 16 byte tags. So I have to figure out how to set up the driver to expose/enforce that limitation. That's where we go awry. Thanks much! ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] crypto: ccp - memset structure fields to zero before reuse 2019-07-11 15:25 ` Gary R Hook @ 2019-07-12 2:20 ` Herbert Xu 0 siblings, 0 replies; 9+ messages in thread From: Herbert Xu @ 2019-07-12 2:20 UTC (permalink / raw) To: Gary R Hook; +Cc: Hook, Gary, linux-crypto, davem, Lendacky, Thomas On Thu, Jul 11, 2019 at 03:25:00PM +0000, Gary R Hook wrote: > > Our device only allows 16 byte tags. So I have to figure out how to set > up the driver to expose/enforce that limitation. That's where we go awry. You need to work around it by using a fallback. 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] crypto: ccp - memset structure fields to zero before reuse 2019-07-10 0:09 [PATCH v2] crypto: ccp - memset structure fields to zero before reuse Hook, Gary 2019-07-10 1:57 ` Eric Biggers @ 2019-07-12 10:18 ` Herbert Xu 1 sibling, 0 replies; 9+ messages in thread From: Herbert Xu @ 2019-07-12 10:18 UTC (permalink / raw) To: Hook, Gary; +Cc: linux-crypto, davem, Lendacky, Thomas On Wed, Jul 10, 2019 at 12:09:22AM +0000, Hook, Gary wrote: > The AES GCM function reuses an 'op' data structure, which members > contain values that must be cleared for each (re)use. > > This fix resolves a crypto self-test failure: > alg: aead: gcm-aes-ccp encryption test failed (wrong result) on test vector 2, cfg="two even aligned splits" > > Fixes: 36cf515b9bbe ("crypto: ccp - Enable support for AES GCM on v5 CCPs") > > Signed-off-by: Gary R Hook <gary.hook@amd.com> > --- > > Changes since v1: > - Explain in the commit message that this fix resolves a failed test > > drivers/crypto/ccp/ccp-ops.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-07-12 10:18 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-10 0:09 [PATCH v2] crypto: ccp - memset structure fields to zero before reuse Hook, Gary 2019-07-10 1:57 ` Eric Biggers 2019-07-10 15:59 ` Gary R Hook 2019-07-10 20:34 ` Eric Biggers 2019-07-10 22:50 ` Gary R Hook 2019-07-11 0:46 ` Eric Biggers 2019-07-11 15:25 ` Gary R Hook 2019-07-12 2:20 ` Herbert Xu 2019-07-12 10:18 ` Herbert Xu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.