[PATCH v2 0/2] crypto: afalg_skcipher - fixes after skcipher conversion

[PATCH v2 0/2] crypto: afalg_skcipher - fixes after skcipher conversion

[Tadeusz Struk]

Hi Herbert,
While testing the algif_aead async patch, I have rerun the async
algif_skcipher tests and I have found some problems.
There are three different issues around algif_skcipher and skcipher.
Two are skcipher conversion related, and one is a bug in the
algif_skcipher, not related to the conversion. It started to
manifest itself after a fix the the qat driver.

v2 changes:
- pass the original skcipher request in ablkcipher.base.data instead of
  casting it back from the ablkcipher request.
- rename _req to base_req
- dropped 3/3

---

Tadeusz Struk (2):
      crypto: skcipher - return the correct request to the user
      crypto: algif_skcipher - fix async callback after skcipher convertion


 crypto/algif_skcipher.c |    5 +++--
 crypto/skcipher.c       |    9 ++++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)
--

[PATCH v2 1/2] crypto: skcipher - return the correct request to the user

[Tadeusz Struk]

A user of the skcipher api may have some private context associated with
a request, like for instance the algif_skcipher does, so the api needs to
return the original skcipher_request in the callback instead of the
ablkcipher_request subtype.

Cc: <stable@vger.kernel.org> # 4.4.x-
Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
 crypto/skcipher.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/crypto/skcipher.c b/crypto/skcipher.c
index 69230e9..22a9c2a 100644
--- a/crypto/skcipher.c
+++ b/crypto/skcipher.c
@@ -142,6 +142,13 @@ static int skcipher_setkey_ablkcipher(struct crypto_skcipher *tfm,
 	return err;
 }
 
+static void skcipher_complete(struct crypto_async_request *req_base, int err)
+{
+	struct skcipher_request *req = req_base->data;
+
+	req->base.complete(&req->base, err);
+}
+
 static int skcipher_crypt_ablkcipher(struct skcipher_request *req,
 				     int (*crypt)(struct ablkcipher_request *))
 {
@@ -151,7 +158,7 @@ static int skcipher_crypt_ablkcipher(struct skcipher_request *req,
 
 	ablkcipher_request_set_tfm(subreq, *ctx);
 	ablkcipher_request_set_callback(subreq, skcipher_request_flags(req),
-					req->base.complete, req->base.data);
+					skcipher_complete, req);
 	ablkcipher_request_set_crypt(subreq, req->src, req->dst, req->cryptlen,
 				     req->iv);
 

[PATCH v2 2/2] crypto: algif_skcipher - fix async callback after skcipher convertion

[Tadeusz Struk]

Before the skcipher conversion the async callback used the
crypto_async_request directly as the ablkcipher_request.
It wasn't quite correct, but it worked fine since the
crypto_async_request *base was the first member of the ablkcipher_request
struct. After the skcipher conversion it is not the case anymore and
a proper cast helper needs to be used.

Cc: <stable@vger.kernel.org> # 4.4.x-
Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
 crypto/algif_skcipher.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 38c1aa8..927ed7a 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -100,11 +100,12 @@ static void skcipher_free_async_sgls(struct skcipher_async_req *sreq)
 	kfree(sreq->tsg);
 }
 
-static void skcipher_async_cb(struct crypto_async_request *req, int err)
+static void skcipher_async_cb(struct crypto_async_request *req_base, int err)
 {
-	struct sock *sk = req->data;
+	struct sock *sk = req_base->data;
 	struct alg_sock *ask = alg_sk(sk);
 	struct skcipher_ctx *ctx = ask->private;
+	struct skcipher_request *req = skcipher_request_cast(req_base);
 	struct skcipher_async_req *sreq = GET_SREQ(req, ctx);
 	struct kiocb *iocb = sreq->iocb;
 

[v3 PATCH 0/3] crypto: algif_skcipher - fixes after skcipher conversion

[Herbert Xu]

On Thu, Jan 28, 2016 at 07:23:54AM -0800, Tadeusz Struk wrote:
> Hi Herbert,
> While testing the algif_aead async patch, I have rerun the async
> algif_skcipher tests and I have found some problems.
> There are three different issues around algif_skcipher and skcipher.
> Two are skcipher conversion related, and one is a bug in the
> algif_skcipher, not related to the conversion. It started to
> manifest itself after a fix the the qat driver.

OK I've decided to fix this differently.  The expectation of getting
back the original request is incorrect.  There are multiple places
where we will return a request other than the original.

We should probably change the API so that instead of passing the
struct crypto_async_request we just pass in void * instead.  That
way nobody could make this mistake.

Anyway, the following patches should fix the algif_skcipher issue.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Herbert Xu]

