Re: [v2 PATCH] crc-t10dif: Fix potential crypto notify dead-lock

[Eric Biggers <ebiggers@kernel.org>]

On Fri, Jun 05, 2020 at 04:59:18PM +1000, Herbert Xu wrote:
> The crypto notify call occurs with a read mutex held so you must
> not do any substantial work directly.  In particular, you cannot
> call crypto_alloc_* as they may trigger further notifications
> which may dead-lock in the presence of another writer.
> 
> This patch fixes this by postponing the work into a work queue and
> taking the same lock in the module init function.
> 
> While we're at it this patch also ensures that all RCU accesses are
> marked appropriately (tested with sparse).
> 
> Finally this also reveals a race condition in module param show
> function as it may be called prior to the module init function.
> It's fixed by testing whether crct10dif_tfm is NULL (this is true
> iff the init function has not completed assuming fallback is false).
> 
> Fixes: 11dcb1037f40 ("crc-t10dif: Allow current transform to be...")
> Fixes: b76377543b73 ("crc-t10dif: Pick better transform if one...")
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c
> index 8cc01a603416..c9acf1c12cfc 100644
> --- a/lib/crc-t10dif.c
> +++ b/lib/crc-t10dif.c
> @@ -19,39 +19,46 @@
>  static struct crypto_shash __rcu *crct10dif_tfm;
>  static struct static_key crct10dif_fallback __read_mostly;
>  static DEFINE_MUTEX(crc_t10dif_mutex);
> +static struct work_struct crct10dif_rehash_work;
>  
> -static int crc_t10dif_rehash(struct notifier_block *self, unsigned long val, void *data)
> +static int crc_t10dif_notify(struct notifier_block *self, unsigned long val, void *data)
>  {
>  	struct crypto_alg *alg = data;
> -	struct crypto_shash *new, *old;
>  
>  	if (val != CRYPTO_MSG_ALG_LOADED ||
>  	    static_key_false(&crct10dif_fallback) ||
>  	    strncmp(alg->cra_name, CRC_T10DIF_STRING, strlen(CRC_T10DIF_STRING)))
>  		return 0;
>  
> +	schedule_work(&crct10dif_rehash_work);
> +	return 0;
> +}
> +
> +static void crc_t10dif_rehash(struct work_struct *work)
> +{
> +	struct crypto_shash *new, *old;
> +
>  	mutex_lock(&crc_t10dif_mutex);
>  	old = rcu_dereference_protected(crct10dif_tfm,
>  					lockdep_is_held(&crc_t10dif_mutex));
>  	if (!old) {
>  		mutex_unlock(&crc_t10dif_mutex);
> -		return 0;
> +		return;
>  	}
>  	new = crypto_alloc_shash("crct10dif", 0, 0);
>  	if (IS_ERR(new)) {
>  		mutex_unlock(&crc_t10dif_mutex);
> -		return 0;
> +		return;
>  	}
>  	rcu_assign_pointer(crct10dif_tfm, new);
>  	mutex_unlock(&crc_t10dif_mutex);
>  
>  	synchronize_rcu();
>  	crypto_free_shash(old);
> -	return 0;
>  }
>  
>  static struct notifier_block crc_t10dif_nb = {
> -	.notifier_call = crc_t10dif_rehash,
> +	.notifier_call = crc_t10dif_notify,
>  };
>  
>  __u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len)
> @@ -86,19 +93,26 @@ EXPORT_SYMBOL(crc_t10dif);
>  
>  static int __init crc_t10dif_mod_init(void)
>  {
> +	struct crypto_shash *tfm;
> +
> +	INIT_WORK(&crct10dif_rehash_work, crc_t10dif_rehash);
>  	crypto_register_notifier(&crc_t10dif_nb);
> -	crct10dif_tfm = crypto_alloc_shash("crct10dif", 0, 0);
> -	if (IS_ERR(crct10dif_tfm)) {
> +	mutex_lock(&crc_t10dif_mutex);
> +	tfm = crypto_alloc_shash("crct10dif", 0, 0);
> +	if (IS_ERR(tfm)) {
>  		static_key_slow_inc(&crct10dif_fallback);
> -		crct10dif_tfm = NULL;
> +		tfm = NULL;
>  	}
> +	RCU_INIT_POINTER(crct10dif_tfm, tfm);
> +	mutex_unlock(&crc_t10dif_mutex);
>  	return 0;
>  }
>  
>  static void __exit crc_t10dif_mod_fini(void)
>  {
>  	crypto_unregister_notifier(&crc_t10dif_nb);
> -	crypto_free_shash(crct10dif_tfm);
> +	cancel_work_sync(&crct10dif_rehash_work);
> +	crypto_free_shash(rcu_dereference_protected(crct10dif_tfm, 1));
>  }
>  
>  module_init(crc_t10dif_mod_init);
> @@ -106,11 +120,27 @@ module_exit(crc_t10dif_mod_fini);
>  
>  static int crc_t10dif_transform_show(char *buffer, const struct kernel_param *kp)
>  {
> +	struct crypto_shash *tfm;
> +	const char *name;
> +	int len;
> +
>  	if (static_key_false(&crct10dif_fallback))
>  		return sprintf(buffer, "fallback\n");
>  
> -	return sprintf(buffer, "%s\n",
> -		crypto_tfm_alg_driver_name(crypto_shash_tfm(crct10dif_tfm)));
> +	rcu_read_lock();
> +	tfm = rcu_dereference(crct10dif_tfm);
> +	if (!tfm) {
> +		len = sprintf(buffer, "init\n");
> +		goto unlock;
> +	}

Wouldn't it be better to have crct10dif_fallback enabled by default, and then
disable it once the tfm is allocated?

That would make the checks for a NULL tfm in crc_t10dif_transform_show() and
crc_t10dif_notify() unnecessary.  Also, it would make it so that
crc_t10dif_update() no longer crashes if called before module_init().

- Eric