[PATCH] crypto: do not free algorithm before using

[PATCH] crypto: do not free algorithm before using

[Pan Bian]

In multiple functions, the algorithm fields are read after its reference
is dropped through crypto_mod_put. In this case, the algorithm memory
may be freed, resulting in use-after-free bugs. This patch delays the
put operation until the algorithm is never used.

Signed-off-by: Pan Bian <bianpan2016@163.com>
---
 crypto/cbc.c  | 6 ++++--
 crypto/cfb.c  | 6 ++++--
 crypto/pcbc.c | 6 ++++--
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/crypto/cbc.c b/crypto/cbc.c
index b761b1f..dd5f332 100644
--- a/crypto/cbc.c
+++ b/crypto/cbc.c
@@ -140,9 +140,8 @@ static int crypto_cbc_create(struct crypto_template *tmpl, struct rtattr **tb)
 	spawn = skcipher_instance_ctx(inst);
 	err = crypto_init_spawn(spawn, alg, skcipher_crypto_instance(inst),
 				CRYPTO_ALG_TYPE_MASK);
-	crypto_mod_put(alg);
 	if (err)
-		goto err_free_inst;
+		goto err_put_alg;
 
 	err = crypto_inst_setname(skcipher_crypto_instance(inst), "cbc", alg);
 	if (err)
@@ -174,12 +173,15 @@ static int crypto_cbc_create(struct crypto_template *tmpl, struct rtattr **tb)
 	err = skcipher_register_instance(tmpl, inst);
 	if (err)
 		goto err_drop_spawn;
+	crypto_mod_put(alg);
 
 out:
 	return err;
 
 err_drop_spawn:
 	crypto_drop_spawn(spawn);
+err_put_alg:
+	crypto_mod_put(alg);
 err_free_inst:
 	kfree(inst);
 	goto out;
diff --git a/crypto/cfb.c b/crypto/cfb.c
index a0d68c0..20987d0 100644
--- a/crypto/cfb.c
+++ b/crypto/cfb.c
@@ -286,9 +286,8 @@ static int crypto_cfb_create(struct crypto_template *tmpl, struct rtattr **tb)
 	spawn = skcipher_instance_ctx(inst);
 	err = crypto_init_spawn(spawn, alg, skcipher_crypto_instance(inst),
 				CRYPTO_ALG_TYPE_MASK);
-	crypto_mod_put(alg);
 	if (err)
-		goto err_free_inst;
+		goto err_put_alg;
 
 	err = crypto_inst_setname(skcipher_crypto_instance(inst), "cfb", alg);
 	if (err)
@@ -317,12 +316,15 @@ static int crypto_cfb_create(struct crypto_template *tmpl, struct rtattr **tb)
 	err = skcipher_register_instance(tmpl, inst);
 	if (err)
 		goto err_drop_spawn;
+	crypto_mod_put(alg);
 
 out:
 	return err;
 
 err_drop_spawn:
 	crypto_drop_spawn(spawn);
+err_put_alg:
+	crypto_mod_put(alg);
 err_free_inst:
 	kfree(inst);
 	goto out;
diff --git a/crypto/pcbc.c b/crypto/pcbc.c
index ef802f6..8aa1014 100644
--- a/crypto/pcbc.c
+++ b/crypto/pcbc.c
@@ -244,9 +244,8 @@ static int crypto_pcbc_create(struct crypto_template *tmpl, struct rtattr **tb)
 	spawn = skcipher_instance_ctx(inst);
 	err = crypto_init_spawn(spawn, alg, skcipher_crypto_instance(inst),
 				CRYPTO_ALG_TYPE_MASK);
-	crypto_mod_put(alg);
 	if (err)
-		goto err_free_inst;
+		goto err_put_alg;
 
 	err = crypto_inst_setname(skcipher_crypto_instance(inst), "pcbc", alg);
 	if (err)
@@ -275,12 +274,15 @@ static int crypto_pcbc_create(struct crypto_template *tmpl, struct rtattr **tb)
 	err = skcipher_register_instance(tmpl, inst);
 	if (err)
 		goto err_drop_spawn;
+	crypto_mod_put(alg);
 
 out:
 	return err;
 
 err_drop_spawn:
 	crypto_drop_spawn(spawn);
+err_put_alg:
+	crypto_mod_put(alg);
 err_free_inst:
 	kfree(inst);
 	goto out;
-- 
2.7.4

Re: [PATCH] crypto: do not free algorithm before using

[Herbert Xu]

On Thu, Nov 22, 2018 at 06:00:16PM +0800, Pan Bian wrote:
> In multiple functions, the algorithm fields are read after its reference
> is dropped through crypto_mod_put. In this case, the algorithm memory
> may be freed, resulting in use-after-free bugs. This patch delays the
> put operation until the algorithm is never used.
> 
> Signed-off-by: Pan Bian <bianpan2016@163.com>

I don't think this patch is needed.

> ---
>  crypto/cbc.c  | 6 ++++--
>  crypto/cfb.c  | 6 ++++--
>  crypto/pcbc.c | 6 ++++--
>  3 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/crypto/cbc.c b/crypto/cbc.c
> index b761b1f..dd5f332 100644
> --- a/crypto/cbc.c
> +++ b/crypto/cbc.c
> @@ -140,9 +140,8 @@ static int crypto_cbc_create(struct crypto_template *tmpl, struct rtattr **tb)
>  	spawn = skcipher_instance_ctx(inst);
>  	err = crypto_init_spawn(spawn, alg, skcipher_crypto_instance(inst),
>  				CRYPTO_ALG_TYPE_MASK);
> -	crypto_mod_put(alg);
>  	if (err)
> -		goto err_free_inst;
> +		goto err_put_alg;