The async path in algif_skcipher assumes that the crypto completion
function will be called with the original request.  This is not
necessarily the case.  In fact there is no need for this anyway
since we already embed information into the request with struct
skcipher_async_req.

This patch adds a pointer to that struct and then passes it as
the data to the callback function.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 crypto/algif_skcipher.c |   59 +++++++++++++++++++++---------------------------
 1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 38c1aa8..a692e06 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -65,18 +65,10 @@ struct skcipher_async_req {
 	struct skcipher_async_rsgl first_sgl;
 	struct list_head list;
 	struct scatterlist *tsg;
-	char iv[];
+	atomic_t *inflight;
+	struct skcipher_request req;
 };
 
-#define GET_SREQ(areq, ctx) (struct skcipher_async_req *)((char *)areq + \
-	crypto_skcipher_reqsize(crypto_skcipher_reqtfm(&ctx->req)))
-
-#define GET_REQ_SIZE(ctx) \
-	crypto_skcipher_reqsize(crypto_skcipher_reqtfm(&ctx->req))
-
-#define GET_IV_SIZE(ctx) \
-	crypto_skcipher_ivsize(crypto_skcipher_reqtfm(&ctx->req))
-
 #define MAX_SGL_ENTS ((4096 - sizeof(struct skcipher_sg_list)) / \
 		      sizeof(struct scatterlist) - 1)
 
@@ -102,15 +94,12 @@ static void skcipher_free_async_sgls(struct skcipher_async_req *sreq)
 
 static void skcipher_async_cb(struct crypto_async_request *req, int err)
 {
-	struct sock *sk = req->data;
-	struct alg_sock *ask = alg_sk(sk);
-	struct skcipher_ctx *ctx = ask->private;
-	struct skcipher_async_req *sreq = GET_SREQ(req, ctx);
+	struct skcipher_async_req *sreq = req->data;
 	struct kiocb *iocb = sreq->iocb;
 
-	atomic_dec(&ctx->inflight);
+	atomic_dec(sreq->inflight);
 	skcipher_free_async_sgls(sreq);
-	kfree(req);
+	kzfree(sreq);
 	iocb->ki_complete(iocb, err, err);
 }
 
@@ -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);
+	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);
+out:
 	return err;
 }
 

[v3 PATCH 2/3] crypto: algif_skcipher - Do not dereference ctx without socket lock

[Herbert Xu]

Any access to non-constant bits of the private context must be
done under the socket lock, in particular, this includes ctx->req.

