* [PATCH] crypto: do not free algorithm before using
@ 2018-11-22 10:00 Pan Bian
2018-11-22 14:44 ` Herbert Xu
2018-11-29 6:57 ` Herbert Xu
0 siblings, 2 replies; 7+ messages in thread
From: Pan Bian @ 2018-11-22 10:00 UTC (permalink / raw)
To: Herbert Xu, David S. Miller; +Cc: linux-crypto, linux-kernel, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] crypto: do not free algorithm before using
2018-11-22 10:00 [PATCH] crypto: do not free algorithm before using Pan Bian
@ 2018-11-22 14:44 ` Herbert Xu
2018-11-23 1:05 ` PanBian
2018-11-29 6:57 ` Herbert Xu
1 sibling, 1 reply; 7+ messages in thread
From: Herbert Xu @ 2018-11-22 14:44 UTC (permalink / raw)
To: Pan Bian; +Cc: David S. Miller, linux-crypto, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] crypto: do not free algorithm before using
2018-11-22 14:44 ` Herbert Xu
@ 2018-11-23 1:05 ` PanBian
2018-11-23 2:36 ` Herbert Xu
0 siblings, 1 reply; 7+ messages in thread
From: PanBian @ 2018-11-23 1:05 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, linux-crypto, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] crypto: do not free algorithm before using
2018-11-23 1:05 ` PanBian
@ 2018-11-23 2:36 ` Herbert Xu
0 siblings, 0 replies; 7+ messages in thread
From: Herbert Xu @ 2018-11-23 2:36 UTC (permalink / raw)
To: PanBian; +Cc: David S. Miller, linux-crypto, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] crypto: do not free algorithm before using
2018-11-22 10:00 [PATCH] crypto: do not free algorithm before using Pan Bian
2018-11-22 14:44 ` Herbert Xu
@ 2018-11-29 6:57 ` Herbert Xu
2018-11-29 7:47 ` PanBian
1 sibling, 1 reply; 7+ messages in thread
From: Herbert Xu @ 2018-11-29 6:57 UTC (permalink / raw)
To: Pan Bian; +Cc: David S. Miller, linux-crypto, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] crypto: do not free algorithm before using
2018-11-29 6:57 ` Herbert Xu
@ 2018-11-29 7:47 ` PanBian
2018-11-29 8:29 ` Herbert Xu
0 siblings, 1 reply; 7+ messages in thread
From: PanBian @ 2018-11-29 7:47 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, linux-crypto, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] crypto: do not free algorithm before using
2018-11-29 7:47 ` PanBian
@ 2018-11-29 8:29 ` Herbert Xu
0 siblings, 0 replies; 7+ messages in thread
From: Herbert Xu @ 2018-11-29 8:29 UTC (permalink / raw)
To: PanBian; +Cc: David S. Miller, linux-crypto, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-11-29 19:33 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-22 10:00 [PATCH] crypto: do not free algorithm before using Pan Bian
2018-11-22 14:44 ` Herbert Xu
2018-11-23 1:05 ` PanBian
2018-11-23 2:36 ` Herbert Xu
2018-11-29 6:57 ` Herbert Xu
2018-11-29 7:47 ` PanBian
2018-11-29 8:29 ` Herbert Xu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.