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

[Tadeusz Struk <tadeusz.struk@intel.com>]

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.struk@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