This patch moves such accesses under the lock, and fetches the
tfm from the parent socket which is guaranteed to be constant,
rather than from ctx->req.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 crypto/algif_skcipher.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index a692e06..2d549aa 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -295,8 +295,11 @@ static int skcipher_sendmsg(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 crypto_skcipher *tfm = crypto_skcipher_reqtfm(&ctx->req);
+	struct skcipher_tfm *skc = pask->private;
+	struct crypto_skcipher *tfm = skc->skcipher;
 	unsigned ivsize = crypto_skcipher_ivsize(tfm);
 	struct skcipher_sg_list *sgl;
 	struct af_alg_control con = {};
@@ -508,7 +511,7 @@ static int skcipher_recvmsg_async(struct socket *sock, struct msghdr *msg,
 	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 txbufs = 0, len = 0, tx_nents;
 	unsigned int reqsize = crypto_skcipher_reqsize(tfm);
 	unsigned int ivsize = crypto_skcipher_ivsize(tfm);
 	int err = -ENOMEM;
@@ -526,6 +529,7 @@ static int skcipher_recvmsg_async(struct socket *sock, struct msghdr *msg,
 	sreq->inflight = &ctx->inflight;
 
 	lock_sock(sk);
+	tx_nents = skcipher_all_sg_nents(ctx);
 	sreq->tsg = kcalloc(tx_nents, sizeof(*sg), GFP_KERNEL);
 	if (unlikely(!sreq->tsg))
 		goto unlock;
@@ -632,9 +636,12 @@ static int skcipher_recvmsg_sync(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;
-	unsigned bs = crypto_skcipher_blocksize(crypto_skcipher_reqtfm(
-		&ctx->req));
+	struct skcipher_tfm *skc = pask->private;
+	struct crypto_skcipher *tfm = skc->skcipher;
+	unsigned bs = crypto_skcipher_blocksize(tfm);
 	struct skcipher_sg_list *sgl;
 	struct scatterlist *sg;
 	int err = -EAGAIN;

[v3 PATCH 3/3] crypto: algif_skcipher - Do not set MAY_BACKLOG on the async path

[Herbert Xu]

The async path cannot use MAY_BACKLOG because it is not meant to
block, which is what MAY_BACKLOG does.  On the other hand, both
the sync and async paths can make use of MAY_SLEEP.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 crypto/algif_skcipher.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 2d549aa..41580b4 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -536,7 +536,7 @@ static int skcipher_recvmsg_async(struct socket *sock, struct msghdr *msg,
 	sg_init_table(sreq->tsg, tx_nents);
 	memcpy(iv, ctx->iv, ivsize);
 	skcipher_request_set_tfm(req, tfm);
-	skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
+	skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP,
 				      skcipher_async_cb, sreq);
 
 	while (iov_iter_count(&msg->msg_iter)) {
@@ -949,7 +949,8 @@ static int skcipher_accept_parent_nokey(void *private, struct sock *sk)
 	ask->private = ctx;
 
 	skcipher_request_set_tfm(&ctx->req, skcipher);
-	skcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
+	skcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_SLEEP |
+						 CRYPTO_TFM_REQ_MAY_BACKLOG,
 				      af_alg_complete, &ctx->completion);
 
 	sk->sk_destruct = skcipher_sock_destruct;

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

[v4 PATCH 0/3] crypto: algif_skcipher - fixes after skcipher conversion

[Herbert Xu]

On Mon, Feb 01, 2016 at 01:03:57PM -0800, Tadeusz Struk wrote:
> 
> I think this should be:
> sreq = kzalloc(sizeof(*sreq) + reqsize + ivsize, GFP_KERNEL);

Good catch.  Here is another spin.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Herbert Xu]

The async path in algif_skcipher assumes that the crypto completion
function will be called with the original request.  This is not
necessarily the case.  In fact there is no need for this anyway
since we already embed information into the request with struct
skcipher_async_req.

This patch adds a pointer to that struct and then passes it as
the data to the callback function.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 crypto/algif_skcipher.c |   60 ++++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 32 deletions(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 38c1aa8..ec07a86 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -65,18 +65,10 @@ struct skcipher_async_req {
 	struct skcipher_async_rsgl first_sgl;
 	struct list_head list;
 	struct scatterlist *tsg;
-	char iv[];
+	atomic_t *inflight;
+	struct skcipher_request req;
 };
 
-#define GET_SREQ(areq, ctx) (struct skcipher_async_req *)((char *)areq + \
-	crypto_skcipher_reqsize(crypto_skcipher_reqtfm(&ctx->req)))
-
-#define GET_REQ_SIZE(ctx) \
-	crypto_skcipher_reqsize(crypto_skcipher_reqtfm(&ctx->req))
-
-#define GET_IV_SIZE(ctx) \
-	crypto_skcipher_ivsize(crypto_skcipher_reqtfm(&ctx->req))
-
 #define MAX_SGL_ENTS ((4096 - sizeof(struct skcipher_sg_list)) / \
 		      sizeof(struct scatterlist) - 1)
 
@@ -102,15 +94,12 @@ static void skcipher_free_async_sgls(struct skcipher_async_req *sreq)
 
 static void skcipher_async_cb(struct crypto_async_request *req, int err)
 {
-	struct sock *sk = req->data;
-	struct alg_sock *ask = alg_sk(sk);
-	struct skcipher_ctx *ctx = ask->private;
-	struct skcipher_async_req *sreq = GET_SREQ(req, ctx);
+	struct skcipher_async_req *sreq = req->data;
 	struct kiocb *iocb = sreq->iocb;
 
-	atomic_dec(&ctx->inflight);
+	atomic_dec(sreq->inflight);
 	skcipher_free_async_sgls(sreq);
-	kfree(req);
+	kzfree(sreq);
 	iocb->ki_complete(iocb, err, err);
 }
 
@@ -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(*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,20 +609,22 @@ 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) {
 		atomic_inc(&ctx->inflight);
 		err = -EIOCBQUEUED;
+		sreq = NULL;
 		goto unlock;
 	}
 free:
 	skcipher_free_async_sgls(sreq);
-	kfree(req);
 unlock:
 	skcipher_wmem_wakeup(sk);
 	release_sock(sk);
+	kzfree(sreq);
+out:
 	return err;
 }
 

[v4 PATCH 2/3] crypto: algif_skcipher - Do not dereference ctx without socket lock

[Herbert Xu]

Any access to non-constant bits of the private context must be
done under the socket lock, in particular, this includes ctx->req.

This patch moves such accesses under the lock, and fetches the
tfm from the parent socket which is guaranteed to be constant,
rather than from ctx->req.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 crypto/algif_skcipher.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index ec07a86..ef84353 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -295,8 +295,11 @@ static int skcipher_sendmsg(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 crypto_skcipher *tfm = crypto_skcipher_reqtfm(&ctx->req);
+	struct skcipher_tfm *skc = pask->private;
+	struct crypto_skcipher *tfm = skc->skcipher;
 	unsigned ivsize = crypto_skcipher_ivsize(tfm);
 	struct skcipher_sg_list *sgl;
 	struct af_alg_control con = {};
@@ -508,7 +511,7 @@ static int skcipher_recvmsg_async(struct socket *sock, struct msghdr *msg,
 	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 txbufs = 0, len = 0, tx_nents;
 	unsigned int reqsize = crypto_skcipher_reqsize(tfm);
 	unsigned int ivsize = crypto_skcipher_ivsize(tfm);
 	int err = -ENOMEM;
@@ -526,6 +529,7 @@ static int skcipher_recvmsg_async(struct socket *sock, struct msghdr *msg,
 	sreq->inflight = &ctx->inflight;
 
 	lock_sock(sk);
+	tx_nents = skcipher_all_sg_nents(ctx);
 	sreq->tsg = kcalloc(tx_nents, sizeof(*sg), GFP_KERNEL);
 	if (unlikely(!sreq->tsg))
 		goto unlock;
@@ -633,9 +637,12 @@ static int skcipher_recvmsg_sync(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;
-	unsigned bs = crypto_skcipher_blocksize(crypto_skcipher_reqtfm(
-		&ctx->req));
+	struct skcipher_tfm *skc = pask->private;
+	struct crypto_skcipher *tfm = skc->skcipher;
+	unsigned bs = crypto_skcipher_blocksize(tfm);
 	struct skcipher_sg_list *sgl;
 	struct scatterlist *sg;
 	int err = -EAGAIN;

[v4 PATCH 3/3] crypto: algif_skcipher - Do not set MAY_BACKLOG on the async path

[Herbert Xu]

The async path cannot use MAY_BACKLOG because it is not meant to
block, which is what MAY_BACKLOG does.  On the other hand, both
the sync and async paths can make use of MAY_SLEEP.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 crypto/algif_skcipher.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index ef84353..28556fc 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -536,7 +536,7 @@ static int skcipher_recvmsg_async(struct socket *sock, struct msghdr *msg,
 	sg_init_table(sreq->tsg, tx_nents);
 	memcpy(iv, ctx->iv, ivsize);
 	skcipher_request_set_tfm(req, tfm);
-	skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
+	skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP,
 				      skcipher_async_cb, sreq);
 
 	while (iov_iter_count(&msg->msg_iter)) {
@@ -950,7 +950,8 @@ static int skcipher_accept_parent_nokey(void *private, struct sock *sk)
 	ask->private = ctx;
 
 	skcipher_request_set_tfm(&ctx->req, skcipher);
-	skcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
+	skcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_SLEEP |
+						 CRYPTO_TFM_REQ_MAY_BACKLOG,
 				      af_alg_complete, &ctx->completion);
 
 	sk->sk_destruct = skcipher_sock_destruct;

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

[Tadeusz Struk]

On 02/03/2016 05:39 AM, Herbert Xu wrote:
> The async path in algif_skcipher assumes that the crypto completion
> function will be called with the original request.  This is not
> necessarily the case.  In fact there is no need for this anyway
> since we already embed information into the request with struct
> skcipher_async_req.
> 
> This patch adds a pointer to that struct and then passes it as
> the data to the callback function.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> ---
> 
>  crypto/algif_skcipher.c |   60 ++++++++++++++++++++++--------------------------
>  1 file changed, 28 insertions(+), 32 deletions(-)

Tested-by: Tadeusz Struk <tadeusz.struk@intel.com>

Thanks,
-- 
TS