Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 17 Oct 2022 11:20:18 -0700
From:   Eric Biggers <ebiggers@kernel.org>
To:     Ard Biesheuvel <ardb@kernel.org>
Cc:     linux-crypto@vger.kernel.org, keescook@chromium.org,
        jason@zx2c4.com, herbert@gondor.apana.org.au,
        Nikunj A Dadhania <nikunj@amd.com>
Subject: Re: [PATCH v2] crypto: gcmaes - Provide minimal library
 implementation
Message-ID: <Y02c4kfTIj4XZxNV@sol.localdomain>
References: <20221014104713.2613195-1-ardb@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221014104713.2613195-1-ardb@kernel.org>
Precedence: bulk

On Fri, Oct 14, 2022 at 12:47:13PM +0200, Ard Biesheuvel wrote:
> Note that table based AES implementations are susceptible to known
> plaintext timing attacks on the encryption key. The AES library already
> attempts to mitigate this to some extent, but given that the counter
> mode encryption used by GCM operates exclusively on known plaintext by
> construction (the IV and therefore the initial counter value are known
> to an attacker), let's take some extra care to mitigate this, by calling
> the AES library with interrupts disabled.

Note that crypto/gf128mul.c has no mitigations against timing attacks.  I take
it that is something that needs to be tolerated here?

> diff --git a/include/crypto/gcm.h b/include/crypto/gcm.h
> index 9d7eff04f224..dfbc381df5ae 100644
> --- a/include/crypto/gcm.h
> +++ b/include/crypto/gcm.h
> @@ -3,6 +3,9 @@
>  
>  #include <linux/errno.h>
>  
> +#include <crypto/aes.h>
> +#include <crypto/gf128mul.h>
> +
>  #define GCM_AES_IV_SIZE 12
>  #define GCM_RFC4106_IV_SIZE 8
>  #define GCM_RFC4543_IV_SIZE 8
> @@ -60,4 +63,67 @@ static inline int crypto_ipsec_check_assoclen(unsigned int assoclen)
>  
>  	return 0;
>  }
> +
> +struct gcmaes_ctx {
> +	be128			ghash_key;
> +	struct crypto_aes_ctx	aes_ctx;
> +	unsigned int		authsize;
> +};
> +
> +/**
> + * gcmaes_expandkey - Expands the AES and GHASH keys for the GCM-AES key
> + * 		      schedule
> + *
> + * @ctx:	The data structure that will hold the GCM-AES key schedule
> + * @key:	The AES encryption input key
> + * @keysize:	The length in bytes of the input key
> + * @authsize:	The size in bytes of the GCM authentication tag
> + *
> + * Returns 0 on success, or -EINVAL if @keysize or @authsize contain values
> + * that are not permitted by the GCM specification.
> + */
> +int gcmaes_expandkey(struct gcmaes_ctx *ctx, const u8 *key,
> +		     unsigned int keysize, unsigned int authsize);

These comments are duplicated in the .c file too.  They should be in just one
place, probably the .c file since that approach is more common in the kernel.

Also, this seems to be intended to be a kerneldoc comment, but the return value
isn't documented in the correct format.  It needs to be "Return:".  Try this:

$ ./scripts/kernel-doc -v -none lib/crypto/gcmaes.c
lib/crypto/gcmaes.c:35: info: Scanning doc for function gcmaes_expandkey
lib/crypto/gcmaes.c:48: warning: No description found for return value of 'gcmaes_expandkey'
lib/crypto/gcmaes.c:114: info: Scanning doc for function gcmaes_encrypt
lib/crypto/gcmaes.c:142: info: Scanning doc for function gcmaes_decrypt
lib/crypto/gcmaes.c:162: warning: No description found for return value of 'gcmaes_decrypt'

> +config CRYPTO_LIB_GCMAES
> +	tristate
> +	select CRYPTO_GF128MUL
> +	select CRYPTO_LIB_AES
> +	select CRYPTO_LIB_UTILS

Doesn't this mean that crypto/gf128mul.c needs to be moved into lib/crypto/?

- Eric