Re: crypto: api - Fix use-after-free and race in crypto_spawn_alg

Re: crypto: api - Fix use-after-free and race in crypto_spawn_alg

[Markus Elfring]

> Secondly the setting of the DYING flag is racy because we hold
> the read-lock instead of the write-lock.  We should instead call

Would an imperative wording be preferred for this change description?


> This patch fixes both problems.

Will another wording alternative become relevant?

Are the provided tags sufficient for such information?

Regards,
Markus

Re: crypto: api - Fix use-after-free and race in crypto_spawn_alg

[Herbert Xu]

On Wed, Apr 15, 2020 at 07:30:01PM -0700, Eric Biggers wrote:
> 
> I'm not sure what you mean here.  crypto_alg_get() is:
> 
> static inline struct crypto_alg *crypto_alg_get(struct crypto_alg *alg)
> {
>         refcount_inc(&alg->cra_refcnt);
>         return alg;
> }
> 
> So given:
> 
> 	target = crypto_alg_get(alg);
> 
> Both alg and target have to be non-NULL.

Yes I know that we know that it can't be NULL, but gcc 8.3 doesn't.

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

Re: crypto: api - Fix use-after-free and race in crypto_spawn_alg

[Eric Biggers]

On Thu, Apr 16, 2020 at 12:25:02PM +1000, Herbert Xu wrote:
> On Wed, Apr 15, 2020 at 07:17:03PM -0700, Eric Biggers wrote:
> > 
> > Wouldn't it be a bit simpler to set 'target = NULL', remove 'shoot',
> > and use 'if (target)' instead of 'if (shoot)'?
> 
> Yes it is simpler but it's actually semantically different because
> the compiler doesn't know that spawn->alg cannot be NULL in this
> case.
> 

I'm not sure what you mean here.  crypto_alg_get() is:

static inline struct crypto_alg *crypto_alg_get(struct crypto_alg *alg)
{
        refcount_inc(&alg->cra_refcnt);
        return alg;
}

So given:

	target = crypto_alg_get(alg);

Both alg and target have to be non-NULL.

- Eric

Re: crypto: api - Fix use-after-free and race in crypto_spawn_alg

[Herbert Xu]

On Wed, Apr 15, 2020 at 07:17:03PM -0700, Eric Biggers wrote:
> 
> Wouldn't it be a bit simpler to set 'target = NULL', remove 'shoot',
> and use 'if (target)' instead of 'if (shoot)'?

Yes it is simpler but it's actually semantically different because
the compiler doesn't know that spawn->alg cannot be NULL in this
case.

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

Re: crypto: api - Fix use-after-free and race in crypto_spawn_alg

[Eric Biggers]

On Fri, Apr 10, 2020 at 04:09:42PM +1000, Herbert Xu wrote:
> There are two problems in crypto_spawn_alg.  First of all it may
> return spawn->alg even if spawn->dead is set.  This results in a
> double-free as detected by syzbot.
> 
> Secondly the setting of the DYING flag is racy because we hold
> the read-lock instead of the write-lock.  We should instead call
> crypto_shoot_alg in a safe manner by gaining a refcount, dropping
> the lock, and then releasing the refcount.
> 
> This patch fixes both problems.
> 
> Reported-by: syzbot+fc0674cde00b66844470@syzkaller.appspotmail.com
> Fixes: 4f87ee118d16 ("crypto: api - Do not zap spawn->alg")
> Fixes: 73669cc55646 ("crypto: api - Fix race condition in...")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index 69605e21af92..f8b4dc161c02 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -716,17 +716,27 @@ EXPORT_SYMBOL_GPL(crypto_drop_spawn);
>  
>  static struct crypto_alg *crypto_spawn_alg(struct crypto_spawn *spawn)
>  {
> -	struct crypto_alg *alg;
> +	struct crypto_alg *alg = ERR_PTR(-EAGAIN);
> +	struct crypto_alg *target;
> +	bool shoot = false;
>  
>  	down_read(&crypto_alg_sem);
> -	alg = spawn->alg;
> -	if (!spawn->dead && !crypto_mod_get(alg)) {
> -		alg->cra_flags |= CRYPTO_ALG_DYING;
> -		alg = NULL;
> +	if (!spawn->dead) {
> +		alg = spawn->alg;
> +		if (!crypto_mod_get(alg)) {
> +			target = crypto_alg_get(alg);
> +			shoot = true;
> +			alg = ERR_PTR(-EAGAIN);
> +		}
>  	}
>  	up_read(&crypto_alg_sem);
>  
> -	return alg ?: ERR_PTR(-EAGAIN);
> +	if (shoot) {
> +		crypto_shoot_alg(target);
> +		crypto_alg_put(target);
> +	}
> +
> +	return alg;
>  }

Wouldn't it be a bit simpler to set 'target = NULL', remove 'shoot',
and use 'if (target)' instead of 'if (shoot)'?

- Eric

crypto: api - Fix use-after-free and race in crypto_spawn_alg

[Herbert Xu]

There are two problems in crypto_spawn_alg.  First of all it may
return spawn->alg even if spawn->dead is set.  This results in a
double-free as detected by syzbot.

Secondly the setting of the DYING flag is racy because we hold
the read-lock instead of the write-lock.  We should instead call
crypto_shoot_alg in a safe manner by gaining a refcount, dropping
the lock, and then releasing the refcount.

This patch fixes both problems.

Reported-by: syzbot+fc0674cde00b66844470@syzkaller.appspotmail.com
Fixes: 4f87ee118d16 ("crypto: api - Do not zap spawn->alg")
Fixes: 73669cc55646 ("crypto: api - Fix race condition in...")
Cc: <stable@vger.kernel.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/algapi.c b/crypto/algapi.c
index 69605e21af92..f8b4dc161c02 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -716,17 +716,27 @@ EXPORT_SYMBOL_GPL(crypto_drop_spawn);
 
 static struct crypto_alg *crypto_spawn_alg(struct crypto_spawn *spawn)
 {
-	struct crypto_alg *alg;
+	struct crypto_alg *alg = ERR_PTR(-EAGAIN);
+	struct crypto_alg *target;
+	bool shoot = false;
 
 	down_read(&crypto_alg_sem);
-	alg = spawn->alg;
-	if (!spawn->dead && !crypto_mod_get(alg)) {
-		alg->cra_flags |= CRYPTO_ALG_DYING;
-		alg = NULL;
+	if (!spawn->dead) {
+		alg = spawn->alg;
+		if (!crypto_mod_get(alg)) {
+			target = crypto_alg_get(alg);
+			shoot = true;
+			alg = ERR_PTR(-EAGAIN);
+		}
 	}
 	up_read(&crypto_alg_sem);
 
-	return alg ?: ERR_PTR(-EAGAIN);
+	if (shoot) {
+		crypto_shoot_alg(target);
+		crypto_alg_put(target);
+	}
+
+	return alg;
 }
 
 struct crypto_tfm *crypto_spawn_tfm(struct crypto_spawn *spawn, u32 type,
diff --git a/crypto/api.c b/crypto/api.c
index 7d71a9b10e5f..edcf690800d4 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -333,12 +333,13 @@ static unsigned int crypto_ctxsize(struct crypto_alg *alg, u32 type, u32 mask)
 	return len;
 }
 
-static void crypto_shoot_alg(struct crypto_alg *alg)
+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 d5ebc60c5143..ff06a3bd1ca1 100644
--- a/crypto/internal.h
+++ b/crypto/internal.h
@@ -65,6 +65,7 @@ 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,
-- 
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