Re: [v3 PATCH 1/3] crypto: algif_skcipher - Do not assume that req is unchanged
From: Tadeusz Struk <hidden>
Date: 2016-02-01 21:07:55
Subsystem:
crypto api, the rest · Maintainers:
Herbert Xu, "David S. Miller", Linus Torvalds
Hi Herbert, On 02/01/2016 05:08 AM, Herbert Xu wrote:
quoted hunk ↗ jump to hunk
@@ -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);
quoted hunk ↗ jump to hunk
+ 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 [off-list ref] ---
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