virtio-crypto: use private buffer for control request

Originally, all of the control requests share a single buffer(
ctrl & input & ctrl_status fields in struct virtio_crypto), this
allows queue depth 1 only, the performance of control queue gets
limited by this design.

In this patch, each request allocates request buffer dynamically, and
free buffer after request, so the scope protected by ctrl_lock also
get optimized here.
It's possible to optimize control queue depth in the next step.

A necessary comment is already in code, still describe it again:
/*
 * Note: there are padding fields in request, clear them to zero before
 * sending to host to avoid to divulge any information.
 * Ex, virtio_crypto_ctrl_request::ctrl::u::destroy_session::padding[48]
 */
So use kzalloc to allocate buffer of struct virtio_crypto_ctrl_request.

Potentially dereferencing uninitialized variables:
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Gonglei <arei.gonglei@huawei.com>
Reviewed-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
Message-Id: <20220506131627.180784-3-pizhenwei@bytedance.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
index 20901a2..698ea57 100644 (file)
--- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
@@ -108,16 +108,22 @@ static int virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher
        unsigned int num_out = 0, num_in = 0;
        struct virtio_crypto_op_ctrl_req *ctrl;
        struct virtio_crypto_session_input *input;
+       struct virtio_crypto_ctrl_request *vc_ctrl_req;
 
        pkey = kmemdup(key, keylen, GFP_ATOMIC);
        if (!pkey)
                return -ENOMEM;
 
-       spin_lock(&vcrypto->ctrl_lock);
-       ctrl = &vcrypto->ctrl;
+       vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL);
+       if (!vc_ctrl_req) {
+               err = -ENOMEM;
+               goto out;
+       }
+
+       ctrl = &vc_ctrl_req->ctrl;
        memcpy(&ctrl->header, header, sizeof(ctrl->header));
        memcpy(&ctrl->u, para, sizeof(ctrl->u));
-       input = &vcrypto->input;
+       input = &vc_ctrl_req->input;
        input->status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
 
        sg_init_one(&outhdr_sg, ctrl, sizeof(*ctrl));
@@ -129,16 +135,22 @@ static int virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher
        sg_init_one(&inhdr_sg, input, sizeof(*input));
        sgs[num_out + num_in++] = &inhdr_sg;
 
+       spin_lock(&vcrypto->ctrl_lock);
        err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in, vcrypto, GFP_ATOMIC);
-       if (err < 0)
+       if (err < 0) {
+               spin_unlock(&vcrypto->ctrl_lock);
                goto out;
+       }
 
        virtqueue_kick(vcrypto->ctrl_vq);
        while (!virtqueue_get_buf(vcrypto->ctrl_vq, &inlen) &&
               !virtqueue_is_broken(vcrypto->ctrl_vq))
                cpu_relax();
+       spin_unlock(&vcrypto->ctrl_lock);
 
        if (le32_to_cpu(input->status) != VIRTIO_CRYPTO_OK) {
+               pr_err("virtio_crypto: Create session failed status: %u\n",
+                       le32_to_cpu(input->status));
                err = -EINVAL;
                goto out;
        }
@@ -148,13 +160,9 @@ static int virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher
        err = 0;
 
 out:
-       spin_unlock(&vcrypto->ctrl_lock);
+       kfree(vc_ctrl_req);
        kfree_sensitive(pkey);
 
-       if (err < 0)
-               pr_err("virtio_crypto: Create session failed status: %u\n",
-                       le32_to_cpu(input->status));
-
        return err;
 }
 
@@ -167,15 +175,18 @@ static int virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
        int err;
        struct virtio_crypto_op_ctrl_req *ctrl;
        struct virtio_crypto_inhdr *ctrl_status;
+       struct virtio_crypto_ctrl_request *vc_ctrl_req;
 
-       spin_lock(&vcrypto->ctrl_lock);
-       if (!ctx->session_valid) {
-               err = 0;
-               goto out;
-       }
-       ctrl_status = &vcrypto->ctrl_status;
+       if (!ctx->session_valid)
+               return 0;
+
+       vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL);
+       if (!vc_ctrl_req)
+               return -ENOMEM;
+
+       ctrl_status = &vc_ctrl_req->ctrl_status;
        ctrl_status->status = VIRTIO_CRYPTO_ERR;
-       ctrl = &vcrypto->ctrl;
+       ctrl = &vc_ctrl_req->ctrl;
        ctrl->header.opcode = cpu_to_le32(VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION);
        ctrl->header.queue_id = 0;
 
@@ -188,16 +199,22 @@ static int virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
        sg_init_one(&inhdr_sg, &ctrl_status->status, sizeof(ctrl_status->status));
        sgs[num_out + num_in++] = &inhdr_sg;
 
+       spin_lock(&vcrypto->ctrl_lock);
        err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in, vcrypto, GFP_ATOMIC);
-       if (err < 0)
+       if (err < 0) {
+               spin_unlock(&vcrypto->ctrl_lock);
                goto out;
+       }
 
        virtqueue_kick(vcrypto->ctrl_vq);
        while (!virtqueue_get_buf(vcrypto->ctrl_vq, &inlen) &&
               !virtqueue_is_broken(vcrypto->ctrl_vq))
                cpu_relax();
+       spin_unlock(&vcrypto->ctrl_lock);
 
        if (ctrl_status->status != VIRTIO_CRYPTO_OK) {
+               pr_err("virtio_crypto: Close session failed status: %u, session_id: 0x%llx\n",
+                       ctrl_status->status, destroy_session->session_id);
                err = -EINVAL;
                goto out;
        }
@@ -206,11 +223,7 @@ static int virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
        ctx->session_valid = false;
 
 out:
-       spin_unlock(&vcrypto->ctrl_lock);
-       if (err < 0) {
-               pr_err("virtio_crypto: Close session failed status: %u, session_id: 0x%llx\n",
-                       ctrl_status->status, destroy_session->session_id);
-       }
+       kfree(vc_ctrl_req);
 
        return err;
 }

diff --git a/drivers/crypto/virtio/virtio_crypto_common.h b/drivers/crypto/virtio/virtio_crypto_common.h
index e693d4e..2422237 100644 (file)
--- a/drivers/crypto/virtio/virtio_crypto_common.h
+++ b/drivers/crypto/virtio/virtio_crypto_common.h
@@ -13,6 +13,7 @@
 #include <crypto/aead.h>
 #include <crypto/aes.h>
 #include <crypto/engine.h>
+#include <uapi/linux/virtio_crypto.h>
 
 
 /* Internal representation of a data virtqueue */
