[v2] crypto: ccp - memset structure fields to zero before reuse
diff mbox series

Message ID 20190710000849.3131-1-gary.hook@amd.com
State Accepted
Delegated to: Herbert Xu
Headers show
Series
  • [v2] crypto: ccp - memset structure fields to zero before reuse
Related show

Commit Message

Gary R Hook July 10, 2019, 12:09 a.m. UTC
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(-)

Comments

Eric Biggers July 10, 2019, 1:57 a.m. UTC | #1
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
Gary R Hook July 10, 2019, 3:59 p.m. UTC | #2
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
Eric Biggers July 10, 2019, 8:34 p.m. UTC | #3
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
Gary R Hook July 10, 2019, 10:50 p.m. UTC | #4
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
Eric Biggers July 11, 2019, 12:46 a.m. UTC | #5
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
Gary R Hook July 11, 2019, 3:25 p.m. UTC | #6
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!
Herbert Xu July 12, 2019, 2:20 a.m. UTC | #7
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,
Herbert Xu July 12, 2019, 10:18 a.m. UTC | #8
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.

Patch
diff mbox series

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;