Re: [v3 PATCH 1/3] crypto: algif_skcipher - Do not assume that req is unchanged

[Tadeusz Struk]

Hi Herbert,
On 02/01/2016 05:08 AM, Herbert Xu wrote:
> @@ -509,37 +498,42 @@ static int skcipher_recvmsg_async(struct socket *sock, 
> struct msghdr *msg,
>  {
>       struct sock *sk = sock->sk;
>       struct alg_sock *ask = alg_sk(sk);
> +     struct sock *psk = ask->parent;
> +     struct alg_sock *pask = alg_sk(psk);
>       struct skcipher_ctx *ctx = ask->private;
> +     struct skcipher_tfm *skc = pask->private;
> +     struct crypto_skcipher *tfm = skc->skcipher;
>       struct skcipher_sg_list *sgl;
>       struct scatterlist *sg;
>       struct skcipher_async_req *sreq;
>       struct skcipher_request *req;
>       struct skcipher_async_rsgl *last_rsgl = NULL;
>       unsigned int txbufs = 0, len = 0, tx_nents = skcipher_all_sg_nents(ctx);
> -     unsigned int reqlen = sizeof(struct skcipher_async_req) +
> -                             GET_REQ_SIZE(ctx) + GET_IV_SIZE(ctx);
> +     unsigned int reqsize = crypto_skcipher_reqsize(tfm);
> +     unsigned int ivsize = crypto_skcipher_ivsize(tfm);
>       int err = -ENOMEM;
>       bool mark = false;
> +     char *iv;
>  
> -     lock_sock(sk);
> -     req = kmalloc(reqlen, GFP_KERNEL);
> -     if (unlikely(!req))
> -             goto unlock;
> +     sreq = kzalloc(sizeof(*req) + reqsize + ivsize, GFP_KERNEL);

I think this should be:
sreq = kzalloc(sizeof(*sreq) + reqsize + ivsize, GFP_KERNEL);

> +     if (unlikely(!sreq))
> +             goto out;
>  
> -     sreq = GET_SREQ(req, ctx);
> +     req = &sreq->req;
> +     iv = (char *)(req + 1) + reqsize;
>       sreq->iocb = msg->msg_iocb;
> -     memset(&sreq->first_sgl, '\0', sizeof(struct skcipher_async_rsgl));
>       INIT_LIST_HEAD(&sreq->list);
> +     sreq->inflight = &ctx->inflight;
> +
> +     lock_sock(sk);
>       sreq->tsg = kcalloc(tx_nents, sizeof(*sg), GFP_KERNEL);
> -     if (unlikely(!sreq->tsg)) {
> -             kfree(req);
> +     if (unlikely(!sreq->tsg))
>               goto unlock;
> -     }
>       sg_init_table(sreq->tsg, tx_nents);
> -     memcpy(sreq->iv, ctx->iv, GET_IV_SIZE(ctx));
> -     skcipher_request_set_tfm(req, crypto_skcipher_reqtfm(&ctx->req));
> +     memcpy(iv, ctx->iv, ivsize);
> +     skcipher_request_set_tfm(req, tfm);
>       skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
> -                                   skcipher_async_cb, sk);
> +                                   skcipher_async_cb, sreq);
>  
>       while (iov_iter_count(&msg->msg_iter)) {
>               struct skcipher_async_rsgl *rsgl;
> @@ -615,7 +609,7 @@ static int skcipher_recvmsg_async(struct socket *sock, 
> struct msghdr *msg,
>               sg_mark_end(sreq->tsg + txbufs - 1);
>  
>       skcipher_request_set_crypt(req, sreq->tsg, sreq->first_sgl.sgl.sg,
> -                                len, sreq->iv);
> +                                len, iv);
>       err = ctx->enc ? crypto_skcipher_encrypt(req) :
>                        crypto_skcipher_decrypt(req);
>       if (err == -EINPROGRESS) {
> @@ -625,10 +619,11 @@ static int skcipher_recvmsg_async(struct socket *sock, 
> struct msghdr *msg,
>       }
>  free:
>       skcipher_free_async_sgls(sreq);
> -     kfree(req);
>  unlock:
>       skcipher_wmem_wakeup(sk);
>       release_sock(sk);
> +     kzfree(sreq);

we can't free sreq here because in case if (err == -EINPROGRESS)
we goto unlock and then we crash in the callback.

> +out:
>       return err;
>  }

With the following changes on top, Tested-by: Tadeusz Struk 
<tadeusz.st...@intel.com>
---

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index a692e06..322891a 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -515,7 +515,7 @@ static int skcipher_recvmsg_async(struct socket *sock, 
struct msghdr *msg,
        bool mark = false;
        char *iv;
 
-       sreq = kzalloc(sizeof(*req) + reqsize + ivsize, GFP_KERNEL);
+       sreq = kzalloc(sizeof(*sreq) + reqsize + ivsize, GFP_KERNEL);
        if (unlikely(!sreq))
                goto out;
 
@@ -528,7 +528,7 @@ static int skcipher_recvmsg_async(struct socket *sock, 
struct msghdr *msg,
        lock_sock(sk);
        sreq->tsg = kcalloc(tx_nents, sizeof(*sg), GFP_KERNEL);
        if (unlikely(!sreq->tsg))
-               goto unlock;
+               goto free;
        sg_init_table(sreq->tsg, tx_nents);
        memcpy(iv, ctx->iv, ivsize);
        skcipher_request_set_tfm(req, tfm);
@@ -619,10 +619,10 @@ static int skcipher_recvmsg_async(struct socket *sock, 
struct msghdr *msg,
        }
 free:
        skcipher_free_async_sgls(sreq);
+       kzfree(sreq);
 unlock:
        skcipher_wmem_wakeup(sk);
        release_sock(sk);
-       kzfree(sreq);
 out:
        return err;
 }

Thanks,

-- 
TS
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html