Re: [v2 PATCH 1/3] crypto: shash - Add init_tfm/exit_tfm and verify descsize

[Eric Biggers <ebiggers@kernel.org>]

On Sun, Dec 08, 2019 at 01:42:51PM +0800, Herbert Xu wrote:
> The shash interface supports a dynamic descsize field because of
> the presence of fallbacks (it's just padlock-sha actually, perhaps
> we can remove it one day).  As it is the API does not verify the
> setting of descsize at all.  It is up to the individual algorithms
> to ensure that descsize does not exceed the specified maximum value
> of HASH_MAX_DESCSIZE (going above would cause stack corruption).
> 
> In order to allow the API to impose this limit directly, this patch
> adds init_tfm/exit_tfm hooks to the shash_alg structure.  We can
> then verify the descsize setting in the API directly.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> ---
> 
>  crypto/shash.c        |   25 +++++++++++++++++++++++++
>  include/crypto/hash.h |   13 +++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/crypto/shash.c b/crypto/shash.c
> index e83c5124f6eb..63a7ea368eb1 100644
> --- a/crypto/shash.c
> +++ b/crypto/shash.c
> @@ -386,15 +386,40 @@ int crypto_init_shash_ops_async(struct crypto_tfm *tfm)
>  	return 0;
>  }
>  
> +static void crypto_shash_exit_tfm(struct crypto_tfm *tfm)
> +{
> +	struct crypto_shash *hash = __crypto_shash_cast(tfm);
> +	struct shash_alg *alg = crypto_shash_alg(hash);
> +
> +	alg->exit_tfm(hash);
> +}
> +
>  static int crypto_shash_init_tfm(struct crypto_tfm *tfm)
>  {
>  	struct crypto_shash *hash = __crypto_shash_cast(tfm);
>  	struct shash_alg *alg = crypto_shash_alg(hash);
> +	int err;
>  
>  	hash->descsize = alg->descsize;
>  
>  	shash_set_needkey(hash, alg);
>  
> +	if (alg->exit_tfm)
> +		tfm->exit = crypto_shash_exit_tfm;
> +
> +	if (!alg->init_tfm)
> +		return 0;
> +
> +	err = alg->init_tfm(hash);
> +	if (err)
> +		return err;
> +
> +	if (WARN_ON_ONCE(hash->descsize > HASH_MAX_DESCSIZE)) {
> +		if (alg->exit_tfm)
> +			alg->exit_tfm(hash);
> +		return -EINVAL;
> +	}

Nit: it would be helpful to have a comment just above the WARN_ON_ONCE() like:

	/* ->init_tfm() may have increased the descsize. */

- Eric