We can safely drop the reference to the algorithm because the spawn
is now meant to hold a reference to it.  As long as the spawn is
alive so will the algorithm.

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: [PATCH] crypto: do not free algorithm before using

[PanBian]

On Thu, Nov 22, 2018 at 10:44:41PM +0800, Herbert Xu wrote:
> On Thu, Nov 22, 2018 at 06:00:16PM +0800, Pan Bian wrote:
> > In multiple functions, the algorithm fields are read after its reference
> > is dropped through crypto_mod_put. In this case, the algorithm memory
> > may be freed, resulting in use-after-free bugs. This patch delays the
> > put operation until the algorithm is never used.
> > 
> > Signed-off-by: Pan Bian <bianpan2016@163.com>
> 
> I don't think this patch is needed.
> 
> > ---
> >  crypto/cbc.c  | 6 ++++--
> >  crypto/cfb.c  | 6 ++++--
> >  crypto/pcbc.c | 6 ++++--
> >  3 files changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/crypto/cbc.c b/crypto/cbc.c
> > index b761b1f..dd5f332 100644
> > --- a/crypto/cbc.c
> > +++ b/crypto/cbc.c
> > @@ -140,9 +140,8 @@ static int crypto_cbc_create(struct crypto_template *tmpl, struct rtattr **tb)
> >  	spawn = skcipher_instance_ctx(inst);
> >  	err = crypto_init_spawn(spawn, alg, skcipher_crypto_instance(inst),
> >  				CRYPTO_ALG_TYPE_MASK);
> > -	crypto_mod_put(alg);
> >  	if (err)
> > -		goto err_free_inst;
> > +		goto err_put_alg;
> 
> We can safely drop the reference to the algorithm because the spawn
> is now meant to hold a reference to it.  As long as the spawn is
> alive so will the algorithm.

Thanks for your explanation! But I find that the function
crypto_init_spawn just lets spawn->alg point to the algorithm without 
increasing the reference count, i.e., alg->cra_refcnt. So I am confused
about how this can protect the algorithm from being freed. Maybe I
missed some key points. Could you please explain it in more details?

Thank you!

Best regards,
Pan Bian

> 
> 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: [PATCH] crypto: do not free algorithm before using

[Herbert Xu]

On Fri, Nov 23, 2018 at 09:05:55AM +0800, PanBian wrote:
>
> Thanks for your explanation! But I find that the function
> crypto_init_spawn just lets spawn->alg point to the algorithm without 
> increasing the reference count, i.e., alg->cra_refcnt. So I am confused
> about how this can protect the algorithm from being freed. Maybe I
> missed some key points. Could you please explain it in more details?

Oh you're right! This bug was introduced during the skcipher
conversion.  The spawns are not meant to carry a reference count
with them because they are not supposed to stop the underlying
algorithm from being removed.  So yes we do need to hold a reference
count on alg until the instance is registered.

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: [PATCH] crypto: do not free algorithm before using

[Herbert Xu]

On Thu, Nov 22, 2018 at 06:00:16PM +0800, Pan Bian wrote:
> In multiple functions, the algorithm fields are read after its reference
> is dropped through crypto_mod_put. In this case, the algorithm memory
> may be freed, resulting in use-after-free bugs. This patch delays the
> put operation until the algorithm is never used.
> 
> Signed-off-by: Pan Bian <bianpan2016@163.com>

Patch applied.  Thanks!

BTW, this problem exists in almost every other skcipher template,
e.g., in ctr.  Are you working on a fix for them too?

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: [PATCH] crypto: do not free algorithm before using

[PanBian]

On Thu, Nov 29, 2018 at 02:57:11PM +0800, Herbert Xu wrote:
> On Thu, Nov 22, 2018 at 06:00:16PM +0800, Pan Bian wrote:
> > In multiple functions, the algorithm fields are read after its reference
> > is dropped through crypto_mod_put. In this case, the algorithm memory
> > may be freed, resulting in use-after-free bugs. This patch delays the
> > put operation until the algorithm is never used.
> > 
> > Signed-off-by: Pan Bian <bianpan2016@163.com>
> 
> Patch applied.  Thanks!
> 
> BTW, this problem exists in almost every other skcipher template,
> e.g., in ctr.  Are you working on a fix for them too?

Probably not. Could you please provide patches to fix them?

Thanks,
Pan

> 
> 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: [PATCH] crypto: do not free algorithm before using

[Herbert Xu]

On Thu, Nov 29, 2018 at 03:47:56PM +0800, PanBian wrote:
> On Thu, Nov 29, 2018 at 02:57:11PM +0800, Herbert Xu wrote:
> > On Thu, Nov 22, 2018 at 06:00:16PM +0800, Pan Bian wrote:
> > > In multiple functions, the algorithm fields are read after its reference
> > > is dropped through crypto_mod_put. In this case, the algorithm memory
> > > may be freed, resulting in use-after-free bugs. This patch delays the
> > > put operation until the algorithm is never used.
> > > 
> > > Signed-off-by: Pan Bian <bianpan2016@163.com>
> > 
> > Patch applied.  Thanks!
> > 
> > BTW, this problem exists in almost every other skcipher template,
> > e.g., in ctr.  Are you working on a fix for them too?
> 
> Probably not. Could you please provide patches to fix them?

OK, I'll work on it then.

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