| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20220506131627.180784-3-pizhenwei@bytedance.com>
Date: Fri, 6 May 2022 21:16:24 +0800
From: zhenwei pi <pizhenwei@...edance.com>
To: arei.gonglei@...wei.com, mst@...hat.com
Cc: jasowang@...hat.com, herbert@...dor.apana.org.au,
linux-kernel@...r.kernel.org,
virtualization@...ts.linux-foundation.org,
linux-crypto@...r.kernel.org, helei.sig11@...edance.com,
pizhenwei@...edance.com, davem@...emloft.net,
kernel test robot <lkp@...el.com>,
Dan Carpenter <dan.carpenter@...cle.com>
Subject: [PATCH v6 2/5] 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@...el.com>
Reported-by: Dan Carpenter <dan.carpenter@...cle.com>
Cc: Michael S. Tsirkin <mst@...hat.com>
Cc: Jason Wang <jasowang@...hat.com>
Cc: Gonglei <arei.gonglei@...wei.com>
Reviewed-by: Gonglei <arei.gonglei@...wei.com>
Signed-off-by: zhenwei pi <pizhenwei@...edance.com>
---
.../virtio/virtio_crypto_akcipher_algs.c | 57 ++++++++++++-------
drivers/crypto/virtio/virtio_crypto_common.h | 17 ++++--
.../virtio/virtio_crypto_skcipher_algs.c | 50 ++++++++++------
3 files changed, 79 insertions(+), 45 deletions(-)
diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
index 20901a263fc8..698ea57e2649 100644
--- 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 e693d4ee83a6..2422237ec4e6 100644
--- 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 e3c5bc8d6112..6aaf0869b211 100644
--- 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(
--
2.20.1
Powered by blists - more mailing lists