On Mon, Nov 05, 2018 at 05:49:06PM -0800, Eric Biggers wrote:
> Hi Corentin,
>
> On Mon, Nov 05, 2018 at 12:51:14PM +0000, Corentin Labbe wrote:
> > All crypto_stats functions use the struct xxx_request for feeding stats,
> > but in some case this structure could already be freed.
> >
> > For fixing this, the needed parameters (len and alg) will be stored
> > before the request being executed.
> > Fixes: cac5818c25d0 ("crypto: user - Implement a generic crypto statistics")
> > Reported-by: syzbot <syzbot+6939a606a5305e9e9799@xxxxxxxxxxxxxxxxxxxxxxxxx>
> >
> > Signed-off-by: Corentin Labbe <clabbe@xxxxxxxxxxxx>
> > ---
> > crypto/ahash.c | 14 ++++++++--
> > include/crypto/acompress.h | 30 ++++++++++----------
> > include/crypto/aead.h | 30 ++++++++++----------
> > include/crypto/akcipher.h | 56 ++++++++++++++++++--------------------
> > include/crypto/hash.h | 25 +++++++++--------
> > include/crypto/kpp.h | 22 +++++++--------
> > include/crypto/skcipher.h | 16 +++++++----
> > include/linux/crypto.h | 34 +++++++++++------------
> > 8 files changed, 118 insertions(+), 109 deletions(-)
> >
> > diff --git a/crypto/ahash.c b/crypto/ahash.c
> > index 3a348fbcf8f9..3f4c44c1e80f 100644
> > --- a/crypto/ahash.c
> > +++ b/crypto/ahash.c
> > @@ -364,20 +364,26 @@ static int crypto_ahash_op(struct ahash_request *req,
> >
> > int crypto_ahash_final(struct ahash_request *req)
> > {
> > + struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> > + struct crypto_alg *alg = tfm->base.__crt_alg;
> > + unsigned int nbytes = req->nbytes;
> > int ret;
> >
> > ret = crypto_ahash_op(req, crypto_ahash_reqtfm(req)->final);
> > - crypto_stat_ahash_final(req, ret);
> > + crypto_stat_ahash_final(nbytes, ret, alg);
> > return ret;
> > }
> > EXPORT_SYMBOL_GPL(crypto_ahash_final);
>
> I'm not confident this is enough. If the request is being processed
> asynchronously, the completion callback could be used to signal another thread
> to go ahead and free the resources, including not only the request but also the
> 'tfm', which could even result in the last reference to the 'alg' being dropped
> and therefore the 'alg' being freed. If I'm wrong, then please explain why :-)
>
> Note: I'd also prefer a solution that is more obviously zero-overhead in the
> !CONFIG_CRYPTO_STATS case.
>
> - Eric
>
I think the best solution is to grab a crypto_alg refcnt before the operation and release it after.
So for example this will lead to:
--- a/include/crypto/skcipher.h
+++ b/include/crypto/skcipher.h
@@ -486,7 +486,7 @@ static inline struct crypto_sync_skcipher *crypto_sync_skcipher_reqtfm(
return container_of(tfm, struct crypto_sync_skcipher, base);
}
-static inline void crypto_stat_skcipher_encrypt(struct skcipher_request *req,
+static inline void crypto_stat_skcipher_encrypt(unsigned int cryptlen,
int ret, struct crypto_alg *alg)
{
#ifdef CONFIG_CRYPTO_STATS
@@ -494,12 +494,13 @@ static inline void crypto_stat_skcipher_encrypt(struct skcipher_request *req,
atomic64_inc(&alg->cipher_err_cnt);
} else {
atomic64_inc(&alg->encrypt_cnt);
- atomic64_add(req->cryptlen, &alg->encrypt_tlen);
+ atomic64_add(cryptlen, &alg->encrypt_tlen);
}
+ crypto_alg_put(alg);
#endif
}
-static inline void crypto_stat_skcipher_decrypt(struct skcipher_request *req,
+static inline void crypto_stat_skcipher_decrypt(unsigned int cryptlen,
int ret, struct crypto_alg *alg)
{
#ifdef CONFIG_CRYPTO_STATS
@@ -507,8 +508,9 @@ static inline void crypto_stat_skcipher_decrypt(struct skcipher_request *req,
atomic64_inc(&alg->cipher_err_cnt);
} else {
atomic64_inc(&alg->decrypt_cnt);
- atomic64_add(req->cryptlen, &alg->decrypt_tlen);
+ atomic64_add(cryptlen, &alg->decrypt_tlen);
}
+ crypto_alg_put(alg);
#endif
}
@@ -526,13 +528,18 @@ static inline void crypto_stat_skcipher_decrypt(struct skcipher_request *req,
static inline int crypto_skcipher_encrypt(struct skcipher_request *req)
{
struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
+ struct crypto_alg *alg = tfm->base.__crt_alg;
+ unsigned int cryptlen = req->cryptlen;
int ret;
+#ifdef CONFIG_CRYPTO_STATS
+ crypto_alg_get(alg);
+#endif
if (crypto_skcipher_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
ret = -ENOKEY;
else
ret = tfm->encrypt(req);
- crypto_stat_skcipher_encrypt(req, ret, tfm->base.__crt_alg);
+ crypto_stat_skcipher_encrypt(cryptlen, ret, alg);
return ret;
}
@@ -550,13 +557,18 @@ static inline int crypto_skcipher_encrypt(struct skcipher_request *req)
static inline int crypto_skcipher_decrypt(struct skcipher_request *req)
{
struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
+ struct crypto_alg *alg = tfm->base.__crt_alg;
+ unsigned int cryptlen = req->cryptlen;
int ret;
+#ifdef CONFIG_CRYPTO_STATS
+ crypto_alg_get(alg);
+#endif
if (crypto_skcipher_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
ret = -ENOKEY;
else
ret = tfm->decrypt(req);
- crypto_stat_skcipher_decrypt(req, ret, tfm->base.__crt_alg);
+ crypto_stat_skcipher_decrypt(cryptlen, ret, alg);
return ret;
}
This should not have any overhead with !CONFIG_CRYPTO_STATS
The only drawback is that crypto_alg_get/crypto_alg_put need to be moved out of crypto/internal.h to include/linux/crypto.h
Herbert what do you think about that ?
Regards
