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 <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 <[email protected]>
>
> 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 <[email protected]>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 <[email protected]>
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 <[email protected]>
>
> 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 <[email protected]>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 <[email protected]>
> >
> > 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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt