All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH 00/17] Add validation for used length
@ 2021-05-17  9:08 Xie Yongji
  2021-05-17  9:08 ` [RFC PATCH 01/17] virtio_ring: Avoid reading unneeded " Xie Yongji
                   ` (17 more replies)
  0 siblings, 18 replies; 32+ messages in thread
From: Xie Yongji @ 2021-05-17  9:08 UTC (permalink / raw)
  To: mst, jasowang, stefanha
  Cc: amit, arei.gonglei, airlied, kraxel, dan.j.williams, johannes,
	ohad, bjorn.andersson, david, vgoyal, miklos, sgarzare,
	virtualization, linux-kernel

Current virtio device drivers may trust the used length returned
in virtqueue_get_buf()/virtqueue_get_buf_ctx(). But the used length
might come from an untrusted device when VDUSE[1] is enabled. To
protect this case, this series tries to add validation for the
used length.

Since many legacy devices will also set the used length incorrectly,
we did not add the validation unconditionally. Instead, we will do
the validation only when the device driver needs the used length.
A NULL len passed to virtqueue_get_buf()/virtqueue_get_buf_ctx()
will mean the used length is not needed by the device driver.

[1] https://lore.kernel.org/kvm/20210331080519.172-1-xieyongji@bytedance.com/

Xie Yongji (17):
  virtio_ring: Avoid reading unneeded used length
  virtio-blk: Remove unused used length
  virtio_console: Remove unused used length
  crypto: virtio - Remove unused used length
  drm/virtio: Remove unused used length
  caif_virtio: Remove unused used length
  virtio_net: Remove unused used length
  mac80211_hwsim: Remove unused used length
  virtio_pmem: Remove unused used length
  rpmsg: virtio: Remove unused used length
  virtio_scsi: Remove unused used length
  virtio_balloon: Remove unused used length
  virtio_input: Remove unused used length
  virtio_mem: Remove unused used length
  virtiofs: Remove unused used length
  vsock: Remove unused used length
  virtio_ring: Add validation for used length

 drivers/block/virtio_blk.c                 |  3 +--
 drivers/char/virtio_console.c              | 12 ++++--------
 drivers/crypto/virtio/virtio_crypto_algs.c |  6 ++----
 drivers/gpu/drm/virtio/virtgpu_vq.c        |  3 +--
 drivers/net/caif/caif_virtio.c             |  3 +--
 drivers/net/virtio_net.c                   | 10 ++++------
 drivers/net/wireless/mac80211_hwsim.c      |  3 +--
 drivers/nvdimm/nd_virtio.c                 |  3 +--
 drivers/rpmsg/virtio_rpmsg_bus.c           |  3 +--
 drivers/scsi/virtio_scsi.c                 |  3 +--
 drivers/virtio/virtio_balloon.c            | 21 ++++++++++-----------
 drivers/virtio/virtio_input.c              |  6 ++----
 drivers/virtio/virtio_mem.c                |  3 +--
 drivers/virtio/virtio_ring.c               | 28 +++++++++++++++++++++++-----
 fs/fuse/virtio_fs.c                        |  6 ++----
 net/vmw_vsock/virtio_transport.c           |  3 +--
 16 files changed, 56 insertions(+), 60 deletions(-)

-- 
2.11.0


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC PATCH 01/17] virtio_ring: Avoid reading unneeded used length
  2021-05-17  9:08 [RFC PATCH 00/17] Add validation for used length Xie Yongji
@ 2021-05-17  9:08 ` Xie Yongji
  2021-05-17  9:11     ` Johannes Berg
  2021-05-17  9:08 ` [RFC PATCH 02/17] virtio-blk: Remove unused " Xie Yongji
                   ` (16 subsequent siblings)
  17 siblings, 1 reply; 32+ messages in thread
From: Xie Yongji @ 2021-05-17  9:08 UTC (permalink / raw)
  To: mst, jasowang, stefanha
  Cc: amit, arei.gonglei, airlied, kraxel, dan.j.williams, johannes,
	ohad, bjorn.andersson, david, vgoyal, miklos, sgarzare,
	virtualization, linux-kernel

If device driver doesn't need used length, it can pass a NULL
len in virtqueue_get_buf()/virtqueue_get_buf_ctx(). Then
we can avoid reading and validating the len value in used
ring entries.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 drivers/virtio/virtio_ring.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 54e12dd91310..d999a1d6d271 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -772,7 +772,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
 	last_used = (vq->last_used_idx & (vq->split.vring.num - 1));
 	i = virtio32_to_cpu(_vq->vdev,
 			vq->split.vring.used->ring[last_used].id);
