[PATCH 0/2] crypto: api - Fix spawn races

[PATCH 0/2] crypto: api - Fix spawn races

[Herbert Xu]

This patch series fixes a couple of race conditions in the spawn
code.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH 1/2] crypto: api - Fix race condition in crypto_spawn_alg

[Herbert Xu]

The function crypto_spawn_alg is racy because it drops the lock
before shooting the dying algorithm.  The algorithm could disappear
altogether before we shoot it.

This patch fixes it by moving the shooting into the locked section.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 crypto/algapi.c   |   16 +++++-----------
 crypto/api.c      |    3 +--
 crypto/internal.h |    1 -
 3 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/crypto/algapi.c b/crypto/algapi.c
index 6869feb31c99..cc55301beef4 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -678,22 +678,16 @@ EXPORT_SYMBOL_GPL(crypto_drop_spawn);
 static struct crypto_alg *crypto_spawn_alg(struct crypto_spawn *spawn)
 {
 	struct crypto_alg *alg;
-	struct crypto_alg *alg2;
 
 	down_read(&crypto_alg_sem);
 	alg = spawn->alg;
-	alg2 = alg;
-	if (alg2)
-		alg2 = crypto_mod_get(alg2);
-	up_read(&crypto_alg_sem);
-
-	if (!alg2) {
-		if (alg)
-			crypto_shoot_alg(alg);
-		return ERR_PTR(-EAGAIN);
+	if (alg && !crypto_mod_get(alg)) {
+		alg->cra_flags |= CRYPTO_ALG_DYING;
+		alg = NULL;
 	}
+	up_read(&crypto_alg_sem);
 
-	return alg;
+	return alg ?: ERR_PTR(-EAGAIN);
 }
 
 struct crypto_tfm *crypto_spawn_tfm(struct crypto_spawn *spawn, u32 type,
diff --git a/crypto/api.c b/crypto/api.c
index 55bca28df92d..0ef9f2a37d3d 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -344,13 +344,12 @@ static unsigned int crypto_ctxsize(struct crypto_alg *alg, u32 type, u32 mask)
 	return len;
 }
 
-void crypto_shoot_alg(struct crypto_alg *alg)
+static void crypto_shoot_alg(struct crypto_alg *alg)
 {
 	down_write(&crypto_alg_sem);
 	alg->cra_flags |= CRYPTO_ALG_DYING;
 	up_write(&crypto_alg_sem);
 }
-EXPORT_SYMBOL_GPL(crypto_shoot_alg);
 
 struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type,
 				      u32 mask)
diff --git a/crypto/internal.h b/crypto/internal.h
index 93df7bec844a..e506a57e2243 100644
--- a/crypto/internal.h
+++ b/crypto/internal.h
@@ -68,7 +68,6 @@ void crypto_alg_tested(const char *name, int err);
 void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
 			  struct crypto_alg *nalg);
 void crypto_remove_final(struct list_head *list);
