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

[Eric Biggers <ebiggers@kernel.org>]

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