-	*len = virtio32_to_cpu(_vq->vdev,
+	if (len)
+		*len = virtio32_to_cpu(_vq->vdev,
 			vq->split.vring.used->ring[last_used].len);
 
 	if (unlikely(i >= vq->split.vring.num)) {
@@ -1444,7 +1445,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
 
 	last_used = vq->last_used_idx;
 	id = le16_to_cpu(vq->packed.vring.desc[last_used].id);
-	*len = le32_to_cpu(vq->packed.vring.desc[last_used].len);
+	if (len)
+		*len = le32_to_cpu(vq->packed.vring.desc[last_used].len);
 
 	if (unlikely(id >= vq->packed.vring.num)) {
 		BAD_RING(vq, "id %u out of range\n", id);
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 02/17] virtio-blk: Remove unused used length
  2021-05-17  9:08 [RFC PATCH 00/17] Add validation for used length Xie Yongji
  2021-05-17  9:08 ` [RFC PATCH 01/17] virtio_ring: Avoid reading unneeded " Xie Yongji
@ 2021-05-17  9:08 ` Xie Yongji
  2021-05-17  9:08 ` [RFC PATCH 03/17] virtio_console: " Xie Yongji
                   ` (15 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Xie Yongji @ 2021-05-17  9:08 UTC (permalink / raw)
  To: mst, jasowang, stefanha
  Cc: amit, arei.gonglei, airlied, kraxel, dan.j.williams, johannes,
	ohad, bjorn.andersson, david, vgoyal, miklos, sgarzare,
	virtualization, linux-kernel

The used length is not used. Let's drop it and
pass NULL to virtqueue_get_buf() instead.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 drivers/block/virtio_blk.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 425bae618131..255adb7a768c 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -178,12 +178,11 @@ static void virtblk_done(struct virtqueue *vq)
 	int qid = vq->index;
 	struct virtblk_req *vbr;
 	unsigned long flags;
-	unsigned int len;
 
 	spin_lock_irqsave(&vblk->vqs[qid].lock, flags);
 	do {
 		virtqueue_disable_cb(vq);
-		while ((vbr = virtqueue_get_buf(vblk->vqs[qid].vq, &len)) != NULL) {
+		while ((vbr = virtqueue_get_buf(vblk->vqs[qid].vq, NULL)) != NULL) {
 			struct request *req = blk_mq_rq_from_pdu(vbr);
 
 			if (likely(!blk_should_fake_timeout(req->q)))
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 03/17] virtio_console: Remove unused used length
  2021-05-17  9:08 [RFC PATCH 00/17] Add validation for used length Xie Yongji
  2021-05-17  9:08 ` [RFC PATCH 01/17] virtio_ring: Avoid reading unneeded " Xie Yongji
  2021-05-17  9:08 ` [RFC PATCH 02/17] virtio-blk: Remove unused " Xie Yongji
@ 2021-05-17  9:08 ` Xie Yongji
  2021-05-17  9:08 ` [RFC PATCH 04/17] crypto: virtio - " Xie Yongji
                   ` (14 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Xie Yongji @ 2021-05-17  9:08 UTC (permalink / raw)
  To: mst, jasowang, stefanha
  Cc: amit, arei.gonglei, airlied, kraxel, dan.j.williams, johannes,
	ohad, bjorn.andersson, david, vgoyal, miklos, sgarzare,
	virtualization, linux-kernel

The used length is not used in some cases. Let's drop it
and pass NULL to virtqueue_get_buf() instead.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 drivers/char/virtio_console.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 1836cc56e357..b85abe1eb2d1 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -549,7 +549,6 @@ static ssize_t __send_control_msg(struct ports_device *portdev, u32 port_id,
 {
 	struct scatterlist sg[1];
 	struct virtqueue *vq;
-	unsigned int len;
 
 	if (!use_multiport(portdev))
 		return 0;
@@ -566,7 +565,7 @@ static ssize_t __send_control_msg(struct ports_device *portdev, u32 port_id,
 
 	if (virtqueue_add_outbuf(vq, sg, 1, &portdev->cpkt, GFP_ATOMIC) == 0) {
 		virtqueue_kick(vq);
-		while (!virtqueue_get_buf(vq, &len)
+		while (!virtqueue_get_buf(vq, NULL)
 			&& !virtqueue_is_broken(vq))
 			cpu_relax();
 	}
@@ -589,13 +588,12 @@ static ssize_t send_control_msg(struct port *port, unsigned int event,
 static void reclaim_consumed_buffers(struct port *port)
 {
 	struct port_buffer *buf;
-	unsigned int len;
 
 	if (!port->portdev) {
 		/* Device has been unplugged.  vqs are already gone. */
 		return;
 	}
-	while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
+	while ((buf = virtqueue_get_buf(port->out_vq, NULL))) {
 		free_buf(buf, false);
 		port->outvq_full = false;
 	}
@@ -608,7 +606,6 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
 	struct virtqueue *out_vq;
 	int err;
 	unsigned long flags;
-	unsigned int len;
 
 	out_vq = port->out_vq;
 
@@ -641,7 +638,7 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
 	 * we need to kmalloc a GFP_ATOMIC buffer each time the
 	 * console driver writes something out.
 	 */
-	while (!virtqueue_get_buf(out_vq, &len)
+	while (!virtqueue_get_buf(out_vq, NULL)
 		&& !virtqueue_is_broken(out_vq))
 		cpu_relax();
 done:
@@ -1730,9 +1727,8 @@ static void control_work_handler(struct work_struct *work)
 static void flush_bufs(struct virtqueue *vq, bool can_sleep)
 {
 	struct port_buffer *buf;
-	unsigned int len;
 
-	while ((buf = virtqueue_get_buf(vq, &len)))
+	while ((buf = virtqueue_get_buf(vq, NULL)))
 		free_buf(buf, can_sleep);
 }
 
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 04/17] crypto: virtio - Remove unused used length
  2021-05-17  9:08 [RFC PATCH 00/17] Add validation for used length Xie Yongji
                   ` (2 preceding siblings ...)
  2021-05-17  9:08 ` [RFC PATCH 03/17] virtio_console: " Xie Yongji
@ 2021-05-17  9:08 ` Xie Yongji
  2021-05-17  9:08 ` [RFC PATCH 05/17] drm/virtio: " Xie Yongji
                   ` (13 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Xie Yongji @ 2021-05-17  9:08 UTC (permalink / raw)
  To: mst, jasowang, stefanha
  Cc: amit, arei.gonglei, airlied, kraxel, dan.j.williams, johannes,
	ohad, bjorn.andersson, david, vgoyal, miklos, sgarzare,
	virtualization, linux-kernel

The used length is not used in some cases. Let's drop it
and pass NULL to virtqueue_get_buf() instead.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 drivers/crypto/virtio/virtio_crypto_algs.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c b/drivers/crypto/virtio/virtio_crypto_algs.c
index 583c0b535d13..818fe31ace38 100644
--- a/drivers/crypto/virtio/virtio_crypto_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_algs.c
@@ -118,7 +118,6 @@ static int virtio_crypto_alg_skcipher_init_session(
 		int encrypt)
 {
 	struct scatterlist outhdr, key_sg, inhdr, *sgs[3];
-	unsigned int tmp;
 	struct virtio_crypto *vcrypto = ctx->vcrypto;
 	int op = encrypt ? VIRTIO_CRYPTO_OP_ENCRYPT : VIRTIO_CRYPTO_OP_DECRYPT;
 	int err;
@@ -176,7 +175,7 @@ static int virtio_crypto_alg_skcipher_init_session(
 	 * Trapping into the hypervisor, so the request should be
 	 * handled immediately.
 	 */
-	while (!virtqueue_get_buf(vcrypto->ctrl_vq, &tmp) &&
+	while (!virtqueue_get_buf(vcrypto->ctrl_vq, NULL) &&
 	       !virtqueue_is_broken(vcrypto->ctrl_vq))
 		cpu_relax();
 
@@ -206,7 +205,6 @@ static int virtio_crypto_alg_skcipher_close_session(
 		int encrypt)
 {
 	struct scatterlist outhdr, status_sg, *sgs[2];
-	unsigned int tmp;
 	struct virtio_crypto_destroy_session_req *destroy_session;
 	struct virtio_crypto *vcrypto = ctx->vcrypto;
 	int err;
@@ -245,7 +243,7 @@ static int virtio_crypto_alg_skcipher_close_session(
 	}
 	virtqueue_kick(vcrypto->ctrl_vq);
 
-	while (!virtqueue_get_buf(vcrypto->ctrl_vq, &tmp) &&
+	while (!virtqueue_get_buf(vcrypto->ctrl_vq, NULL) &&
 	       !virtqueue_is_broken(vcrypto->ctrl_vq))
 		cpu_relax();
 
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 05/17] drm/virtio: Remove unused used length
  2021-05-17  9:08 [RFC PATCH 00/17] Add validation for used length Xie Yongji
                   ` (3 preceding siblings ...)
  2021-05-17  9:08 ` [RFC PATCH 04/17] crypto: virtio - " Xie Yongji
@ 2021-05-17  9:08 ` Xie Yongji
  2021-05-17  9:08 ` [RFC PATCH 06/17] caif_virtio: " Xie Yongji
                   ` (12 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Xie Yongji @ 2021-05-17  9:08 UTC (permalink / raw)
  To: mst, jasowang, stefanha
  Cc: amit, arei.gonglei, airlied, kraxel, dan.j.williams, johannes,
	ohad, bjorn.andersson, david, vgoyal, miklos, sgarzare,
	virtualization, linux-kernel

The used length is not used. Let's drop it and
pass NULL to virtqueue_get_buf() instead.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 drivers/gpu/drm/virtio/virtgpu_vq.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index cf84d382dd41..4b164bacc68e 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -186,10 +186,9 @@ static void free_vbuf(struct virtio_gpu_device *vgdev,
 static void reclaim_vbufs(struct virtqueue *vq, struct list_head *reclaim_list)
 {
 	struct virtio_gpu_vbuffer *vbuf;
-	unsigned int len;
 	int freed = 0;
 
-	while ((vbuf = virtqueue_get_buf(vq, &len))) {
+	while ((vbuf = virtqueue_get_buf(vq, NULL))) {
 		list_add_tail(&vbuf->list, reclaim_list);
 		freed++;
 	}
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 06/17] caif_virtio: Remove unused used length
  2021-05-17  9:08 [RFC PATCH 00/17] Add validation for used length Xie Yongji
                   ` (4 preceding siblings ...)
  2021-05-17  9:08 ` [RFC PATCH 05/17] drm/virtio: " Xie Yongji
@ 2021-05-17  9:08 ` Xie Yongji
  2021-05-17  9:08 ` [RFC PATCH 07/17] virtio_net: " Xie Yongji
                   ` (11 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Xie Yongji @ 2021-05-17  9:08 UTC (permalink / raw)
  To: mst, jasowang, stefanha
  Cc: amit, arei.gonglei, airlied, kraxel, dan.j.williams, johannes,
	ohad, bjorn.andersson, david, vgoyal, miklos, sgarzare,
	virtualization, linux-kernel

The used length is not used. Let's drop it and
pass NULL to virtqueue_get_buf() instead.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 drivers/net/caif/caif_virtio.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/caif/caif_virtio.c b/drivers/net/caif/caif_virtio.c
index 106f089eb2a8..020ce35c2d65 100644
--- a/drivers/net/caif/caif_virtio.c
+++ b/drivers/net/caif/caif_virtio.c
@@ -165,12 +165,11 @@ static void cfv_release_used_buf(struct virtqueue *vq_tx)
 	BUG_ON(vq_tx != cfv->vq_tx);
 
 	for (;;) {
-		unsigned int len;
 		struct buf_info *buf_info;
 
 		/* Get used buffer from used ring to recycle used descriptors */
 		spin_lock_irqsave(&cfv->tx_lock, flags);
-		buf_info = virtqueue_get_buf(vq_tx, &len);
+		buf_info = virtqueue_get_buf(vq_tx, NULL);
 		spin_unlock_irqrestore(&cfv->tx_lock, flags);
 
 		/* Stop looping if there are no more buffers to free */
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 07/17] virtio_net: Remove unused used length
  2021-05-17  9:08 [RFC PATCH 00/17] Add validation for used length Xie Yongji
                   ` (5 preceding siblings ...)
  2021-05-17  9:08 ` [RFC PATCH 06/17] caif_virtio: " Xie Yongji
@ 2021-05-17  9:08 ` Xie Yongji
  2021-05-17  9:08 ` [RFC PATCH 08/17] mac80211_hwsim: " Xie Yongji
                   ` (10 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Xie Yongji @ 2021-05-17  9:08 UTC (permalink / raw)
  To: mst, jasowang, stefanha
  Cc: amit, arei.gonglei, airlied, kraxel, dan.j.williams, johannes,
	ohad, bjorn.andersson, david, vgoyal, miklos, sgarzare,
	virtualization, linux-kernel

The used length is not used in some cases. Let's drop it
and pass NULL to virtqueue_get_buf() instead.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 drivers/net/virtio_net.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 5ca7d6780add..01e54e99cae6 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -500,7 +500,6 @@ static int virtnet_xdp_xmit(struct net_device *dev,
 	struct receive_queue *rq = vi->rq;
 	struct bpf_prog *xdp_prog;
 	struct send_queue *sq;
-	unsigned int len;
 	int packets = 0;
 	int bytes = 0;
 	int drops = 0;
@@ -525,7 +524,7 @@ static int virtnet_xdp_xmit(struct net_device *dev,
 	}
 
 	/* Free up any pending old buffers before queueing new ones. */
-	while ((ptr = virtqueue_get_buf(sq->vq, &len)) != NULL) {
+	while ((ptr = virtqueue_get_buf(sq->vq, NULL)) != NULL) {
 		if (likely(is_xdp_frame(ptr))) {
 			struct xdp_frame *frame = ptr_to_xdp(ptr);
 
@@ -1378,12 +1377,11 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
 
 static void free_old_xmit_skbs(struct send_queue *sq, bool in_napi)
 {
-	unsigned int len;
 	unsigned int packets = 0;
 	unsigned int bytes = 0;
 	void *ptr;
 
-	while ((ptr = virtqueue_get_buf(sq->vq, &len)) != NULL) {
+	while ((ptr = virtqueue_get_buf(sq->vq, NULL)) != NULL) {
 		if (likely(!is_xdp_frame(ptr))) {
 			struct sk_buff *skb = ptr;
 
@@ -1681,7 +1679,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
 				 struct scatterlist *out)
 {
 	struct scatterlist *sgs[4], hdr, stat;
-	unsigned out_num = 0, tmp;
+	unsigned out_num = 0;
 
 	/* Caller should know better */
 	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
@@ -1709,7 +1707,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
 	/* Spin for a response, the kick causes an ioport write, trapping
 	 * into the hypervisor, so the request should be handled immediately.
 	 */
-	while (!virtqueue_get_buf(vi->cvq, &tmp) &&
+	while (!virtqueue_get_buf(vi->cvq, NULL) &&
 	       !virtqueue_is_broken(vi->cvq))
 		cpu_relax();
 
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 08/17] mac80211_hwsim: Remove unused used length
  2021-05-17  9:08 [RFC PATCH 00/17] Add validation for used length Xie Yongji
                   ` (6 preceding siblings ...)
  2021-05-17  9:08 ` [RFC PATCH 07/17] virtio_net: " Xie Yongji
@ 2021-05-17  9:08 ` Xie Yongji
  2021-05-17  9:08 ` [RFC PATCH 09/17] virtio_pmem: " Xie Yongji
                   ` (9 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Xie Yongji @ 2021-05-17  9:08 UTC (permalink / raw)
  To: mst, jasowang, stefanha
  Cc: amit, arei.gonglei, airlied, kraxel, dan.j.williams, johannes,
	ohad, bjorn.andersson, david, vgoyal, miklos, sgarzare,
	virtualization, linux-kernel

The used length is not used in some cases. Let's drop it
and pass NULL to virtqueue_get_buf() instead.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 drivers/net/wireless/mac80211_hwsim.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index fa7d4c20dc13..56d1484e7efa 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -4210,12 +4210,11 @@ static void hwsim_exit_netlink(void)
 #if IS_REACHABLE(CONFIG_VIRTIO)
 static void hwsim_virtio_tx_done(struct virtqueue *vq)
 {
-	unsigned int len;
 	struct sk_buff *skb;
 	unsigned long flags;
 
 	spin_lock_irqsave(&hwsim_virtio_lock, flags);
-	while ((skb = virtqueue_get_buf(vq, &len)))
+	while ((skb = virtqueue_get_buf(vq, NULL)))
 		nlmsg_free(skb);
 	spin_unlock_irqrestore(&hwsim_virtio_lock, flags);
 }
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 09/17] virtio_pmem: Remove unused used length
  2021-05-17  9:08 [RFC PATCH 00/17] Add validation for used length Xie Yongji
                   ` (7 preceding siblings ...)
  2021-05-17  9:08 ` [RFC PATCH 08/17] mac80211_hwsim: " Xie Yongji
@ 2021-05-17  9:08 ` Xie Yongji
  2021-05-17  9:08 ` [RFC PATCH 10/17] rpmsg: virtio: " Xie Yongji
                   ` (8 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Xie Yongji @ 2021-05-17  9:08 UTC (permalink / raw)
  To: mst, jasowang, stefanha
  Cc: amit, arei.gonglei, airlied, kraxel, dan.j.williams, johannes,
	ohad, bjorn.andersson, david, vgoyal, miklos, sgarzare,
	virtualization, linux-kernel

The used length is not used. Let's drop it and
pass NULL to virtqueue_get_buf() instead.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 drivers/nvdimm/nd_virtio.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c
index 10351d5b49fa..f57d7663b505 100644
--- a/drivers/nvdimm/nd_virtio.c
+++ b/drivers/nvdimm/nd_virtio.c
@@ -15,10 +15,9 @@ void virtio_pmem_host_ack(struct virtqueue *vq)
 	struct virtio_pmem *vpmem = vq->vdev->priv;
 	struct virtio_pmem_request *req_data, *req_buf;
 	unsigned long flags;
-	unsigned int len;
 
 	spin_lock_irqsave(&vpmem->pmem_lock, flags);
-	while ((req_data = virtqueue_get_buf(vq, &len)) != NULL) {
+	while ((req_data = virtqueue_get_buf(vq, NULL)) != NULL) {
 		req_data->done = true;
 		wake_up(&req_data->host_acked);
 
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 10/17] rpmsg: virtio: Remove unused used length
  2021-05-17  9:08 [RFC PATCH 00/17] Add validation for used length Xie Yongji
                   ` (8 preceding siblings ...)
  2021-05-17  9:08 ` [RFC PATCH 09/17] virtio_pmem: " Xie Yongji
@ 2021-05-17  9:08 ` Xie Yongji
  2021-05-17  9:08 ` [RFC PATCH 11/17] virtio_scsi: " Xie Yongji
                   ` (7 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Xie Yongji @ 2021-05-17  9:08 UTC (permalink / raw)
  To: mst, jasowang, stefanha
  Cc: amit, arei.gonglei, airlied, kraxel, dan.j.williams, johannes,
	ohad, bjorn.andersson, david, vgoyal, miklos, sgarzare,
	virtualization, linux-kernel

The used length is not used in some cases. Let's drop it
and pass NULL to virtqueue_get_buf() instead.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index e87d4cf926eb..10df8bc0c3e1 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -437,7 +437,6 @@ static struct rpmsg_device *__rpmsg_create_channel(struct virtproc_info *vrp,
 /* super simple buffer "allocator" that is just enough for now */
 static void *get_a_tx_buf(struct virtproc_info *vrp)
 {
-	unsigned int len;
 	void *ret;
 
 	/* support multiple concurrent senders */
@@ -451,7 +450,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
 		ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
 	/* or recycle a used one */
 	else
-		ret = virtqueue_get_buf(vrp->svq, &len);
+		ret = virtqueue_get_buf(vrp->svq, NULL);
 
 	mutex_unlock(&vrp->tx_lock);
 
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 11/17] virtio_scsi: Remove unused used length
  2021-05-17  9:08 [RFC PATCH 00/17] Add validation for used length Xie Yongji
                   ` (9 preceding siblings ...)
  2021-05-17  9:08 ` [RFC PATCH 10/17] rpmsg: virtio: " Xie Yongji
@ 2021-05-17  9:08 ` Xie Yongji
  2021-05-17  9:08 ` [RFC PATCH 12/17] virtio_balloon: " Xie Yongji
                   ` (6 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Xie Yongji @ 2021-05-17  9:08 UTC (permalink / raw)
  To: mst, jasowang, stefanha
  Cc: amit, arei.gonglei, airlied, kraxel, dan.j.williams, johannes,
	ohad, bjorn.andersson, david, vgoyal, miklos, sgarzare,
	virtualization, linux-kernel

The used length is not used. Let's drop it and
pass NULL to virtqueue_get_buf() instead.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 drivers/scsi/virtio_scsi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index b9c86a7e3b97..0ebf23ec844b 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -173,14 +173,13 @@ static void virtscsi_vq_done(struct virtio_scsi *vscsi,
 			     void (*fn)(struct virtio_scsi *vscsi, void *buf))
 {
 	void *buf;
-	unsigned int len;
 	unsigned long flags;
 	struct virtqueue *vq = virtscsi_vq->vq;
 
 	spin_lock_irqsave(&virtscsi_vq->vq_lock, flags);
 	do {
 		virtqueue_disable_cb(vq);
-		while ((buf = virtqueue_get_buf(vq, &len)) != NULL)
+		while ((buf = virtqueue_get_buf(vq, NULL)) != NULL)
 			fn(vscsi, buf);
 
 		if (unlikely(virtqueue_is_broken(vq)))
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 12/17] virtio_balloon: Remove unused used length
  2021-05-17  9:08 [RFC PATCH 00/17] Add validation for used length Xie Yongji
                   ` (10 preceding siblings ...)
  2021-05-17  9:08 ` [RFC PATCH 11/17] virtio_scsi: " Xie Yongji
@ 2021-05-17  9:08 ` Xie Yongji
  2021-05-17  9:08 ` [RFC PATCH 13/17] virtio_input: " Xie Yongji
                   ` (5 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Xie Yongji @ 2021-05-17  9:08 UTC (permalink / raw)
  To: mst, jasowang, stefanha
  Cc: amit, arei.gonglei, airlied, kraxel, dan.j.williams, johannes,
	ohad, bjorn.andersson, david, vgoyal, miklos, sgarzare,
	virtualization, linux-kernel

The used length is not used. Let's drop it and
pass NULL to virtqueue_get_buf() instead.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 drivers/virtio/virtio_balloon.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 510e9318854d..7565970c3901 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -152,7 +152,6 @@ static void balloon_ack(struct virtqueue *vq)
 static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
 {
 	struct scatterlist sg;
-	unsigned int len;
 
 	sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
 
@@ -161,7 +160,7 @@ static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
 	virtqueue_kick(vq);
 
 	/* When host has read buffer, this completes via balloon_ack */
-	wait_event(vb->acked, virtqueue_get_buf(vq, &len));
+	wait_event(vb->acked, virtqueue_get_buf(vq, NULL));
 
 }
 
@@ -171,7 +170,7 @@ static int virtballoon_free_page_report(struct page_reporting_dev_info *pr_dev_i
 	struct virtio_balloon *vb =
 		container_of(pr_dev_info, struct virtio_balloon, pr_dev_info);
 	struct virtqueue *vq = vb->reporting_vq;
-	unsigned int unused, err;
+	unsigned int err;
 
 	/* We should always be able to add these buffers to an empty queue. */
 	err = virtqueue_add_inbuf(vq, sg, nents, vb, GFP_NOWAIT | __GFP_NOWARN);
@@ -187,7 +186,7 @@ static int virtballoon_free_page_report(struct page_reporting_dev_info *pr_dev_i
 	virtqueue_kick(vq);
 
 	/* When host has read buffer, this completes via balloon_ack */
-	wait_event(vb->acked, virtqueue_get_buf(vq, &unused));
+	wait_event(vb->acked, virtqueue_get_buf(vq, NULL));
 
 	return 0;
 }
@@ -386,7 +385,7 @@ static void stats_handle_request(struct virtio_balloon *vb)
 	num_stats = update_balloon_stats(vb);
 
 	vq = vb->stats_vq;
-	if (!virtqueue_get_buf(vq, &len))
+	if (!virtqueue_get_buf(vq, NULL))
 		return;
 	sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats);
 	virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
@@ -586,10 +585,10 @@ static int send_cmd_id_start(struct virtio_balloon *vb)
 {
 	struct scatterlist sg;
 	struct virtqueue *vq = vb->free_page_vq;
-	int err, unused;
+	int err;
 
 	/* Detach all the used buffers from the vq */
-	while (virtqueue_get_buf(vq, &unused))
+	while (virtqueue_get_buf(vq, NULL))
 		;
 
 	vb->cmd_id_active = cpu_to_virtio32(vb->vdev,
@@ -605,10 +604,10 @@ static int send_cmd_id_stop(struct virtio_balloon *vb)
 {
 	struct scatterlist sg;
 	struct virtqueue *vq = vb->free_page_vq;
-	int err, unused;
+	int err;
 
 	/* Detach all the used buffers from the vq */
-	while (virtqueue_get_buf(vq, &unused))
+	while (virtqueue_get_buf(vq, NULL))
 		;
 
 	sg_init_one(&sg, &vb->cmd_id_stop, sizeof(vb->cmd_id_stop));
@@ -623,11 +622,11 @@ static int get_free_page_and_send(struct virtio_balloon *vb)
 	struct virtqueue *vq = vb->free_page_vq;
 	struct page *page;
 	struct scatterlist sg;
-	int err, unused;
+	int err;
 	void *p;
 
 	/* Detach all the used buffers from the vq */
-	while (virtqueue_get_buf(vq, &unused))
+	while (virtqueue_get_buf(vq, NULL))
 		;
 
 	page = alloc_pages(VIRTIO_BALLOON_FREE_PAGE_ALLOC_FLAG,
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 13/17] virtio_input: Remove unused used length
  2021-05-17  9:08 [RFC PATCH 00/17] Add validation for used length Xie Yongji
                   ` (11 preceding siblings ...)
  2021-05-17  9:08 ` [RFC PATCH 12/17] virtio_balloon: " Xie Yongji
@ 2021-05-17  9:08 ` Xie Yongji
  2021-05-17  9:08 ` [RFC PATCH 14/17] virtio_mem: " Xie Yongji
                   ` (4 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Xie Yongji @ 2021-05-17  9:08 UTC (permalink / raw)
  To: mst, jasowang, stefanha
  Cc: amit, arei.gonglei, airlied, kraxel, dan.j.williams, johannes,
	ohad, bjorn.andersson, david, vgoyal, miklos, sgarzare,
	virtualization, linux-kernel

The used length is not used. Let's drop it and
pass NULL to virtqueue_get_buf() instead.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 drivers/virtio/virtio_input.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
index ce51ae165943..f83f8e556fba 100644
--- a/drivers/virtio/virtio_input.c
+++ b/drivers/virtio/virtio_input.c
@@ -35,11 +35,10 @@ static void virtinput_recv_events(struct virtqueue *vq)
 	struct virtio_input *vi = vq->vdev->priv;
 	struct virtio_input_event *event;
 	unsigned long flags;
-	unsigned int len;
 
 	spin_lock_irqsave(&vi->lock, flags);
 	if (vi->ready) {
-		while ((event = virtqueue_get_buf(vi->evt, &len)) != NULL) {
+		while ((event = virtqueue_get_buf(vi->evt, NULL)) != NULL) {
 			spin_unlock_irqrestore(&vi->lock, flags);
 			input_event(vi->idev,
 				    le16_to_cpu(event->type),
@@ -108,10 +107,9 @@ static void virtinput_recv_status(struct virtqueue *vq)
 	struct virtio_input *vi = vq->vdev->priv;
 	struct virtio_input_event *stsbuf;
 	unsigned long flags;
-	unsigned int len;
 
 	spin_lock_irqsave(&vi->lock, flags);
-	while ((stsbuf = virtqueue_get_buf(vi->sts, &len)) != NULL)
+	while ((stsbuf = virtqueue_get_buf(vi->sts, NULL)) != NULL)
 		kfree(stsbuf);
 	spin_unlock_irqrestore(&vi->lock, flags);
 }
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 14/17] virtio_mem: Remove unused used length
  2021-05-17  9:08 [RFC PATCH 00/17] Add validation for used length Xie Yongji
                   ` (12 preceding siblings ...)
  2021-05-17  9:08 ` [RFC PATCH 13/17] virtio_input: " Xie Yongji
@ 2021-05-17  9:08 ` Xie Yongji
  2021-05-17  9:08 ` [RFC PATCH 15/17] virtiofs: " Xie Yongji
                   ` (3 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Xie Yongji @ 2021-05-17  9:08 UTC (permalink / raw)
  To: mst, jasowang, stefanha
  Cc: amit, arei.gonglei, airlied, kraxel, dan.j.williams, johannes,
	ohad, bjorn.andersson, david, vgoyal, miklos, sgarzare,
	virtualization, linux-kernel

The used length is not used. Let's drop it and
pass NULL to virtqueue_get_buf() instead.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 drivers/virtio/virtio_mem.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
index 10ec60d81e84..32a8e359a5c3 100644
--- a/drivers/virtio/virtio_mem.c
+++ b/drivers/virtio/virtio_mem.c
@@ -1256,7 +1256,6 @@ static uint64_t virtio_mem_send_request(struct virtio_mem *vm,
 					const struct virtio_mem_req *req)
 {
 	struct scatterlist *sgs[2], sg_req, sg_resp;
-	unsigned int len;
 	int rc;
 
 	/* don't use the request residing on the stack (vaddr) */
@@ -1277,7 +1276,7 @@ static uint64_t virtio_mem_send_request(struct virtio_mem *vm,
 	virtqueue_kick(vm->vq);
 
 	/* wait for a response */
-	wait_event(vm->host_resp, virtqueue_get_buf(vm->vq, &len));
+	wait_event(vm->host_resp, virtqueue_get_buf(vm->vq, NULL));
 
 	return virtio16_to_cpu(vm->vdev, vm->resp.type);
 }
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 15/17] virtiofs: Remove unused used length
  2021-05-17  9:08 [RFC PATCH 00/17] Add validation for used length Xie Yongji
                   ` (13 preceding siblings ...)
  2021-05-17  9:08 ` [RFC PATCH 14/17] virtio_mem: " Xie Yongji
@ 2021-05-17  9:08 ` Xie Yongji
  2021-05-17  9:08 ` [RFC PATCH 16/17] vsock: " Xie Yongji
                   ` (2 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Xie Yongji @ 2021-05-17  9:08 UTC (permalink / raw)
  To: mst, jasowang, stefanha
  Cc: amit, arei.gonglei, airlied, kraxel, dan.j.williams, johannes,
	ohad, bjorn.andersson, david, vgoyal, miklos, sgarzare,
	virtualization, linux-kernel

The used length is not used. Let's drop it and
pass NULL to virtqueue_get_buf() instead.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 fs/fuse/virtio_fs.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 4ee6f734ba83..e61c94eaa20f 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -322,12 +322,11 @@ static void virtio_fs_hiprio_done_work(struct work_struct *work)
 	/* Free completed FUSE_FORGET requests */
 	spin_lock(&fsvq->lock);
 	do {
-		unsigned int len;
 		void *req;
 
 		virtqueue_disable_cb(vq);
 
-		while ((req = virtqueue_get_buf(vq, &len)) != NULL) {
+		while ((req = virtqueue_get_buf(vq, NULL)) != NULL) {
 			kfree(req);
 			dec_in_flight_req(fsvq);
 		}
@@ -600,7 +599,6 @@ static void virtio_fs_requests_done_work(struct work_struct *work)
 	struct virtqueue *vq = fsvq->vq;
 	struct fuse_req *req;
 	struct fuse_req *next;
-	unsigned int len;
 	LIST_HEAD(reqs);
 
 	/* Collect completed requests off the virtqueue */
@@ -608,7 +606,7 @@ static void virtio_fs_requests_done_work(struct work_struct *work)
 	do {
 		virtqueue_disable_cb(vq);
 
-		while ((req = virtqueue_get_buf(vq, &len)) != NULL) {
+		while ((req = virtqueue_get_buf(vq, NULL)) != NULL) {
 			spin_lock(&fpq->lock);
 			list_move_tail(&req->list, &reqs);
 			spin_unlock(&fpq->lock);
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 16/17] vsock: Remove unused used length
  2021-05-17  9:08 [RFC PATCH 00/17] Add validation for used length Xie Yongji
                   ` (14 preceding siblings ...)
  2021-05-17  9:08 ` [RFC PATCH 15/17] virtiofs: " Xie Yongji
@ 2021-05-17  9:08 ` Xie Yongji
  2021-05-17  9:08 ` [RFC PATCH 17/17] virtio_ring: Add validation for " Xie Yongji
  2021-05-17 23:40   ` Michael S. Tsirkin
  17 siblings, 0 replies; 32+ messages in thread
From: Xie Yongji @ 2021-05-17  9:08 UTC (permalink / raw)
  To: mst, jasowang, stefanha
  Cc: amit, arei.gonglei, airlied, kraxel, dan.j.williams, johannes,
	ohad, bjorn.andersson, david, vgoyal, miklos, sgarzare,
	virtualization, linux-kernel

The used length is not used in some cases. Let's drop it
and pass NULL to virtqueue_get_buf() instead.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 net/vmw_vsock/virtio_transport.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
index 2700a63ab095..f0fc432c8697 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -298,10 +298,9 @@ static void virtio_transport_tx_work(struct work_struct *work)
 
 	do {
 		struct virtio_vsock_pkt *pkt;
-		unsigned int len;
 
 		virtqueue_disable_cb(vq);
-		while ((pkt = virtqueue_get_buf(vq, &len)) != NULL) {
+		while ((pkt = virtqueue_get_buf(vq, NULL)) != NULL) {
 			virtio_transport_free_pkt(pkt);
 			added = true;
 		}
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 17/17] virtio_ring: Add validation for used length
  2021-05-17  9:08 [RFC PATCH 00/17] Add validation for used length Xie Yongji
                   ` (15 preceding siblings ...)
  2021-05-17  9:08 ` [RFC PATCH 16/17] vsock: " Xie Yongji
@ 2021-05-17  9:08 ` Xie Yongji
  2021-05-17 23:39     ` Michael S. Tsirkin
  2021-05-17 23:40   ` Michael S. Tsirkin
  17 siblings, 1 reply; 32+ messages in thread
From: Xie Yongji @ 2021-05-17  9:08 UTC (permalink / raw)
  To: mst, jasowang, stefanha
  Cc: amit, arei.gonglei, airlied, kraxel, dan.j.williams, johannes,
	ohad, bjorn.andersson, david, vgoyal, miklos, sgarzare,
	virtualization, linux-kernel

This adds validation for used length (might come
from an untrusted device) when it will be used by
virtio device driver.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 drivers/virtio/virtio_ring.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index d999a1d6d271..7d4845d06f21 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -68,11 +68,13 @@
 struct vring_desc_state_split {
 	void *data;			/* Data for callback. */
 	struct vring_desc *indir_desc;	/* Indirect descriptor, if any. */
+	u32 in_len;			/* Total length of writable buffer */
 };
 
 struct vring_desc_state_packed {
 	void *data;			/* Data for callback. */
 	struct vring_packed_desc *indir_desc; /* Indirect descriptor, if any. */
+	u32 in_len;			/* Total length of writable buffer */
 	u16 num;			/* Descriptor list length. */
 	u16 last;			/* The last desc state in a list. */
 };
@@ -486,7 +488,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 	struct vring_virtqueue *vq = to_vvq(_vq);
 	struct scatterlist *sg;
 	struct vring_desc *desc;
-	unsigned int i, n, avail, descs_used, prev, err_idx;
+	unsigned int i, n, avail, descs_used, prev, err_idx, in_len = 0;
 	int head;
 	bool indirect;
 
@@ -570,6 +572,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 						     VRING_DESC_F_NEXT |
 						     VRING_DESC_F_WRITE,
 						     indirect);
+			in_len += sg->length;
 		}
 	}
 	/* Last one doesn't continue. */
@@ -604,6 +607,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 
 	/* Store token and indirect buffer state. */
 	vq->split.desc_state[head].data = data;
+	vq->split.desc_state[head].in_len = in_len;
 	if (indirect)
 		vq->split.desc_state[head].indir_desc = desc;
 	else
@@ -784,6 +788,10 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
 		BAD_RING(vq, "id %u is not a head!\n", i);
 		return NULL;
 	}
+	if (unlikely(len && vq->split.desc_state[i].in_len < *len)) {
+		BAD_RING(vq, "id %u has invalid length: %u!\n", i, *len);
+		return NULL;
+	}
 
 	/* detach_buf_split clears data, so grab it now. */
 	ret = vq->split.desc_state[i].data;
@@ -1059,7 +1067,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
 {
 	struct vring_packed_desc *desc;
 	struct scatterlist *sg;
-	unsigned int i, n, err_idx;
+	unsigned int i, n, err_idx, in_len = 0;
 	u16 head, id;
 	dma_addr_t addr;
 
@@ -1084,6 +1092,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
 			if (vring_mapping_error(vq, addr))
 				goto unmap_release;
 
+			in_len += (n < out_sgs) ? 0 : sg->length;
 			desc[i].flags = cpu_to_le16(n < out_sgs ?
 						0 : VRING_DESC_F_WRITE);
 			desc[i].addr = cpu_to_le64(addr);
@@ -1141,6 +1150,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
 	vq->packed.desc_state[id].data = data;
 	vq->packed.desc_state[id].indir_desc = desc;
 	vq->packed.desc_state[id].last = id;
+	vq->packed.desc_state[id].in_len = in_len;
 
 	vq->num_added += 1;
 
@@ -1173,7 +1183,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
 	struct vring_virtqueue *vq = to_vvq(_vq);
 	struct vring_packed_desc *desc;
 	struct scatterlist *sg;
-	unsigned int i, n, c, descs_used, err_idx;
+	unsigned int i, n, c, descs_used, err_idx, in_len = 0;
 	__le16 head_flags, flags;
 	u16 head, id, prev, curr, avail_used_flags;
 
@@ -1223,6 +1233,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
 			if (vring_mapping_error(vq, addr))
 				goto unmap_release;
 
+			in_len += (n < out_sgs) ? 0 : sg->length;
 			flags = cpu_to_le16(vq->packed.avail_used_flags |
 				    (++c == total_sg ? 0 : VRING_DESC_F_NEXT) |
 				    (n < out_sgs ? 0 : VRING_DESC_F_WRITE));
@@ -1268,6 +1279,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
 	vq->packed.desc_state[id].data = data;
 	vq->packed.desc_state[id].indir_desc = ctx;
 	vq->packed.desc_state[id].last = prev;
+	vq->packed.desc_state[id].in_len = in_len;
 
 	/*
 	 * A driver MUST NOT make the first descriptor in the list
@@ -1456,6 +1468,10 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
 		BAD_RING(vq, "id %u is not a head!\n", id);
 		return NULL;
 	}
+	if (unlikely(len && vq->packed.desc_state[id].in_len < *len)) {
+		BAD_RING(vq, "id %u has invalid length: %u!\n", id, *len);
+		return NULL;
+	}
 
 	/* detach_buf_packed clears data, so grab it now. */
 	ret = vq->packed.desc_state[id].data;
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [RFC PATCH 01/17] virtio_ring: Avoid reading unneeded used length
  2021-05-17  9:08 ` [RFC PATCH 01/17] virtio_ring: Avoid reading unneeded " Xie Yongji
@ 2021-05-17  9:11     ` Johannes Berg
  0 siblings, 0 replies; 32+ messages in thread
From: Johannes Berg @ 2021-05-17  9:11 UTC (permalink / raw)
  To: Xie Yongji, mst, jasowang, stefanha
  Cc: amit, arei.gonglei, airlied, kraxel, dan.j.williams, ohad,
	bjorn.andersson, david, vgoyal, miklos, sgarzare, virtualization,
	linux-kernel

On Mon, 2021-05-17 at 17:08 +0800, Xie Yongji wrote:
> If device driver doesn't need used length, it can pass a NULL
> len in virtqueue_get_buf()/virtqueue_get_buf_ctx().
> 

Well, actually, it can't right now?

You should probably rephrase this, saying something like

   Allow passing NULL to len in ... if the device driver doesn't need
   the length, and don't read it in that case.

or so?

>  Then
> we can avoid reading and validating the len value in used
> ring entries.

Not sure what that "validating" is about, I only see reading?

johannes


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [RFC PATCH 01/17] virtio_ring: Avoid reading unneeded used length
@ 2021-05-17  9:11     ` Johannes Berg
  0 siblings, 0 replies; 32+ messages in thread
From: Johannes Berg @ 2021-05-17  9:11 UTC (permalink / raw)
  To: Xie Yongji, mst, jasowang, stefanha
  Cc: ohad, amit, airlied, linux-kernel, bjorn.andersson, miklos,
	dan.j.williams, virtualization, vgoyal

On Mon, 2021-05-17 at 17:08 +0800, Xie Yongji wrote:
> If device driver doesn't need used length, it can pass a NULL
> len in virtqueue_get_buf()/virtqueue_get_buf_ctx().
> 

Well, actually, it can't right now?

You should probably rephrase this, saying something like

   Allow passing NULL to len in ... if the device driver doesn't need
   the length, and don't read it in that case.

or so?

>  Then
> we can avoid reading and validating the len value in used
> ring entries.

Not sure what that "validating" is about, I only see reading?

johannes

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: Re: [RFC PATCH 01/17] virtio_ring: Avoid reading unneeded used length
  2021-05-17  9:11     ` Johannes Berg
  (?)
@ 2021-05-17  9:41     ` Yongji Xie
  -1 siblings, 0 replies; 32+ messages in thread
From: Yongji Xie @ 2021-05-17  9:41 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Michael S. Tsirkin, Jason Wang, Stefan Hajnoczi, amit,
	arei.gonglei, airlied, kraxel, dan.j.williams, Ohad Ben Cohen,
	bjorn.andersson, david, vgoyal, miklos, Stefano Garzarella,
	virtualization, linux-kernel

On Mon, May 17, 2021 at 5:12 PM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Mon, 2021-05-17 at 17:08 +0800, Xie Yongji wrote:
> > If device driver doesn't need used length, it can pass a NULL
> > len in virtqueue_get_buf()/virtqueue_get_buf_ctx().
> >
>
> Well, actually, it can't right now?
>

Yes.

> You should probably rephrase this, saying something like
>
>    Allow passing NULL to len in ... if the device driver doesn't need
>    the length, and don't read it in that case.
>
> or so?
>

Looks good to me.

> >  Then
> > we can avoid reading and validating the len value in used
> > ring entries.
>
> Not sure what that "validating" is about, I only see reading?
>

The “validating" is actually done in the last patch of this series.
Will remove it.

Thanks,
Yongji

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [RFC PATCH 17/17] virtio_ring: Add validation for used length
  2021-05-17  9:08 ` [RFC PATCH 17/17] virtio_ring: Add validation for " Xie Yongji
@ 2021-05-17 23:39     ` Michael S. Tsirkin
  0 siblings, 0 replies; 32+ messages in thread
From: Michael S. Tsirkin @ 2021-05-17 23:39 UTC (permalink / raw)
  To: Xie Yongji
  Cc: jasowang, stefanha, amit, arei.gonglei, airlied, kraxel,
	dan.j.williams, johannes, ohad, bjorn.andersson, david, vgoyal,
	miklos, sgarzare, virtualization, linux-kernel

On Mon, May 17, 2021 at 05:08:36PM +0800, Xie Yongji wrote:
> This adds validation for used length (might come
> from an untrusted device) when it will be used by
> virtio device driver.
> 
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
>  drivers/virtio/virtio_ring.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index d999a1d6d271..7d4845d06f21 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -68,11 +68,13 @@
>  struct vring_desc_state_split {
>  	void *data;			/* Data for callback. */
>  	struct vring_desc *indir_desc;	/* Indirect descriptor, if any. */
> +	u32 in_len;			/* Total length of writable buffer */
>  };
>  
>  struct vring_desc_state_packed {
>  	void *data;			/* Data for callback. */
>  	struct vring_packed_desc *indir_desc; /* Indirect descriptor, if any. */
> +	u32 in_len;			/* Total length of writable buffer */
>  	u16 num;			/* Descriptor list length. */
>  	u16 last;			/* The last desc state in a list. */
>  };


Hmm for packed it's aligned to 64 bit anyway, so we are not making it
any worse. But for split this pushes struct size up by 1/3 increasing
cache pressure.


> @@ -486,7 +488,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>  	struct vring_virtqueue *vq = to_vvq(_vq);
>  	struct scatterlist *sg;
>  	struct vring_desc *desc;
> -	unsigned int i, n, avail, descs_used, prev, err_idx;
> +	unsigned int i, n, avail, descs_used, prev, err_idx, in_len = 0;
>  	int head;
>  	bool indirect;
>  
> @@ -570,6 +572,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>  						     VRING_DESC_F_NEXT |
>  						     VRING_DESC_F_WRITE,
>  						     indirect);
> +			in_len += sg->length;
>  		}
>  	}
>  	/* Last one doesn't continue. */
> @@ -604,6 +607,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>  
>  	/* Store token and indirect buffer state. */
>  	vq->split.desc_state[head].data = data;
> +	vq->split.desc_state[head].in_len = in_len;
>  	if (indirect)
>  		vq->split.desc_state[head].indir_desc = desc;
>  	else
> @@ -784,6 +788,10 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
>  		BAD_RING(vq, "id %u is not a head!\n", i);
>  		return NULL;
>  	}
> +	if (unlikely(len && vq->split.desc_state[i].in_len < *len)) {
> +		BAD_RING(vq, "id %u has invalid length: %u!\n", i, *len);
> +		return NULL;
> +	}
>  
>  	/* detach_buf_split clears data, so grab it now. */
>  	ret = vq->split.desc_state[i].data;
> @@ -1059,7 +1067,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>  {
>  	struct vring_packed_desc *desc;
>  	struct scatterlist *sg;
> -	unsigned int i, n, err_idx;
> +	unsigned int i, n, err_idx, in_len = 0;
>  	u16 head, id;
>  	dma_addr_t addr;
>  
> @@ -1084,6 +1092,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>  			if (vring_mapping_error(vq, addr))
>  				goto unmap_release;
>  
> +			in_len += (n < out_sgs) ? 0 : sg->length;
>  			desc[i].flags = cpu_to_le16(n < out_sgs ?
>  						0 : VRING_DESC_F_WRITE);
>  			desc[i].addr = cpu_to_le64(addr);
> @@ -1141,6 +1150,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>  	vq->packed.desc_state[id].data = data;
>  	vq->packed.desc_state[id].indir_desc = desc;
>  	vq->packed.desc_state[id].last = id;
> +	vq->packed.desc_state[id].in_len = in_len;
>  
>  	vq->num_added += 1;
>  
> @@ -1173,7 +1183,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>  	struct vring_virtqueue *vq = to_vvq(_vq);
>  	struct vring_packed_desc *desc;
>  	struct scatterlist *sg;
> -	unsigned int i, n, c, descs_used, err_idx;
> +	unsigned int i, n, c, descs_used, err_idx, in_len = 0;
>  	__le16 head_flags, flags;
>  	u16 head, id, prev, curr, avail_used_flags;
>  
> @@ -1223,6 +1233,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>  			if (vring_mapping_error(vq, addr))
>  				goto unmap_release;
>  
> +			in_len += (n < out_sgs) ? 0 : sg->length;
>  			flags = cpu_to_le16(vq->packed.avail_used_flags |
>  				    (++c == total_sg ? 0 : VRING_DESC_F_NEXT) |
>  				    (n < out_sgs ? 0 : VRING_DESC_F_WRITE));
> @@ -1268,6 +1279,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>  	vq->packed.desc_state[id].data = data;
>  	vq->packed.desc_state[id].indir_desc = ctx;
>  	vq->packed.desc_state[id].last = prev;
> +	vq->packed.desc_state[id].in_len = in_len;
>  
>  	/*
>  	 * A driver MUST NOT make the first descriptor in the list
> @@ -1456,6 +1468,10 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
>  		BAD_RING(vq, "id %u is not a head!\n", id);
>  		return NULL;
>  	}
> +	if (unlikely(len && vq->packed.desc_state[id].in_len < *len)) {
> +		BAD_RING(vq, "id %u has invalid length: %u!\n", id, *len);
> +		return NULL;
> +	}
>  
>  	/* detach_buf_packed clears data, so grab it now. */
>  	ret = vq->packed.desc_state[id].data;
> -- 
> 2.11.0


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [RFC PATCH 17/17] virtio_ring: Add validation for used length
@ 2021-05-17 23:39     ` Michael S. Tsirkin
  0 siblings, 0 replies; 32+ messages in thread
From: Michael S. Tsirkin @ 2021-05-17 23:39 UTC (permalink / raw)
  To: Xie Yongji
  Cc: ohad, amit, airlied, linux-kernel, bjorn.andersson, miklos,
	stefanha, dan.j.williams, virtualization, johannes, vgoyal

On Mon, May 17, 2021 at 05:08:36PM +0800, Xie Yongji wrote:
> This adds validation for used length (might come
> from an untrusted device) when it will be used by
> virtio device driver.
> 
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
>  drivers/virtio/virtio_ring.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index d999a1d6d271..7d4845d06f21 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -68,11 +68,13 @@
>  struct vring_desc_state_split {
>  	void *data;			/* Data for callback. */
>  	struct vring_desc *indir_desc;	/* Indirect descriptor, if any. */
> +	u32 in_len;			/* Total length of writable buffer */
>  };
>  
>  struct vring_desc_state_packed {
>  	void *data;			/* Data for callback. */
>  	struct vring_packed_desc *indir_desc; /* Indirect descriptor, if any. */
> +	u32 in_len;			/* Total length of writable buffer */
>  	u16 num;			/* Descriptor list length. */
>  	u16 last;			/* The last desc state in a list. */
>  };


Hmm for packed it's aligned to 64 bit anyway, so we are not making it
any worse. But for split this pushes struct size up by 1/3 increasing
cache pressure.


> @@ -486,7 +488,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>  	struct vring_virtqueue *vq = to_vvq(_vq);
>  	struct scatterlist *sg;
>  	struct vring_desc *desc;
> -	unsigned int i, n, avail, descs_used, prev, err_idx;
> +	unsigned int i, n, avail, descs_used, prev, err_idx, in_len = 0;
>  	int head;
>  	bool indirect;
>  
> @@ -570,6 +572,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>  						     VRING_DESC_F_NEXT |
>  						     VRING_DESC_F_WRITE,
>  						     indirect);
> +			in_len += sg->length;
>  		}
>  	}
>  	/* Last one doesn't continue. */
> @@ -604,6 +607,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>  
>  	/* Store token and indirect buffer state. */
>  	vq->split.desc_state[head].data = data;
> +	vq->split.desc_state[head].in_len = in_len;
>  	if (indirect)
>  		vq->split.desc_state[head].indir_desc = desc;
>  	else
> @@ -784,6 +788,10 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
>  		BAD_RING(vq, "id %u is not a head!\n", i);
>  		return NULL;
>  	}
> +	if (unlikely(len && vq->split.desc_state[i].in_len < *len)) {
> +		BAD_RING(vq, "id %u has invalid length: %u!\n", i, *len);
> +		return NULL;
> +	}
>  
>  	/* detach_buf_split clears data, so grab it now. */
>  	ret = vq->split.desc_state[i].data;
> @@ -1059,7 +1067,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>  {
>  	struct vring_packed_desc *desc;
>  	struct scatterlist *sg;
> -	unsigned int i, n, err_idx;
> +	unsigned int i, n, err_idx, in_len = 0;
>  	u16 head, id;
>  	dma_addr_t addr;
>  
> @@ -1084,6 +1092,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>  			if (vring_mapping_error(vq, addr))
>  				goto unmap_release;
>  
> +			in_len += (n < out_sgs) ? 0 : sg->length;
>  			desc[i].flags = cpu_to_le16(n < out_sgs ?
>  						0 : VRING_DESC_F_WRITE);
>  			desc[i].addr = cpu_to_le64(addr);
> @@ -1141,6 +1150,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>  	vq->packed.desc_state[id].data = data;
>  	vq->packed.desc_state[id].indir_desc = desc;
>  	vq->packed.desc_state[id].last = id;
> +	vq->packed.desc_state[id].in_len = in_len;
>  
>  	vq->num_added += 1;
>  
> @@ -1173,7 +1183,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>  	struct vring_virtqueue *vq = to_vvq(_vq);
>  	struct vring_packed_desc *desc;
>  	struct scatterlist *sg;
> -	unsigned int i, n, c, descs_used, err_idx;
> +	unsigned int i, n, c, descs_used, err_idx, in_len = 0;
>  	__le16 head_flags, flags;
>  	u16 head, id, prev, curr, avail_used_flags;
>  
> @@ -1223,6 +1233,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>  			if (vring_mapping_error(vq, addr))
>  				goto unmap_release;
>  
> +			in_len += (n < out_sgs) ? 0 : sg->length;
>  			flags = cpu_to_le16(vq->packed.avail_used_flags |
>  				    (++c == total_sg ? 0 : VRING_DESC_F_NEXT) |
>  				    (n < out_sgs ? 0 : VRING_DESC_F_WRITE));
> @@ -1268,6 +1279,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>  	vq->packed.desc_state[id].data = data;
>  	vq->packed.desc_state[id].indir_desc = ctx;
>  	vq->packed.desc_state[id].last = prev;
> +	vq->packed.desc_state[id].in_len = in_len;
>  
>  	/*
>  	 * A driver MUST NOT make the first descriptor in the list
> @@ -1456,6 +1468,10 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
>  		BAD_RING(vq, "id %u is not a head!\n", id);
>  		return NULL;
>  	}
> +	if (unlikely(len && vq->packed.desc_state[id].in_len < *len)) {
> +		BAD_RING(vq, "id %u has invalid length: %u!\n", id, *len);
> +		return NULL;
> +	}
>  
>  	/* detach_buf_packed clears data, so grab it now. */
>  	ret = vq->packed.desc_state[id].data;
> -- 
> 2.11.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [RFC PATCH 00/17] Add validation for used length
  2021-05-17  9:08 [RFC PATCH 00/17] Add validation for used length Xie Yongji
@ 2021-05-17 23:40   ` Michael S. Tsirkin
  2021-05-17  9:08 ` [RFC PATCH 02/17] virtio-blk: Remove unused " Xie Yongji
                     ` (16 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Michael S. Tsirkin @ 2021-05-17 23:40 UTC (permalink / raw)
  To: Xie Yongji
  Cc: jasowang, stefanha, amit, arei.gonglei, airlied, kraxel,
	dan.j.williams, johannes, ohad, bjorn.andersson, david, vgoyal,
	miklos, sgarzare, virtualization, linux-kernel

On Mon, May 17, 2021 at 05:08:19PM +0800, Xie Yongji wrote:
> Current virtio device drivers may trust the used length returned
> in virtqueue_get_buf()/virtqueue_get_buf_ctx(). But the used length
> might come from an untrusted device when VDUSE[1] is enabled. To
> protect this case, this series tries to add validation for the
> used length.
> 
> Since many legacy devices will also set the used length incorrectly,
> we did not add the validation unconditionally. Instead, we will do
> the validation only when the device driver needs the used length.
> A NULL len passed to virtqueue_get_buf()/virtqueue_get_buf_ctx()
> will mean the used length is not needed by the device driver.

Can we be more specific? Which drivers have problems when used len
is incorrect? Maybe there's an easier way like validating the length
in the driver ...

> [1] https://lore.kernel.org/kvm/20210331080519.172-1-xieyongji@bytedance.com/
> 
> Xie Yongji (17):
>   virtio_ring: Avoid reading unneeded used length
>   virtio-blk: Remove unused used length
>   virtio_console: Remove unused used length
>   crypto: virtio - Remove unused used length
>   drm/virtio: Remove unused used length
>   caif_virtio: Remove unused used length
>   virtio_net: Remove unused used length
>   mac80211_hwsim: Remove unused used length
>   virtio_pmem: Remove unused used length
>   rpmsg: virtio: Remove unused used length
>   virtio_scsi: Remove unused used length
>   virtio_balloon: Remove unused used length
>   virtio_input: Remove unused used length
>   virtio_mem: Remove unused used length
>   virtiofs: Remove unused used length
>   vsock: Remove unused used length
>   virtio_ring: Add validation for used length
> 
>  drivers/block/virtio_blk.c                 |  3 +--
>  drivers/char/virtio_console.c              | 12 ++++--------
>  drivers/crypto/virtio/virtio_crypto_algs.c |  6 ++----
>  drivers/gpu/drm/virtio/virtgpu_vq.c        |  3 +--
>  drivers/net/caif/caif_virtio.c             |  3 +--
>  drivers/net/virtio_net.c                   | 10 ++++------
>  drivers/net/wireless/mac80211_hwsim.c      |  3 +--
>  drivers/nvdimm/nd_virtio.c                 |  3 +--
>  drivers/rpmsg/virtio_rpmsg_bus.c           |  3 +--
>  drivers/scsi/virtio_scsi.c                 |  3 +--
>  drivers/virtio/virtio_balloon.c            | 21 ++++++++++-----------
>  drivers/virtio/virtio_input.c              |  6 ++----
>  drivers/virtio/virtio_mem.c                |  3 +--
>  drivers/virtio/virtio_ring.c               | 28 +++++++++++++++++++++++-----
>  fs/fuse/virtio_fs.c                        |  6 ++----
>  net/vmw_vsock/virtio_transport.c           |  3 +--
>  16 files changed, 56 insertions(+), 60 deletions(-)
> 
> -- 
> 2.11.0


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [RFC PATCH 00/17] Add validation for used length
@ 2021-05-17 23:40   ` Michael S. Tsirkin
  0 siblings, 0 replies; 32+ messages in thread
From: Michael S. Tsirkin @ 2021-05-17 23:40 UTC (permalink / raw)
  To: Xie Yongji
  Cc: ohad, amit, airlied, linux-kernel, bjorn.andersson, miklos,
	stefanha, dan.j.williams, virtualization, johannes, vgoyal

On Mon, May 17, 2021 at 05:08:19PM +0800, Xie Yongji wrote:
> Current virtio device drivers may trust the used length returned
> in virtqueue_get_buf()/virtqueue_get_buf_ctx(). But the used length
> might come from an untrusted device when VDUSE[1] is enabled. To
> protect this case, this series tries to add validation for the
> used length.
> 
> Since many legacy devices will also set the used length incorrectly,
> we did not add the validation unconditionally. Instead, we will do
> the validation only when the device driver needs the used length.
> A NULL len passed to virtqueue_get_buf()/virtqueue_get_buf_ctx()
> will mean the used length is not needed by the device driver.

Can we be more specific? Which drivers have problems when used len
is incorrect? Maybe there's an easier way like validating the length
in the driver ...

> [1] https://lore.kernel.org/kvm/20210331080519.172-1-xieyongji@bytedance.com/
> 
> Xie Yongji (17):
>   virtio_ring: Avoid reading unneeded used length
>   virtio-blk: Remove unused used length
>   virtio_console: Remove unused used length
>   crypto: virtio - Remove unused used length
>   drm/virtio: Remove unused used length
>   caif_virtio: Remove unused used length
>   virtio_net: Remove unused used length
>   mac80211_hwsim: Remove unused used length
>   virtio_pmem: Remove unused used length
>   rpmsg: virtio: Remove unused used length
>   virtio_scsi: Remove unused used length
>   virtio_balloon: Remove unused used length
>   virtio_input: Remove unused used length
>   virtio_mem: Remove unused used length
>   virtiofs: Remove unused used length
>   vsock: Remove unused used length
>   virtio_ring: Add validation for used length
> 
>  drivers/block/virtio_blk.c                 |  3 +--
>  drivers/char/virtio_console.c              | 12 ++++--------
>  drivers/crypto/virtio/virtio_crypto_algs.c |  6 ++----
>  drivers/gpu/drm/virtio/virtgpu_vq.c        |  3 +--
>  drivers/net/caif/caif_virtio.c             |  3 +--
>  drivers/net/virtio_net.c                   | 10 ++++------
>  drivers/net/wireless/mac80211_hwsim.c      |  3 +--
>  drivers/nvdimm/nd_virtio.c                 |  3 +--
>  drivers/rpmsg/virtio_rpmsg_bus.c           |  3 +--
>  drivers/scsi/virtio_scsi.c                 |  3 +--
>  drivers/virtio/virtio_balloon.c            | 21 ++++++++++-----------
>  drivers/virtio/virtio_input.c              |  6 ++----
>  drivers/virtio/virtio_mem.c                |  3 +--
>  drivers/virtio/virtio_ring.c               | 28 +++++++++++++++++++++++-----
>  fs/fuse/virtio_fs.c                        |  6 ++----
>  net/vmw_vsock/virtio_transport.c           |  3 +--
>  16 files changed, 56 insertions(+), 60 deletions(-)
> 
> -- 
> 2.11.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: Re: [RFC PATCH 00/17] Add validation for used length
  2021-05-17 23:40   ` Michael S. Tsirkin
  (?)
@ 2021-05-18  8:29   ` Yongji Xie
  2021-05-18  9:52       ` Michael S. Tsirkin
  -1 siblings, 1 reply; 32+ messages in thread
From: Yongji Xie @ 2021-05-18  8:29 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Jason Wang, Stefan Hajnoczi, amit, arei.gonglei, airlied, kraxel,
	dan.j.williams, Johannes Berg, Ohad Ben Cohen, bjorn.andersson,
	David Hildenbrand, vgoyal, miklos, Stefano Garzarella,
	virtualization, linux-kernel

On Tue, May 18, 2021 at 7:40 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, May 17, 2021 at 05:08:19PM +0800, Xie Yongji wrote:
> > Current virtio device drivers may trust the used length returned
> > in virtqueue_get_buf()/virtqueue_get_buf_ctx(). But the used length
> > might come from an untrusted device when VDUSE[1] is enabled. To
> > protect this case, this series tries to add validation for the
> > used length.
> >
> > Since many legacy devices will also set the used length incorrectly,
> > we did not add the validation unconditionally. Instead, we will do
> > the validation only when the device driver needs the used length.
> > A NULL len passed to virtqueue_get_buf()/virtqueue_get_buf_ctx()
> > will mean the used length is not needed by the device driver.
>
> Can we be more specific? Which drivers have problems when used len
> is incorrect? Maybe there's an easier way like validating the length
> in the driver ...
>

It's ok to me. But this means all future new drivers need to remember
to do the validation.

Now only virtio-net and virtio-console drivers have this problem. I
can send some patches to fix it.

Thanks,
Yongji

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: Re: [RFC PATCH 00/17] Add validation for used length
  2021-05-18  8:29   ` Yongji Xie
@ 2021-05-18  9:52       ` Michael S. Tsirkin
  0 siblings, 0 replies; 32+ messages in thread
From: Michael S. Tsirkin @ 2021-05-18  9:52 UTC (permalink / raw)
  To: Yongji Xie
  Cc: Jason Wang, Stefan Hajnoczi, amit, arei.gonglei, airlied, kraxel,
	dan.j.williams, Johannes Berg, Ohad Ben Cohen, bjorn.andersson,
	David Hildenbrand, vgoyal, miklos, Stefano Garzarella,
	virtualization, linux-kernel

On Tue, May 18, 2021 at 04:29:44PM +0800, Yongji Xie wrote:
> On Tue, May 18, 2021 at 7:40 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, May 17, 2021 at 05:08:19PM +0800, Xie Yongji wrote:
> > > Current virtio device drivers may trust the used length returned
> > > in virtqueue_get_buf()/virtqueue_get_buf_ctx(). But the used length
> > > might come from an untrusted device when VDUSE[1] is enabled. To
> > > protect this case, this series tries to add validation for the
> > > used length.
> > >
> > > Since many legacy devices will also set the used length incorrectly,
> > > we did not add the validation unconditionally. Instead, we will do
> > > the validation only when the device driver needs the used length.
> > > A NULL len passed to virtqueue_get_buf()/virtqueue_get_buf_ctx()
> > > will mean the used length is not needed by the device driver.
> >
> > Can we be more specific? Which drivers have problems when used len
> > is incorrect? Maybe there's an easier way like validating the length
> > in the driver ...
> >
> 
> It's ok to me. But this means all future new drivers need to remember
> to do the validation.
> 
> Now only virtio-net and virtio-console drivers have this problem. I
> can send some patches to fix it.
> 
> Thanks,
> Yongji

I'd say let's just document the requirement for now.

-- 
MST


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: Re: [RFC PATCH 00/17] Add validation for used length
@ 2021-05-18  9:52       ` Michael S. Tsirkin
  0 siblings, 0 replies; 32+ messages in thread
From: Michael S. Tsirkin @ 2021-05-18  9:52 UTC (permalink / raw)
  To: Yongji Xie
  Cc: Ohad Ben Cohen, amit, airlied, linux-kernel, bjorn.andersson,
	miklos, Stefan Hajnoczi, dan.j.williams, virtualization,
	Johannes Berg, vgoyal

On Tue, May 18, 2021 at 04:29:44PM +0800, Yongji Xie wrote:
> On Tue, May 18, 2021 at 7:40 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, May 17, 2021 at 05:08:19PM +0800, Xie Yongji wrote:
> > > Current virtio device drivers may trust the used length returned
> > > in virtqueue_get_buf()/virtqueue_get_buf_ctx(). But the used length
> > > might come from an untrusted device when VDUSE[1] is enabled. To
> > > protect this case, this series tries to add validation for the
> > > used length.
> > >
> > > Since many legacy devices will also set the used length incorrectly,
> > > we did not add the validation unconditionally. Instead, we will do
> > > the validation only when the device driver needs the used length.
> > > A NULL len passed to virtqueue_get_buf()/virtqueue_get_buf_ctx()
> > > will mean the used length is not needed by the device driver.
> >
> > Can we be more specific? Which drivers have problems when used len
> > is incorrect? Maybe there's an easier way like validating the length
> > in the driver ...
> >
> 
> It's ok to me. But this means all future new drivers need to remember
> to do the validation.
> 
> Now only virtio-net and virtio-console drivers have this problem. I
> can send some patches to fix it.
> 
> Thanks,
> Yongji

I'd say let's just document the requirement for now.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: Re: Re: [RFC PATCH 00/17] Add validation for used length
  2021-05-18  9:52       ` Michael S. Tsirkin
  (?)
@ 2021-05-18 12:28       ` Yongji Xie
  -1 siblings, 0 replies; 32+ messages in thread
From: Yongji Xie @ 2021-05-18 12:28 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Jason Wang, Stefan Hajnoczi, amit, arei.gonglei, airlied, kraxel,
	dan.j.williams, Johannes Berg, Ohad Ben Cohen, bjorn.andersson,
	David Hildenbrand, vgoyal, miklos, Stefano Garzarella,
	virtualization, linux-kernel

On Tue, May 18, 2021 at 5:52 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, May 18, 2021 at 04:29:44PM +0800, Yongji Xie wrote:
> > On Tue, May 18, 2021 at 7:40 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, May 17, 2021 at 05:08:19PM +0800, Xie Yongji wrote:
> > > > Current virtio device drivers may trust the used length returned
> > > > in virtqueue_get_buf()/virtqueue_get_buf_ctx(). But the used length
> > > > might come from an untrusted device when VDUSE[1] is enabled. To
> > > > protect this case, this series tries to add validation for the
> > > > used length.
> > > >
> > > > Since many legacy devices will also set the used length incorrectly,
> > > > we did not add the validation unconditionally. Instead, we will do
> > > > the validation only when the device driver needs the used length.
> > > > A NULL len passed to virtqueue_get_buf()/virtqueue_get_buf_ctx()
> > > > will mean the used length is not needed by the device driver.
> > >
> > > Can we be more specific? Which drivers have problems when used len
> > > is incorrect? Maybe there's an easier way like validating the length
> > > in the driver ...
> > >
> >
> > It's ok to me. But this means all future new drivers need to remember
> > to do the validation.
> >
> > Now only virtio-net and virtio-console drivers have this problem. I
> > can send some patches to fix it.
> >
> > Thanks,
> > Yongji
>
> I'd say let's just document the requirement for now.
>

It's fine with me.

Thanks,
Yongji

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [RFC PATCH 17/17] virtio_ring: Add validation for used length
  2021-05-17 23:39     ` Michael S. Tsirkin
@ 2021-05-25  1:31       ` Jason Wang
  -1 siblings, 0 replies; 32+ messages in thread
From: Jason Wang @ 2021-05-25  1:31 UTC (permalink / raw)
  To: Michael S. Tsirkin, Xie Yongji
  Cc: stefanha, amit, arei.gonglei, airlied, kraxel, dan.j.williams,
	johannes, ohad, bjorn.andersson, david, vgoyal, miklos, sgarzare,
	virtualization, linux-kernel


在 2021/5/18 上午7:39, Michael S. Tsirkin 写道:
> On Mon, May 17, 2021 at 05:08:36PM +0800, Xie Yongji wrote:
>> This adds validation for used length (might come
>> from an untrusted device) when it will be used by
>> virtio device driver.
>>
>> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
>> ---
>>   drivers/virtio/virtio_ring.c | 22 +++++++++++++++++++---
>>   1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>> index d999a1d6d271..7d4845d06f21 100644
>> --- a/drivers/virtio/virtio_ring.c
>> +++ b/drivers/virtio/virtio_ring.c
>> @@ -68,11 +68,13 @@
>>   struct vring_desc_state_split {
>>   	void *data;			/* Data for callback. */
>>   	struct vring_desc *indir_desc;	/* Indirect descriptor, if any. */
>> +	u32 in_len;			/* Total length of writable buffer */
>>   };
>>   
>>   struct vring_desc_state_packed {
>>   	void *data;			/* Data for callback. */
>>   	struct vring_packed_desc *indir_desc; /* Indirect descriptor, if any. */
>> +	u32 in_len;			/* Total length of writable buffer */
>>   	u16 num;			/* Descriptor list length. */
>>   	u16 last;			/* The last desc state in a list. */
>>   };
>
> Hmm for packed it's aligned to 64 bit anyway, so we are not making it
> any worse. But for split this pushes struct size up by 1/3 increasing
> cache pressure.


We can eliminate this by validating through virtio device driver instead 
of virtio core.

E.g for virtio-net we know the rx buffer size so there's no need to 
store in twice in the core.

Thanks


>
>
>> @@ -486,7 +488,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>>   	struct vring_virtqueue *vq = to_vvq(_vq);
>>   	struct scatterlist *sg;
>>   	struct vring_desc *desc;
>> -	unsigned int i, n, avail, descs_used, prev, err_idx;
>> +	unsigned int i, n, avail, descs_used, prev, err_idx, in_len = 0;
>>   	int head;
>>   	bool indirect;
>>   
>> @@ -570,6 +572,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>>   						     VRING_DESC_F_NEXT |
>>   						     VRING_DESC_F_WRITE,
>>   						     indirect);
>> +			in_len += sg->length;
>>   		}
>>   	}
>>   	/* Last one doesn't continue. */
>> @@ -604,6 +607,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>>   
>>   	/* Store token and indirect buffer state. */
>>   	vq->split.desc_state[head].data = data;
>> +	vq->split.desc_state[head].in_len = in_len;
>>   	if (indirect)
>>   		vq->split.desc_state[head].indir_desc = desc;
>>   	else
>> @@ -784,6 +788,10 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
>>   		BAD_RING(vq, "id %u is not a head!\n", i);
>>   		return NULL;
>>   	}
>> +	if (unlikely(len && vq->split.desc_state[i].in_len < *len)) {
>> +		BAD_RING(vq, "id %u has invalid length: %u!\n", i, *len);
>> +		return NULL;
>> +	}
>>   
>>   	/* detach_buf_split clears data, so grab it now. */
>>   	ret = vq->split.desc_state[i].data;
>> @@ -1059,7 +1067,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>>   {
>>   	struct vring_packed_desc *desc;
>>   	struct scatterlist *sg;
>> -	unsigned int i, n, err_idx;
>> +	unsigned int i, n, err_idx, in_len = 0;
>>   	u16 head, id;
>>   	dma_addr_t addr;
>>   
>> @@ -1084,6 +1092,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>>   			if (vring_mapping_error(vq, addr))
>>   				goto unmap_release;
>>   
>> +			in_len += (n < out_sgs) ? 0 : sg->length;
>>   			desc[i].flags = cpu_to_le16(n < out_sgs ?
>>   						0 : VRING_DESC_F_WRITE);
>>   			desc[i].addr = cpu_to_le64(addr);
>> @@ -1141,6 +1150,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>>   	vq->packed.desc_state[id].data = data;
>>   	vq->packed.desc_state[id].indir_desc = desc;
>>   	vq->packed.desc_state[id].last = id;
>> +	vq->packed.desc_state[id].in_len = in_len;
>>   
>>   	vq->num_added += 1;
>>   
>> @@ -1173,7 +1183,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>>   	struct vring_virtqueue *vq = to_vvq(_vq);
>>   	struct vring_packed_desc *desc;
>>   	struct scatterlist *sg;
>> -	unsigned int i, n, c, descs_used, err_idx;
>> +	unsigned int i, n, c, descs_used, err_idx, in_len = 0;
>>   	__le16 head_flags, flags;
>>   	u16 head, id, prev, curr, avail_used_flags;
>>   
>> @@ -1223,6 +1233,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>>   			if (vring_mapping_error(vq, addr))
>>   				goto unmap_release;
>>   
>> +			in_len += (n < out_sgs) ? 0 : sg->length;
>>   			flags = cpu_to_le16(vq->packed.avail_used_flags |
>>   				    (++c == total_sg ? 0 : VRING_DESC_F_NEXT) |
>>   				    (n < out_sgs ? 0 : VRING_DESC_F_WRITE));
>> @@ -1268,6 +1279,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>>   	vq->packed.desc_state[id].data = data;
>>   	vq->packed.desc_state[id].indir_desc = ctx;
>>   	vq->packed.desc_state[id].last = prev;
>> +	vq->packed.desc_state[id].in_len = in_len;
>>   
>>   	/*
>>   	 * A driver MUST NOT make the first descriptor in the list
>> @@ -1456,6 +1468,10 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
>>   		BAD_RING(vq, "id %u is not a head!\n", id);
>>   		return NULL;
>>   	}
>> +	if (unlikely(len && vq->packed.desc_state[id].in_len < *len)) {
>> +		BAD_RING(vq, "id %u has invalid length: %u!\n", id, *len);
>> +		return NULL;
>> +	}
>>   
>>   	/* detach_buf_packed clears data, so grab it now. */
>>   	ret = vq->packed.desc_state[id].data;
>> -- 
>> 2.11.0


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [RFC PATCH 17/17] virtio_ring: Add validation for used length
@ 2021-05-25  1:31       ` Jason Wang
  0 siblings, 0 replies; 32+ messages in thread
From: Jason Wang @ 2021-05-25  1:31 UTC (permalink / raw)
  To: Michael S. Tsirkin, Xie Yongji
  Cc: ohad, amit, airlied, linux-kernel, bjorn.andersson, miklos,
	stefanha, dan.j.williams, virtualization, johannes, vgoyal


在 2021/5/18 上午7:39, Michael S. Tsirkin 写道:
> On Mon, May 17, 2021 at 05:08:36PM +0800, Xie Yongji wrote:
>> This adds validation for used length (might come
>> from an untrusted device) when it will be used by
>> virtio device driver.
>>
>> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
>> ---
>>   drivers/virtio/virtio_ring.c | 22 +++++++++++++++++++---
>>   1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>> index d999a1d6d271..7d4845d06f21 100644
>> --- a/drivers/virtio/virtio_ring.c
>> +++ b/drivers/virtio/virtio_ring.c
>> @@ -68,11 +68,13 @@
>>   struct vring_desc_state_split {
>>   	void *data;			/* Data for callback. */
>>   	struct vring_desc *indir_desc;	/* Indirect descriptor, if any. */
>> +	u32 in_len;			/* Total length of writable buffer */
>>   };
>>   
>>   struct vring_desc_state_packed {
>>   	void *data;			/* Data for callback. */
>>   	struct vring_packed_desc *indir_desc; /* Indirect descriptor, if any. */
>> +	u32 in_len;			/* Total length of writable buffer */
>>   	u16 num;			/* Descriptor list length. */
>>   	u16 last;			/* The last desc state in a list. */
>>   };
>
> Hmm for packed it's aligned to 64 bit anyway, so we are not making it
> any worse. But for split this pushes struct size up by 1/3 increasing
> cache pressure.


We can eliminate this by validating through virtio device driver instead 
of virtio core.

E.g for virtio-net we know the rx buffer size so there's no need to 
store in twice in the core.

Thanks


>
>
>> @@ -486,7 +488,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>>   	struct vring_virtqueue *vq = to_vvq(_vq);
>>   	struct scatterlist *sg;
>>   	struct vring_desc *desc;
>> -	unsigned int i, n, avail, descs_used, prev, err_idx;
>> +	unsigned int i, n, avail, descs_used, prev, err_idx, in_len = 0;
>>   	int head;
>>   	bool indirect;
>>   
>> @@ -570,6 +572,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>>   						     VRING_DESC_F_NEXT |
>>   						     VRING_DESC_F_WRITE,
>>   						     indirect);
>> +			in_len += sg->length;
>>   		}
>>   	}
>>   	/* Last one doesn't continue. */
>> @@ -604,6 +607,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>>   
>>   	/* Store token and indirect buffer state. */
>>   	vq->split.desc_state[head].data = data;
>> +	vq->split.desc_state[head].in_len = in_len;
>>   	if (indirect)
>>   		vq->split.desc_state[head].indir_desc = desc;
>>   	else
>> @@ -784,6 +788,10 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
>>   		BAD_RING(vq, "id %u is not a head!\n", i);
>>   		return NULL;
>>   	}
>> +	if (unlikely(len && vq->split.desc_state[i].in_len < *len)) {
>> +		BAD_RING(vq, "id %u has invalid length: %u!\n", i, *len);
>> +		return NULL;
>> +	}
>>   
>>   	/* detach_buf_split clears data, so grab it now. */
>>   	ret = vq->split.desc_state[i].data;
>> @@ -1059,7 +1067,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>>   {
>>   	struct vring_packed_desc *desc;
>>   	struct scatterlist *sg;
>> -	unsigned int i, n, err_idx;
>> +	unsigned int i, n, err_idx, in_len = 0;
>>   	u16 head, id;
>>   	dma_addr_t addr;
>>   
>> @@ -1084,6 +1092,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>>   			if (vring_mapping_error(vq, addr))
>>   				goto unmap_release;
>>   
>> +			in_len += (n < out_sgs) ? 0 : sg->length;
>>   			desc[i].flags = cpu_to_le16(n < out_sgs ?
>>   						0 : VRING_DESC_F_WRITE);
>>   			desc[i].addr = cpu_to_le64(addr);
>> @@ -1141,6 +1150,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>>   	vq->packed.desc_state[id].data = data;
>>   	vq->packed.desc_state[id].indir_desc = desc;
>>   	vq->packed.desc_state[id].last = id;
>> +	vq->packed.desc_state[id].in_len = in_len;
>>   
>>   	vq->num_added += 1;
>>   
>> @@ -1173,7 +1183,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>>   	struct vring_virtqueue *vq = to_vvq(_vq);
>>   	struct vring_packed_desc *desc;
>>   	struct scatterlist *sg;
>> -	unsigned int i, n, c, descs_used, err_idx;
>> +	unsigned int i, n, c, descs_used, err_idx, in_len = 0;
>>   	__le16 head_flags, flags;
>>   	u16 head, id, prev, curr, avail_used_flags;
>>   
>> @@ -1223,6 +1233,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>>   			if (vring_mapping_error(vq, addr))
>>   				goto unmap_release;
>>   
>> +			in_len += (n < out_sgs) ? 0 : sg->length;
>>   			flags = cpu_to_le16(vq->packed.avail_used_flags |
>>   				    (++c == total_sg ? 0 : VRING_DESC_F_NEXT) |
>>   				    (n < out_sgs ? 0 : VRING_DESC_F_WRITE));
>> @@ -1268,6 +1279,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>>   	vq->packed.desc_state[id].data = data;
>>   	vq->packed.desc_state[id].indir_desc = ctx;
>>   	vq->packed.desc_state[id].last = prev;
>> +	vq->packed.desc_state[id].in_len = in_len;
>>   
>>   	/*
>>   	 * A driver MUST NOT make the first descriptor in the list
>> @@ -1456,6 +1468,10 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
>>   		BAD_RING(vq, "id %u is not a head!\n", id);
>>   		return NULL;
>>   	}
>> +	if (unlikely(len && vq->packed.desc_state[id].in_len < *len)) {
>> +		BAD_RING(vq, "id %u has invalid length: %u!\n", id, *len);
>> +		return NULL;
>> +	}
>>   
>>   	/* detach_buf_packed clears data, so grab it now. */
>>   	ret = vq->packed.desc_state[id].data;
>> -- 
>> 2.11.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: Re: [RFC PATCH 17/17] virtio_ring: Add validation for used length
  2021-05-25  1:31       ` Jason Wang
  (?)
@ 2021-05-25  5:08       ` Yongji Xie
  -1 siblings, 0 replies; 32+ messages in thread
From: Yongji Xie @ 2021-05-25  5:08 UTC (permalink / raw)
  To: Jason Wang
  Cc: Michael S. Tsirkin, Stefan Hajnoczi, amit, arei.gonglei, airlied,
	kraxel, dan.j.williams, Johannes Berg, Ohad Ben Cohen,
	bjorn.andersson, David Hildenbrand, vgoyal, miklos,
	Stefano Garzarella, virtualization, linux-kernel

On Tue, May 25, 2021 at 9:31 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2021/5/18 上午7:39, Michael S. Tsirkin 写道:
> > On Mon, May 17, 2021 at 05:08:36PM +0800, Xie Yongji wrote:
> >> This adds validation for used length (might come
> >> from an untrusted device) when it will be used by
> >> virtio device driver.
> >>
> >> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> >> ---
> >>   drivers/virtio/virtio_ring.c | 22 +++++++++++++++++++---
> >>   1 file changed, 19 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> >> index d999a1d6d271..7d4845d06f21 100644
> >> --- a/drivers/virtio/virtio_ring.c
> >> +++ b/drivers/virtio/virtio_ring.c
> >> @@ -68,11 +68,13 @@
> >>   struct vring_desc_state_split {
> >>      void *data;                     /* Data for callback. */
> >>      struct vring_desc *indir_desc;  /* Indirect descriptor, if any. */
> >> +    u32 in_len;                     /* Total length of writable buffer */
> >>   };
> >>
> >>   struct vring_desc_state_packed {
> >>      void *data;                     /* Data for callback. */
> >>      struct vring_packed_desc *indir_desc; /* Indirect descriptor, if any. */
> >> +    u32 in_len;                     /* Total length of writable buffer */
> >>      u16 num;                        /* Descriptor list length. */
> >>      u16 last;                       /* The last desc state in a list. */
> >>   };
> >
> > Hmm for packed it's aligned to 64 bit anyway, so we are not making it
> > any worse. But for split this pushes struct size up by 1/3 increasing
> > cache pressure.
>
>
> We can eliminate this by validating through virtio device driver instead
> of virtio core.
>
> E.g for virtio-net we know the rx buffer size so there's no need to
> store in twice in the core.
>

I see. I have sent the new fix just now.

Thanks,
Yongji

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2021-05-25  5:08 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-17  9:08 [RFC PATCH 00/17] Add validation for used length Xie Yongji
2021-05-17  9:08 ` [RFC PATCH 01/17] virtio_ring: Avoid reading unneeded " Xie Yongji
2021-05-17  9:11   ` Johannes Berg
2021-05-17  9:11     ` Johannes Berg
2021-05-17  9:41     ` Yongji Xie
2021-05-17  9:08 ` [RFC PATCH 02/17] virtio-blk: Remove unused " Xie Yongji
2021-05-17  9:08 ` [RFC PATCH 03/17] virtio_console: " Xie Yongji
2021-05-17  9:08 ` [RFC PATCH 04/17] crypto: virtio - " Xie Yongji
2021-05-17  9:08 ` [RFC PATCH 05/17] drm/virtio: " Xie Yongji
2021-05-17  9:08 ` [RFC PATCH 06/17] caif_virtio: " Xie Yongji
2021-05-17  9:08 ` [RFC PATCH 07/17] virtio_net: " Xie Yongji
2021-05-17  9:08 ` [RFC PATCH 08/17] mac80211_hwsim: " Xie Yongji
2021-05-17  9:08 ` [RFC PATCH 09/17] virtio_pmem: " Xie Yongji
2021-05-17  9:08 ` [RFC PATCH 10/17] rpmsg: virtio: " Xie Yongji
2021-05-17  9:08 ` [RFC PATCH 11/17] virtio_scsi: " Xie Yongji
2021-05-17  9:08 ` [RFC PATCH 12/17] virtio_balloon: " Xie Yongji
2021-05-17  9:08 ` [RFC PATCH 13/17] virtio_input: " Xie Yongji
2021-05-17  9:08 ` [RFC PATCH 14/17] virtio_mem: " Xie Yongji
2021-05-17  9:08 ` [RFC PATCH 15/17] virtiofs: " Xie Yongji
2021-05-17  9:08 ` [RFC PATCH 16/17] vsock: " Xie Yongji
2021-05-17  9:08 ` [RFC PATCH 17/17] virtio_ring: Add validation for " Xie Yongji
2021-05-17 23:39   ` Michael S. Tsirkin
2021-05-17 23:39     ` Michael S. Tsirkin
2021-05-25  1:31     ` Jason Wang
2021-05-25  1:31       ` Jason Wang
2021-05-25  5:08       ` Yongji Xie
2021-05-17 23:40 ` [RFC PATCH 00/17] " Michael S. Tsirkin
2021-05-17 23:40   ` Michael S. Tsirkin
2021-05-18  8:29   ` Yongji Xie
2021-05-18  9:52     ` Michael S. Tsirkin
2021-05-18  9:52       ` Michael S. Tsirkin
2021-05-18 12:28       ` Yongji Xie

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.