crc-t10dif: Fix potential crypto notify dead-lock

Commit Message

Herbert Xu June 4, 2020, 6:33 a.m. UTC

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 <[e-mail]>

Comments

Eric Biggers June 5, 2020, 5:40 a.m. UTC | #1

On Thu, Jun 04, 2020 at 04:33:24PM +1000, Herbert Xu wrote:
> +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;
> +	return;
>  }

The last return statement is unnecessary.

>  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;
>  }

Wouldn't it make more sense to initialize crct10dif_tfm before registering the
notifier?  Then the mutex wouldn't be needed.

- Eric

Herbert Xu June 5, 2020, 6:49 a.m. UTC | #2

On Thu, Jun 04, 2020 at 10:40:49PM -0700, Eric Biggers wrote:
> On Thu, Jun 04, 2020 at 04:33:24PM +1000, Herbert Xu wrote:
> > +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;
> > +	return;
> >  }
> 
> The last return statement is unnecessary.

Thanks I'll change this.

> >  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;
> >  }
> 
> Wouldn't it make more sense to initialize crct10dif_tfm before registering the
> notifier?  Then the mutex wouldn't be needed.

Right the mutex wouldn't be needed, but you open up a race window
where a better algorithm could be registered in between the first
crypto_alloc call and the notifier registration.

Cheers,

Patch

diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c
index 8cc01a603416..7063442377b1 100644
--- a/lib/crc-t10dif.c
+++ b/lib/crc-t10dif.c
@@ -19,39 +19,47 @@ 
 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;
+	return;
 }
 
 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 +94,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 +121,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;
+	}
+
+	name = crypto_tfm_alg_driver_name(crypto_shash_tfm(tfm));
+	len = sprintf(buffer, "%s\n", name);
+
+unlock:
+	rcu_read_unlock();
+
+	return len;
 }
 
 module_param_call(transform, NULL, crc_t10dif_transform_show, NULL, 0644);