[PATCH v2] crypto: ccp - memset structure fields to zero before reuse

[PATCH v2] crypto: ccp - memset structure fields to zero before reuse

[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

Re: [PATCH v2] crypto: ccp - memset structure fields to zero before reuse

[Eric Biggers]

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

Re: [PATCH v2] crypto: ccp - memset structure fields to zero before reuse

[Gary R Hook]

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

Re: [PATCH v2] crypto: ccp - memset structure fields to zero before reuse

[Eric Biggers]

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

Re: [PATCH v2] crypto: ccp - memset structure fields to zero before reuse

[Gary R Hook]

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

Re: [PATCH v2] crypto: ccp - memset structure fields to zero before reuse

[Eric Biggers]

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

Re: [PATCH v2] crypto: ccp - memset structure fields to zero before reuse

[Gary R Hook]

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!

Re: [PATCH v2] crypto: ccp - memset structure fields to zero before reuse

[Herbert Xu]

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

Re: [PATCH v2] crypto: ccp - memset structure fields to zero before reuse

[Herbert Xu]

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