-void crypto_shoot_alg(struct crypto_alg *alg);
 struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type,
 				      u32 mask);
 void *crypto_create_tfm(struct crypto_alg *alg,

[PATCH 2/2] crypto: api - Do not zap spawn->alg

[Herbert Xu]

Currently when a spawn is removed we will zap its alg field.
This is racy because the spawn could belong to an unregistered
instance which may dereference the spawn->alg field.

This patch fixes this by keeping spawn->alg constant and instead
adding a new spawn->dead field to indicate that a spawn is going
away.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 crypto/algapi.c         |   21 +++++++++++----------
 include/crypto/algapi.h |    1 +
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/crypto/algapi.c b/crypto/algapi.c
index cc55301beef4..2d0697c4f69c 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -93,15 +93,16 @@ static struct list_head *crypto_more_spawns(struct crypto_alg *alg,
 	if (!spawn)
 		return NULL;
 
-	n = list_next_entry(spawn, list);
+	list_move(&spawn->list, secondary_spawns);
 
-	if (spawn->alg && &n->list != stack && !n->alg)
-		n->alg = (n->list.next == stack) ? alg :
-			 &list_next_entry(n, list)->inst->alg;
+	if (list_is_last(&spawn->list, stack))
+		return top;
 
-	list_move(&spawn->list, secondary_spawns);
+	n = list_next_entry(spawn, list);
+	if (!spawn->dead)
+		n->dead = false;
 
-	return &n->list == stack ? top : &n->inst->alg.cra_users;
+	return &n->inst->alg.cra_users;
 }
 
 static void crypto_remove_instance(struct crypto_instance *inst,
@@ -160,7 +161,7 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
 			if (&inst->alg == nalg)
 				break;
 
-			spawn->alg = NULL;
+			spawn->dead = true;
 			spawns = &inst->alg.cra_users;
 
 			/*
@@ -179,7 +180,7 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
 					      &secondary_spawns)));
 
 	list_for_each_entry_safe(spawn, n, &secondary_spawns, list) {
-		if (spawn->alg)
+		if (!spawn->dead)
 			list_move(&spawn->list, &spawn->alg->cra_users);
 		else
 			crypto_remove_instance(spawn->inst, list);
@@ -669,7 +670,7 @@ EXPORT_SYMBOL_GPL(crypto_grab_spawn);
 void crypto_drop_spawn(struct crypto_spawn *spawn)
 {
 	down_write(&crypto_alg_sem);
-	if (spawn->alg)
+	if (!spawn->dead)
 		list_del(&spawn->list);
 	up_write(&crypto_alg_sem);
 }
@@ -681,7 +682,7 @@ static struct crypto_alg *crypto_spawn_alg(struct crypto_spawn *spawn)
 
 	down_read(&crypto_alg_sem);
 	alg = spawn->alg;
-	if (alg && !crypto_mod_get(alg)) {
+	if (!spawn->dead && !crypto_mod_get(alg)) {
 		alg->cra_flags |= CRYPTO_ALG_DYING;
 		alg = NULL;
 	}
diff --git a/include/crypto/algapi.h b/include/crypto/algapi.h
index 5cd846defdd6..771a295ac755 100644
--- a/include/crypto/algapi.h
+++ b/include/crypto/algapi.h
@@ -70,6 +70,7 @@ struct crypto_spawn {
 	struct crypto_instance *inst;
 	const struct crypto_type *frontend;
 	u32 mask;
+	bool dead;
 };
 
 struct crypto_queue {

Re: [PATCH 2/2] crypto: api - Do not zap spawn->alg

[Eric Biggers]

On Fri, Dec 06, 2019 at 10:39:53PM +0800, Herbert Xu wrote:
> Currently when a spawn is removed we will zap its alg field.
> This is racy because the spawn could belong to an unregistered
> instance which may dereference the spawn->alg field.
> 
> This patch fixes this by keeping spawn->alg constant and instead
> adding a new spawn->dead field to indicate that a spawn is going
> away.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> ---
> 
>  crypto/algapi.c         |   21 +++++++++++----------
>  include/crypto/algapi.h |    1 +
>  2 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index cc55301beef4..2d0697c4f69c 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -93,15 +93,16 @@ static struct list_head *crypto_more_spawns(struct crypto_alg *alg,
>  	if (!spawn)
>  		return NULL;
>  
> -	n = list_next_entry(spawn, list);
> +	list_move(&spawn->list, secondary_spawns);
>  
> -	if (spawn->alg && &n->list != stack && !n->alg)
> -		n->alg = (n->list.next == stack) ? alg :
> -			 &list_next_entry(n, list)->inst->alg;
> +	if (list_is_last(&spawn->list, stack))
> +		return top;
>  
> -	list_move(&spawn->list, secondary_spawns);
> +	n = list_next_entry(spawn, list);
> +	if (!spawn->dead)
> +		n->dead = false;
>  
> -	return &n->list == stack ? top : &n->inst->alg.cra_users;
> +	return &n->inst->alg.cra_users;
>  }
>  
>  static void crypto_remove_instance(struct crypto_instance *inst,
> @@ -160,7 +161,7 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
>  			if (&inst->alg == nalg)
>  				break;
>  
> -			spawn->alg = NULL;
> +			spawn->dead = true;
>  			spawns = &inst->alg.cra_users;
>  
>  			/*
> @@ -179,7 +180,7 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
>  					      &secondary_spawns)));
>  
>  	list_for_each_entry_safe(spawn, n, &secondary_spawns, list) {
> -		if (spawn->alg)
> +		if (!spawn->dead)
>  			list_move(&spawn->list, &spawn->alg->cra_users);
>  		else
>  			crypto_remove_instance(spawn->inst, list);
> @@ -669,7 +670,7 @@ EXPORT_SYMBOL_GPL(crypto_grab_spawn);
>  void crypto_drop_spawn(struct crypto_spawn *spawn)
>  {
>  	down_write(&crypto_alg_sem);
> -	if (spawn->alg)
> +	if (!spawn->dead)
>  		list_del(&spawn->list);
>  	up_write(&crypto_alg_sem);
>  }
> @@ -681,7 +682,7 @@ static struct crypto_alg *crypto_spawn_alg(struct crypto_spawn *spawn)
>  
>  	down_read(&crypto_alg_sem);
>  	alg = spawn->alg;
> -	if (alg && !crypto_mod_get(alg)) {
> +	if (!spawn->dead && !crypto_mod_get(alg)) {
>  		alg->cra_flags |= CRYPTO_ALG_DYING;
>  		alg = NULL;
>  	}
> diff --git a/include/crypto/algapi.h b/include/crypto/algapi.h
> index 5cd846defdd6..771a295ac755 100644
> --- a/include/crypto/algapi.h
> +++ b/include/crypto/algapi.h
> @@ -70,6 +70,7 @@ struct crypto_spawn {
>  	struct crypto_instance *inst;
>  	const struct crypto_type *frontend;
>  	u32 mask;
> +	bool dead;
>  };
>  

This patch causes the below crash.

Also, some comments (e.g. for struct crypto_spawn and crypto_remove_spawns())
would be really helpful to understand what's going on here.

==================================================================
BUG: KASAN: stack-out-of-bounds in crypto_more_spawns crypto/algapi.c:105 [inline]
BUG: KASAN: stack-out-of-bounds in crypto_remove_spawns+0x9cd/0xc30 crypto/algapi.c:179
Read of size 8 at addr ffff88806a1f7d58 by task cryptomgr_test/430

CPU: 2 PID: 430 Comm: cryptomgr_test Not tainted 5.4.0-07583-g2c69d0491b7e2 #5
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0xa0/0xea lib/dump_stack.c:118
 print_address_description.constprop.4+0x21/0x2c0 mm/kasan/report.c:374
 __kasan_report.cold.8+0x7a/0xb5 mm/kasan/report.c:506
 kasan_report+0x12/0x20 mm/kasan/common.c:638
 __asan_report_load8_noabort+0x14/0x20 mm/kasan/generic_report.c:135
 crypto_more_spawns crypto/algapi.c:105 [inline]
 crypto_remove_spawns+0x9cd/0xc30 crypto/algapi.c:179
 crypto_alg_tested+0x493/0x5b0 crypto/algapi.c:357
 cryptomgr_test+0x60/0x80 crypto/algboss.c:225
 kthread+0x331/0x3f0 kernel/kthread.c:255
 ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352

The buggy address belongs to the page:
page:ffffea0001736e08 refcount:0 mapcount:0 mapping:0000000000000000 index:0x0
raw: 0100000000000000 ffffea0001736e10 ffffea0001736e10 0000000000000000
raw: 0000000000000000 0000000000000000 00000000ffffffff
page dumped because: kasan: bad access detected

addr ffff88806a1f7d58 is located in stack of task cryptomgr_test/430 at offset 56 in frame:
 crypto_remove_spawns+0x0/0xc30 crypto/algapi.c:956

this frame has 3 objects:
 [32, 48) 'secondary_spawns'
 [96, 112) 'stack'
 [160, 176) 'top'

Memory state around the buggy address:
 ffff88806a1f7c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 ffff88806a1f7c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffff88806a1f7d00: 00 00 00 00 f1 f1 f1 f1 00 00 f2 f2 f2 f2 f2 f2
                                                    ^
 ffff88806a1f7d80: 00 00 f2 f2 f2 f2 f2 f2 00 00 f2 f2 00 00 00 00
 ffff88806a1f7e00: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
==================================================================
Disabling lock debugging due to kernel taint
kasan: CONFIG_KASAN_INLINE enabled
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] SMP KASAN
CPU: 2 PID: 430 Comm: cryptomgr_test Tainted: G    B             5.4.0-07583-g2c69d0491b7e2 #5
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
RIP: 0010:crypto_remove_spawns+0x359/0xc30 crypto/algapi.c:155
Code: 39 c4 0f 84 71 01 00 00 4c 89 e0 48 c1 e8 03 42 80 3c 38 00 0f 85 a7 05 00 00 4d 8b 24 24 49 8d 7c 24 18 48 89 f8 48 c1 e8 03 <42> 80 3c 38 00 0f 85 6b 05 00 0f
RSP: 0000:ffff88806a1f7cc8 EFLAGS: 00010206
RAX: 0000000000000003 RBX: ffff88806a1f7e00 RCX: ffffffff81339704
RDX: 0000000000000000 RSI: 0000000000000004 RDI: 0000000000000018
RBP: ffff88806a1f7e28 R08: fffffbfff06973a5 R09: fffffbfff06973a5
R10: fffffbfff06973a4 R11: ffffffff834b9d23 R12: 0000000000000000
R13: ffff88806a1f7d80 R14: ffff88806a1f7d40 R15: dffffc0000000000
FS:  0000000000000000(0000) GS:ffff88806d900000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000ffffffff CR3: 0000000003213001 CR4: 0000000000760ee0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
PKRU: 55555554
Call Trace:
 crypto_alg_tested+0x493/0x5b0 crypto/algapi.c:357
 cryptomgr_test+0x60/0x80 crypto/algboss.c:225
 kthread+0x331/0x3f0 kernel/kthread.c:255
 ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
Modules linked in:
---[ end trace e42d20303e3e7399 ]---
RIP: 0010:crypto_remove_spawns+0x359/0xc30 crypto/algapi.c:155
Code: 39 c4 0f 84 71 01 00 00 4c 89 e0 48 c1 e8 03 42 80 3c 38 00 0f 85 a7 05 00 00 4d 8b 24 24 49 8d 7c 24 18 48 89 f8 48 c1 e8 03 <42> 80 3c 38 00 0f 85 6b 05 00 0f
RSP: 0000:ffff88806a1f7cc8 EFLAGS: 00010206
RAX: 0000000000000003 RBX: ffff88806a1f7e00 RCX: ffffffff81339704
RDX: 0000000000000000 RSI: 0000000000000004 RDI: 0000000000000018
RBP: ffff88806a1f7e28 R08: fffffbfff06973a5 R09: fffffbfff06973a5
R10: fffffbfff06973a4 R11: ffffffff834b9d23 R12: 0000000000000000
R13: ffff88806a1f7d80 R14: ffff88806a1f7d40 R15: dffffc0000000000
FS:  0000000000000000(0000) GS:ffff88806d900000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000ffffffff CR3: 0000000003213001 CR4: 0000000000760ee0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
PKRU: 55555554

Re: [PATCH 2/2] crypto: api - Do not zap spawn->alg

[Herbert Xu]

On Fri, Dec 06, 2019 at 02:50:21PM -0800, Eric Biggers wrote:
>
> This patch causes the below crash.

Yes I got carried away with rearranging the code in the function
crypto_more_spawns.  I shouldn't be using spawn as a list node
after doing the list_move call.  The code now looks like:

	n = list_prev_entry(spawn, list);
	list_move(&spawn->list, secondary_spawns);

	if (list_is_last(&n->list, stack))
		return top;

	n = list_next_entry(n, list);
	if (!spawn->dead)
		n->dead = false;

	return &n->inst->alg.cra_users;

> Also, some comments (e.g. for struct crypto_spawn and crypto_remove_spawns())
> would be really helpful to understand what's going on here.

crypto_remove_spawns is performing a depth-first walk on cra_users
without recursion.  In the specific case of a spawn removal triggered
by a new registration, we will halt the walk when we hit the
newly registered algorithm, and undo any actions that we did
on the path leading to that object.  The function crypto_more_spawns
performs the undo action.

I'll add this to crypto_remove_spawns.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[v2 PATCH 0/2] crypto: api - Fix spawn races

[Herbert Xu]

This patch series fixes a couple of race conditions in the spawn
code.

v2 fixes a crash in crypto_more_spawns.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH 1/2] crypto: api - Fix race condition in crypto_spawn_alg

[Herbert Xu]

The function crypto_spawn_alg is racy because it drops the lock
before shooting the dying algorithm.  The algorithm could disappear
altogether before we shoot it.

This patch fixes it by moving the shooting into the locked section.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 crypto/algapi.c   |   16 +++++-----------
 crypto/api.c      |    3 +--
 crypto/internal.h |    1 -
 3 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/crypto/algapi.c b/crypto/algapi.c
index 6869feb31c99..cc55301beef4 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -678,22 +678,16 @@ EXPORT_SYMBOL_GPL(crypto_drop_spawn);
 static struct crypto_alg *crypto_spawn_alg(struct crypto_spawn *spawn)
 {
 	struct crypto_alg *alg;
-	struct crypto_alg *alg2;
 
 	down_read(&crypto_alg_sem);
 	alg = spawn->alg;
-	alg2 = alg;
-	if (alg2)
-		alg2 = crypto_mod_get(alg2);
-	up_read(&crypto_alg_sem);
-
-	if (!alg2) {
-		if (alg)
-			crypto_shoot_alg(alg);
-		return ERR_PTR(-EAGAIN);
+	if (alg && !crypto_mod_get(alg)) {
+		alg->cra_flags |= CRYPTO_ALG_DYING;
+		alg = NULL;
 	}
+	up_read(&crypto_alg_sem);
 
-	return alg;
+	return alg ?: ERR_PTR(-EAGAIN);
 }
 
 struct crypto_tfm *crypto_spawn_tfm(struct crypto_spawn *spawn, u32 type,
diff --git a/crypto/api.c b/crypto/api.c
index 55bca28df92d..0ef9f2a37d3d 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -344,13 +344,12 @@ static unsigned int crypto_ctxsize(struct crypto_alg *alg, u32 type, u32 mask)
 	return len;
 }
 
-void crypto_shoot_alg(struct crypto_alg *alg)
+static void crypto_shoot_alg(struct crypto_alg *alg)
 {
 	down_write(&crypto_alg_sem);
 	alg->cra_flags |= CRYPTO_ALG_DYING;
 	up_write(&crypto_alg_sem);
 }
-EXPORT_SYMBOL_GPL(crypto_shoot_alg);
 
 struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type,
 				      u32 mask)
diff --git a/crypto/internal.h b/crypto/internal.h
index 93df7bec844a..e506a57e2243 100644
--- a/crypto/internal.h
+++ b/crypto/internal.h
@@ -68,7 +68,6 @@ void crypto_alg_tested(const char *name, int err);
 void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
 			  struct crypto_alg *nalg);
 void crypto_remove_final(struct list_head *list);
-void crypto_shoot_alg(struct crypto_alg *alg);
 struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type,
 				      u32 mask);
 void *crypto_create_tfm(struct crypto_alg *alg,

[PATCH 2/2] crypto: api - Do not zap spawn->alg

[Herbert Xu]

Currently when a spawn is removed we will zap its alg field.
This is racy because the spawn could belong to an unregistered
instance which may dereference the spawn->alg field.

This patch fixes this by keeping spawn->alg constant and instead
adding a new spawn->dead field to indicate that a spawn is going
away.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 crypto/algapi.c         |   22 ++++++++++++----------
 include/crypto/algapi.h |    1 +
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/crypto/algapi.c b/crypto/algapi.c
index cc55301beef4..adb516380be9 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -93,15 +93,17 @@ static struct list_head *crypto_more_spawns(struct crypto_alg *alg,
 	if (!spawn)
 		return NULL;
 
-	n = list_next_entry(spawn, list);
+	n = list_prev_entry(spawn, list);
+	list_move(&spawn->list, secondary_spawns);
 
-	if (spawn->alg && &n->list != stack && !n->alg)
-		n->alg = (n->list.next == stack) ? alg :
-			 &list_next_entry(n, list)->inst->alg;
+	if (list_is_last(&n->list, stack))
+		return top;
 
-	list_move(&spawn->list, secondary_spawns);
+	n = list_next_entry(n, list);
+	if (!spawn->dead)
+		n->dead = false;
 
-	return &n->list == stack ? top : &n->inst->alg.cra_users;
+	return &n->inst->alg.cra_users;
 }
 
 static void crypto_remove_instance(struct crypto_instance *inst,
@@ -160,7 +162,7 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
 			if (&inst->alg == nalg)
 				break;
 
-			spawn->alg = NULL;
+			spawn->dead = true;
 			spawns = &inst->alg.cra_users;
 
 			/*
@@ -179,7 +181,7 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
 					      &secondary_spawns)));
 
 	list_for_each_entry_safe(spawn, n, &secondary_spawns, list) {
-		if (spawn->alg)
+		if (!spawn->dead)
 			list_move(&spawn->list, &spawn->alg->cra_users);
 		else
 			crypto_remove_instance(spawn->inst, list);
@@ -669,7 +671,7 @@ EXPORT_SYMBOL_GPL(crypto_grab_spawn);
 void crypto_drop_spawn(struct crypto_spawn *spawn)
 {
 	down_write(&crypto_alg_sem);
-	if (spawn->alg)
+	if (!spawn->dead)
 		list_del(&spawn->list);
 	up_write(&crypto_alg_sem);
 }
@@ -681,7 +683,7 @@ static struct crypto_alg *crypto_spawn_alg(struct crypto_spawn *spawn)
 
 	down_read(&crypto_alg_sem);
 	alg = spawn->alg;
-	if (alg && !crypto_mod_get(alg)) {
+	if (!spawn->dead && !crypto_mod_get(alg)) {
 		alg->cra_flags |= CRYPTO_ALG_DYING;
 		alg = NULL;
 	}
diff --git a/include/crypto/algapi.h b/include/crypto/algapi.h
index 5cd846defdd6..771a295ac755 100644
--- a/include/crypto/algapi.h
+++ b/include/crypto/algapi.h
@@ -70,6 +70,7 @@ struct crypto_spawn {
 	struct crypto_instance *inst;
 	const struct crypto_type *frontend;
 	u32 mask;
+	bool dead;
 };
 
 struct crypto_queue {

[PATCH] crypto: api - Add more comments to crypto_remove_spawns

[Herbert Xu]

On Sat, Dec 07, 2019 at 11:40:17AM +0800, Herbert Xu wrote:
>
> > Also, some comments (e.g. for struct crypto_spawn and crypto_remove_spawns())
> > would be really helpful to understand what's going on here.
> 
> crypto_remove_spawns is performing a depth-first walk on cra_users
> without recursion.  In the specific case of a spawn removal triggered
> by a new registration, we will halt the walk when we hit the
> newly registered algorithm, and undo any actions that we did
> on the path leading to that object.  The function crypto_more_spawns
> performs the undo action.
> 
> I'll add this to crypto_remove_spawns.

Here is the patch:

---8<---
This patch explains the logic behind crypto_remove_spawns and its
underling crypto_more_spawns.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/algapi.c b/crypto/algapi.c
index adb516380be9..8ef771ab0ee3 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -82,6 +82,15 @@ static void crypto_destroy_instance(struct crypto_alg *alg)
 	crypto_tmpl_put(tmpl);
 }
 
+/*
+ * This function adds a spawn to the list secondary_spawns which
+ * will be used at the end of crypto_remove_spawns to unregister
+ * instances, unless the spawn happens to be one that is depended
+ * on by the new algorithm (nalg in crypto_remove_spawns).
+ *
+ * This function is also responsible for resurrecting any algorithms
+ * in the dependency chain of nalg by unsetting n->dead.
+ */
 static struct list_head *crypto_more_spawns(struct crypto_alg *alg,
 					    struct list_head *stack,
 					    struct list_head *top,
@@ -128,6 +137,12 @@ static void crypto_remove_instance(struct crypto_instance *inst,
 	BUG_ON(!list_empty(&inst->alg.cra_users));
 }
 
+/*
+ * Given an algorithm alg, remove all algorithms that depend on it
+ * through spawns.  If nalg is not null, then exempt any algorithms
+ * that is depended on by nalg.  This is useful when nalg itself
+ * depends on alg.
+ */
 void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
 			  struct crypto_alg *nalg)
 {
@@ -146,6 +161,11 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
 		list_move(&spawn->list, &top);
 	}
 
+	/*
+	 * Perform a depth-first walk starting from alg through
+	 * the cra_users tree.  The list stack records the path
+	 * from alg to the current spawn.
+	 */
 	spawns = &top;
 	do {
 		while (!list_empty(spawns)) {
@@ -180,6 +200,11 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
 	} while ((spawns = crypto_more_spawns(alg, &stack, &top,
 					      &secondary_spawns)));
 
+	/*
+	 * Remove all instances that are marked as dead.  Also
+	 * complete the resurrection of the others by moving them
+	 * back to the cra_users list.
+	 */
 	list_for_each_entry_safe(spawn, n, &secondary_spawns, list) {
 		if (!spawn->dead)
 			list_move(&spawn->list, &spawn->alg->cra_users);
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] crypto: api - Fix race condition in crypto_spawn_alg

[Eric Biggers]

On Sat, Dec 07, 2019 at 10:15:15PM +0800, Herbert Xu wrote:
> The function crypto_spawn_alg is racy because it drops the lock
> before shooting the dying algorithm.  The algorithm could disappear
> altogether before we shoot it.
> 
> This patch fixes it by moving the shooting into the locked section.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Does this need Fixes and Cc stable tags?

- Eric

Re: [PATCH 1/2] crypto: api - Fix race condition in crypto_spawn_alg

[Herbert Xu]

On Tue, Dec 10, 2019 at 07:38:28PM -0800, Eric Biggers wrote:
> On Sat, Dec 07, 2019 at 10:15:15PM +0800, Herbert Xu wrote:
> > The function crypto_spawn_alg is racy because it drops the lock
> > before shooting the dying algorithm.  The algorithm could disappear
> > altogether before we shoot it.
> > 
> > This patch fixes it by moving the shooting into the locked section.
> > 
> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> Does this need Fixes and Cc stable tags?

Fixes: 6bfd48096ff8 ("[CRYPTO] api: Added spawns")

I don't think we want this to go through stable right away.  Perhaps
a few cycles down the track it could be pushed to stable.  It's
certainly not an urgent problem.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt