lkml.org 
[lkml]   [2018]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v4 06/11] crypto: crypto_user_stat: fix use_after_free of struct xxx_request
    Hi Corentin,

    On Fri, Nov 23, 2018 at 12:02:16PM +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@syzkaller.appspotmail.com>
    >
    > Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
    > ---
    > crypto/ahash.c | 17 ++-
    > crypto/algapi.c | 285 +++++++++++++++++++++++++++++++++++++
    > crypto/rng.c | 4 +-
    > include/crypto/acompress.h | 38 ++---
    > include/crypto/aead.h | 38 ++---
    > include/crypto/akcipher.h | 74 ++--------
    > include/crypto/hash.h | 32 +----
    > include/crypto/kpp.h | 48 ++-----
    > include/crypto/rng.h | 27 +---
    > include/crypto/skcipher.h | 36 ++---
    > include/linux/crypto.h | 63 ++++----
    > 11 files changed, 386 insertions(+), 276 deletions(-)
    >
    > diff --git a/crypto/ahash.c b/crypto/ahash.c
    > index 3a348fbcf8f9..5d320a811f75 100644
    > --- a/crypto/ahash.c
    > +++ b/crypto/ahash.c
    > @@ -364,20 +364,28 @@ 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;
    >
    > + crypto_stats_get(alg);
    > ret = crypto_ahash_op(req, crypto_ahash_reqtfm(req)->final);
    > - crypto_stat_ahash_final(req, ret);
    > + crypto_stats_ahash_final(nbytes, ret, alg);
    > return ret;
    > }
    > EXPORT_SYMBOL_GPL(crypto_ahash_final);
    >
    > int crypto_ahash_finup(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;
    >
    > + crypto_stats_get(alg);
    > ret = crypto_ahash_op(req, crypto_ahash_reqtfm(req)->finup);
    > - crypto_stat_ahash_final(req, ret);
    > + crypto_stats_ahash_final(nbytes, ret, alg);
    > return ret;
    > }
    > EXPORT_SYMBOL_GPL(crypto_ahash_finup);
    > @@ -385,13 +393,16 @@ EXPORT_SYMBOL_GPL(crypto_ahash_finup);
    > int crypto_ahash_digest(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;
    >
    > + crypto_stats_get(alg);
    > if (crypto_ahash_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
    > ret = -ENOKEY;
    > else
    > ret = crypto_ahash_op(req, tfm->digest);
    > - crypto_stat_ahash_final(req, ret);
    > + crypto_stats_ahash_final(nbytes, ret, alg);
    > return ret;
    > }
    > EXPORT_SYMBOL_GPL(crypto_ahash_digest);
    > diff --git a/crypto/algapi.c b/crypto/algapi.c
    > index 42fe316f80ee..aae302d92c2a 100644
    > --- a/crypto/algapi.c
    > +++ b/crypto/algapi.c
    > @@ -1078,6 +1078,291 @@ int crypto_type_has_alg(const char *name, const struct crypto_type *frontend,
    > }
    > EXPORT_SYMBOL_GPL(crypto_type_has_alg);
    >
    > +#ifdef CONFIG_CRYPTO_STATS
    > +void crypto_stats_get(struct crypto_alg *alg)
    > +{
    > + crypto_alg_get(alg);
    > +}
    > +
    > +void crypto_stats_ablkcipher_encrypt(unsigned int nbytes, int ret,
    > + struct crypto_alg *alg)
    > +{
    > + if (ret && ret != -EINPROGRESS && ret != -EBUSY) {
    > + atomic64_inc(&alg->cipher_err_cnt);
    > + } else {
    > + atomic64_inc(&alg->encrypt_cnt);
    > + atomic64_add(nbytes, &alg->encrypt_tlen);
    > + }
    > + crypto_alg_put(alg);
    > +}
    > +
    > +void crypto_stats_ablkcipher_decrypt(unsigned int nbytes, int ret,
    > + struct crypto_alg *alg)
    > +{
    > + if (ret && ret != -EINPROGRESS && ret != -EBUSY) {
    > + atomic64_inc(&alg->cipher_err_cnt);
    > + } else {
    > + atomic64_inc(&alg->decrypt_cnt);
    > + atomic64_add(nbytes, &alg->decrypt_tlen);
    > + }
    > + crypto_alg_put(alg);
    > +}
    > +
    > +void crypto_stats_aead_encrypt(unsigned int cryptlen, struct crypto_alg *alg,
    > + int ret)
    > +{
    > + if (ret && ret != -EINPROGRESS && ret != -EBUSY) {
    > + atomic64_inc(&alg->aead_err_cnt);
    > + } else {
    > + atomic64_inc(&alg->encrypt_cnt);
    > + atomic64_add(cryptlen, &alg->encrypt_tlen);
    > + }
    > + crypto_alg_put(alg);
    > +}
    > +
    > +void crypto_stats_aead_decrypt(unsigned int cryptlen, struct crypto_alg *alg,
    > + int ret)
    > +{
    > + if (ret && ret != -EINPROGRESS && ret != -EBUSY) {
    > + atomic64_inc(&alg->aead_err_cnt);
    > + } else {
    > + atomic64_inc(&alg->decrypt_cnt);
    > + atomic64_add(cryptlen, &alg->decrypt_tlen);
    > + }
    > + crypto_alg_put(alg);
    > +}
    > +
    > +void crypto_stats_akcipher_encrypt(unsigned int src_len, int ret,
    > + struct crypto_alg *alg)
    > +{
    > + if (ret && ret != -EINPROGRESS && ret != -EBUSY) {
    > + atomic64_inc(&alg->akcipher_err_cnt);
    > + } else {
    > + atomic64_inc(&alg->encrypt_cnt);
    > + atomic64_add(src_len, &alg->encrypt_tlen);
    > + }
    > + crypto_alg_put(alg);
    > +}
    > +
    > +void crypto_stats_akcipher_decrypt(unsigned int src_len, int ret,
    > + struct crypto_alg *alg)
    > +{
    > + if (ret && ret != -EINPROGRESS && ret != -EBUSY) {
    > + atomic64_inc(&alg->akcipher_err_cnt);
    > + } else {
    > + atomic64_inc(&alg->decrypt_cnt);
    > + atomic64_add(src_len, &alg->decrypt_tlen);
    > + }
    > + crypto_alg_put(alg);
    > +}
    > +
    > +void crypto_stats_akcipher_sign(int ret, struct crypto_alg *alg)
    > +{
    > + if (ret && ret != -EINPROGRESS && ret != -EBUSY)
    > + atomic64_inc(&alg->akcipher_err_cnt);
    > + else
    > + atomic64_inc(&alg->sign_cnt);
    > + crypto_alg_put(alg);
    > +}
    > +
    > +void crypto_stats_akcipher_verify(int ret, struct crypto_alg *alg)
    > +{
    > + if (ret && ret != -EINPROGRESS && ret != -EBUSY)
    > + atomic64_inc(&alg->akcipher_err_cnt);
    > + else
    > + atomic64_inc(&alg->verify_cnt);
    > + crypto_alg_put(alg);
    > +}
    > +
    > +void crypto_stats_compress(unsigned int slen, int ret, struct crypto_alg *alg)
    > +{
    > + if (ret && ret != -EINPROGRESS && ret != -EBUSY) {
    > + atomic64_inc(&alg->compress_err_cnt);
    > + } else {
    > + atomic64_inc(&alg->compress_cnt);
    > + atomic64_add(slen, &alg->compress_tlen);
    > + }
    > + crypto_alg_put(alg);
    > +}
    > +
    > +void crypto_stats_decompress(unsigned int slen, int ret, struct crypto_alg *alg)
    > +{
    > + if (ret && ret != -EINPROGRESS && ret != -EBUSY) {
    > + atomic64_inc(&alg->compress_err_cnt);
    > + } else {
    > + atomic64_inc(&alg->decompress_cnt);
    > + atomic64_add(slen, &alg->decompress_tlen);
    > + }
    > + crypto_alg_put(alg);
    > +}
    > +
    > +void crypto_stats_ahash_update(unsigned int nbytes, int ret,
    > + struct crypto_alg *alg)
    > +{
    > + if (ret && ret != -EINPROGRESS && ret != -EBUSY)
    > + atomic64_inc(&alg->hash_err_cnt);
    > + else
    > + atomic64_add(nbytes, &alg->hash_tlen);
    > + crypto_alg_put(alg);
    > +}
    > +
    > +void crypto_stats_ahash_final(unsigned int nbytes, int ret,
    > + struct crypto_alg *alg)
    > +{
    > + if (ret && ret != -EINPROGRESS && ret != -EBUSY) {
    > + atomic64_inc(&alg->hash_err_cnt);
    > + } else {
    > + atomic64_inc(&alg->hash_cnt);
    > + atomic64_add(nbytes, &alg->hash_tlen);
    > + }
    > + crypto_alg_put(alg);
    > +}
    > +
    > +void crypto_stats_kpp_set_secret(struct crypto_alg *alg, int ret)
    > +{
    > + if (ret)
    > + atomic64_inc(&alg->kpp_err_cnt);
    > + else
    > + atomic64_inc(&alg->setsecret_cnt);
    > + crypto_alg_put(alg);
    > +}
    > +
    > +void crypto_stats_kpp_generate_public_key(struct crypto_alg *alg, int ret)
    > +{
    > + if (ret)
    > + atomic64_inc(&alg->kpp_err_cnt);
    > + else
    > + atomic64_inc(&alg->generate_public_key_cnt);
    > + crypto_alg_put(alg);
    > +}
    > +
    > +void crypto_stats_kpp_compute_shared_secret(struct crypto_alg *alg, int ret)
    > +{
    > + if (ret)
    > + atomic64_inc(&alg->kpp_err_cnt);
    > + else
    > + atomic64_inc(&alg->compute_shared_secret_cnt);
    > + crypto_alg_put(alg);
    > +}
    > +
    > +void crypto_stats_rng_seed(struct crypto_alg *alg, int ret)
    > +{
    > + if (ret && ret != -EINPROGRESS && ret != -EBUSY)
    > + atomic64_inc(&alg->rng_err_cnt);
    > + else
    > + atomic64_inc(&alg->seed_cnt);
    > + crypto_alg_put(alg);
    > +}
    > +
    > +void crypto_stats_rng_generate(struct crypto_alg *alg, unsigned int dlen,
    > + int ret)
    > +{
    > + if (ret && ret != -EINPROGRESS && ret != -EBUSY) {
    > + atomic64_inc(&alg->rng_err_cnt);
    > + } else {
    > + atomic64_inc(&alg->generate_cnt);
    > + atomic64_add(dlen, &alg->generate_tlen);
    > + }
    > + crypto_alg_put(alg);
    > +}
    > +
    > +void crypto_stats_skcipher_encrypt(unsigned int cryptlen, int ret,
    > + struct crypto_alg *alg)
    > +{
    > + if (ret && ret != -EINPROGRESS && ret != -EBUSY) {
    > + atomic64_inc(&alg->cipher_err_cnt);
    > + } else {
    > + atomic64_inc(&alg->encrypt_cnt);
    > + atomic64_add(cryptlen, &alg->encrypt_tlen);
    > + }
    > + crypto_alg_put(alg);
    > +}
    > +
    > +void crypto_stats_skcipher_decrypt(unsigned int cryptlen, int ret,
    > + struct crypto_alg *alg)
    > +{
    > + if (ret && ret != -EINPROGRESS && ret != -EBUSY) {
    > + atomic64_inc(&alg->cipher_err_cnt);
    > + } else {
    > + atomic64_inc(&alg->decrypt_cnt);
    > + atomic64_add(cryptlen, &alg->decrypt_tlen);
    > + }
    > + crypto_alg_put(alg);
    > +}
    > +#else
    > +void crypto_stats_get(struct crypto_alg *alg)
    > +{}
    > +void crypto_stats_ablkcipher_encrypt(unsigned int nbytes, int ret,
    > + struct crypto_alg *alg)
    > +{}
    > +void crypto_stats_ablkcipher_decrypt(unsigned int nbytes, int ret,
    > + struct crypto_alg *alg)
    > +{}
    > +void crypto_stats_aead_encrypt(unsigned int cryptlen, struct crypto_alg *alg,
    > + int ret)
    > +{}
    > +void crypto_stats_aead_decrypt(unsigned int cryptlen, struct crypto_alg *alg,
    > + int ret)
    > +{}
    > +void crypto_stats_ahash_update(unsigned int nbytes, int ret,
    > + struct crypto_alg *alg)
    > +{}
    > +void crypto_stats_ahash_final(unsigned int nbytes, int ret,
    > + struct crypto_alg *alg)
    > +{}
    > +void crypto_stats_akcipher_encrypt(unsigned int src_len, int ret,
    > + struct crypto_alg *alg)
    > +{}
    > +void crypto_stats_akcipher_decrypt(unsigned int src_len, int ret,
    > + struct crypto_alg *alg)
    > +{}
    > +void crypto_stats_akcipher_sign(int ret, struct crypto_alg *alg)
    > +{}
    > +void crypto_stats_akcipher_verify(int ret, struct crypto_alg *alg)
    > +{}
    > +void crypto_stats_compress(unsigned int slen, int ret, struct crypto_alg *alg)
    > +{}
    > +void crypto_stats_decompress(unsigned int slen, int ret, struct crypto_alg *alg)
    > +{}
    > +void crypto_stats_kpp_set_secret(struct crypto_alg *alg, int ret)
    > +{}
    > +void crypto_stats_kpp_generate_public_key(struct crypto_alg *alg, int ret)
    > +{}
    > +void crypto_stats_kpp_compute_shared_secret(struct crypto_alg *alg, int ret)
    > +{}
    > +void crypto_stats_rng_seed(struct crypto_alg *alg, int ret)
    > +{}
    > +void crypto_stats_rng_generate(struct crypto_alg *alg, unsigned int dlen,
    > + int ret)
    > +{}
    > +void crypto_stats_skcipher_encrypt(unsigned int cryptlen, int ret,
    > + struct crypto_alg *alg)
    > +{}
    > +void crypto_stats_skcipher_decrypt(unsigned int cryptlen, int ret,
    > + struct crypto_alg *alg)
    > +{}
    > +#endif

    The stubs need to be static inline in the .h file so that they are optimized out
    when !CONFIG_CRYPTO_STATS. Otherwise there is a massive bloat. See the
    dissassembly of a call to crypto_skcipher_encrypt() in each case:

    With inline stubs (same as original, before the crypto stats feature):

    ffffffff812f6e80 <encrypt>:
    ffffffff812f6e80: 48 8b 47 40 mov 0x40(%rdi),%rax
    ffffffff812f6e84: f6 00 01 testb $0x1,(%rax)
    ffffffff812f6e87: 75 03 jne ffffffff812f6e8c <encrypt+0xc>
    ffffffff812f6e89: ff 60 e0 jmpq *-0x20(%rax)
    ffffffff812f6e8c: b8 82 ff ff ff mov $0xffffff82,%eax
    ffffffff812f6e91: c3 retq

    With non-inline stubs (even when !CONFIG_CRYPTO_STATS):

    ffffffff812f75e0 <encrypt>:
    ffffffff812f75e0: 41 55 push %r13
    ffffffff812f75e2: 41 54 push %r12
    ffffffff812f75e4: 55 push %rbp
    ffffffff812f75e5: 48 89 fd mov %rdi,%rbp
    ffffffff812f75e8: 53 push %rbx
    ffffffff812f75e9: 48 8b 5f 40 mov 0x40(%rdi),%rbx
    ffffffff812f75ed: 44 8b 2f mov (%rdi),%r13d
    ffffffff812f75f0: 4c 8b 63 38 mov 0x38(%rbx),%r12
    ffffffff812f75f4: 4c 89 e7 mov %r12,%rdi
    ffffffff812f75f7: e8 14 df fd ff callq ffffffff812d5510 <crypto_stats_get>
    ffffffff812f75fc: f6 03 01 testb $0x1,(%rbx)
    ffffffff812f75ff: 75 1e jne ffffffff812f761f <encrypt+0x3f>
    ffffffff812f7601: 48 89 ef mov %rbp,%rdi
    ffffffff812f7604: ff 53 e0 callq *-0x20(%rbx)
    ffffffff812f7607: 89 c3 mov %eax,%ebx
    ffffffff812f7609: 4c 89 e2 mov %r12,%rdx
    ffffffff812f760c: 89 de mov %ebx,%esi
    ffffffff812f760e: 44 89 ef mov %r13d,%edi
    ffffffff812f7611: e8 3a de fd ff callq ffffffff812d5450 <crypto_stats_skcipher_encrypt>
    ffffffff812f7616: 89 d8 mov %ebx,%eax
    ffffffff812f7618: 5b pop %rbx
    ffffffff812f7619: 5d pop %rbp
    ffffffff812f761a: 41 5c pop %r12
    ffffffff812f761c: 41 5d pop %r13
    ffffffff812f761e: c3 retq
    ffffffff812f761f: bb 82 ff ff ff mov $0xffffff82,%ebx
    ffffffff812f7624: eb e3 jmp ffffffff812f7609 <encrypt+0x29>

    - Eric
    \
     
     \ /
      Last update: 2018-11-29 00:18    [W:4.336 / U:0.436 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site