@@ -65,11 +66,6 @@ struct virtio_crypto {
        /* Maximum size of per request */
        u64 max_size;
 
-       /* Control VQ buffers: protected by the ctrl_lock */
-       struct virtio_crypto_op_ctrl_req ctrl;
-       struct virtio_crypto_session_input input;
-       struct virtio_crypto_inhdr ctrl_status;
-
        unsigned long status;
        atomic_t ref_count;
        struct list_head list;
@@ -85,6 +81,17 @@ struct virtio_crypto_sym_session_info {
        __u64 session_id;
 };
 
+/*
+ * Note: there are padding fields in request, clear them to zero before
+ *       sending to host to avoid to divulge any information.
+ * Ex, virtio_crypto_ctrl_request::ctrl::u::destroy_session::padding[48]
+ */
+struct virtio_crypto_ctrl_request {
+       struct virtio_crypto_op_ctrl_req ctrl;
+       struct virtio_crypto_session_input input;
+       struct virtio_crypto_inhdr ctrl_status;
+};
+
 struct virtio_crypto_request;
 typedef void (*virtio_crypto_data_callback)
                (struct virtio_crypto_request *vc_req, int len);

diff --git a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
index e3c5bc8..6aaf086 100644 (file)
--- a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
@@ -126,6 +126,7 @@ static int virtio_crypto_alg_skcipher_init_session(
        struct virtio_crypto_op_ctrl_req *ctrl;
        struct virtio_crypto_session_input *input;
        struct virtio_crypto_sym_create_session_req *sym_create_session;
+       struct virtio_crypto_ctrl_request *vc_ctrl_req;
 
        /*
         * Avoid to do DMA from the stack, switch to using
@@ -136,15 +137,20 @@ static int virtio_crypto_alg_skcipher_init_session(
        if (!cipher_key)
                return -ENOMEM;
 
-       spin_lock(&vcrypto->ctrl_lock);
+       vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL);
+       if (!vc_ctrl_req) {
+               err = -ENOMEM;
+               goto out;
+       }
+
        /* Pad ctrl header */
-       ctrl = &vcrypto->ctrl;
+       ctrl = &vc_ctrl_req->ctrl;
        ctrl->header.opcode = cpu_to_le32(VIRTIO_CRYPTO_CIPHER_CREATE_SESSION);
        ctrl->header.algo = cpu_to_le32(alg);
        /* Set the default dataqueue id to 0 */
        ctrl->header.queue_id = 0;
 
-       input = &vcrypto->input;
+       input = &vc_ctrl_req->input;
        input->status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
        /* Pad cipher's parameters */
        sym_create_session = &ctrl->u.sym_create_session;
@@ -164,12 +170,12 @@ static int virtio_crypto_alg_skcipher_init_session(
        sg_init_one(&inhdr, input, sizeof(*input));
        sgs[num_out + num_in++] = &inhdr;
 
+       spin_lock(&vcrypto->ctrl_lock);
        err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out,
                                num_in, vcrypto, GFP_ATOMIC);
        if (err < 0) {
                spin_unlock(&vcrypto->ctrl_lock);
-               kfree_sensitive(cipher_key);
-               return err;
+               goto out;
        }
        virtqueue_kick(vcrypto->ctrl_vq);
 
@@ -180,13 +186,13 @@ static int virtio_crypto_alg_skcipher_init_session(
        while (!virtqueue_get_buf(vcrypto->ctrl_vq, &tmp) &&
               !virtqueue_is_broken(vcrypto->ctrl_vq))
                cpu_relax();
+       spin_unlock(&vcrypto->ctrl_lock);
 
        if (le32_to_cpu(input->status) != VIRTIO_CRYPTO_OK) {
-               spin_unlock(&vcrypto->ctrl_lock);
                pr_err("virtio_crypto: Create session failed status: %u\n",
                        le32_to_cpu(input->status));
-               kfree_sensitive(cipher_key);
-               return -EINVAL;
+               err = -EINVAL;
+               goto out;
        }
 
        if (encrypt)
@@ -194,10 +200,11 @@ static int virtio_crypto_alg_skcipher_init_session(
        else
                ctx->dec_sess_info.session_id = le64_to_cpu(input->session_id);
 
-       spin_unlock(&vcrypto->ctrl_lock);
-
+       err = 0;
+out:
+       kfree(vc_ctrl_req);
        kfree_sensitive(cipher_key);
-       return 0;
+       return err;
 }
 
 static int virtio_crypto_alg_skcipher_close_session(
@@ -212,12 +219,16 @@ static int virtio_crypto_alg_skcipher_close_session(
        unsigned int num_out = 0, num_in = 0;
        struct virtio_crypto_op_ctrl_req *ctrl;
        struct virtio_crypto_inhdr *ctrl_status;
+       struct virtio_crypto_ctrl_request *vc_ctrl_req;
 
-       spin_lock(&vcrypto->ctrl_lock);
-       ctrl_status = &vcrypto->ctrl_status;
+       vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL);
+       if (!vc_ctrl_req)
+               return -ENOMEM;
+
+       ctrl_status = &vc_ctrl_req->ctrl_status;
        ctrl_status->status = VIRTIO_CRYPTO_ERR;
        /* Pad ctrl header */
-       ctrl = &vcrypto->ctrl;
+       ctrl = &vc_ctrl_req->ctrl;
        ctrl->header.opcode = cpu_to_le32(VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION);
        /* Set the default virtqueue id to 0 */
        ctrl->header.queue_id = 0;
@@ -236,28 +247,31 @@ static int virtio_crypto_alg_skcipher_close_session(
        sg_init_one(&status_sg, &ctrl_status->status, sizeof(ctrl_status->status));
        sgs[num_out + num_in++] = &status_sg;
 
+       spin_lock(&vcrypto->ctrl_lock);
        err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out,
                        num_in, vcrypto, GFP_ATOMIC);
        if (err < 0) {
                spin_unlock(&vcrypto->ctrl_lock);
-               return err;
+               goto out;
        }
        virtqueue_kick(vcrypto->ctrl_vq);
 
        while (!virtqueue_get_buf(vcrypto->ctrl_vq, &tmp) &&
               !virtqueue_is_broken(vcrypto->ctrl_vq))
                cpu_relax();
+       spin_unlock(&vcrypto->ctrl_lock);
 
        if (ctrl_status->status != VIRTIO_CRYPTO_OK) {
-               spin_unlock(&vcrypto->ctrl_lock);
                pr_err("virtio_crypto: Close session failed status: %u, session_id: 0x%llx\n",
                        ctrl_status->status, destroy_session->session_id);
 
                return -EINVAL;
        }
-       spin_unlock(&vcrypto->ctrl_lock);
 
-       return 0;
+       err = 0;
+out:
+       kfree(vc_ctrl_req);
+       return err;
 }
 
 static int virtio_crypto_alg_skcipher_init_sessions(