[RFC PATCH 00/17] Add validation for used length

[RFC PATCH 00/17] Add validation for used length

[Xie Yongji]

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

[RFC PATCH 01/17] virtio_ring: Avoid reading unneeded used length

[Xie Yongji]

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

Re: [RFC PATCH 01/17] virtio_ring: Avoid reading unneeded used length

[Johannes Berg]

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

Re: Re: [RFC PATCH 01/17] virtio_ring: Avoid reading unneeded used length

[Yongji Xie]

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

[RFC PATCH 02/17] virtio-blk: Remove unused used length

[Xie Yongji]

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

[RFC PATCH 03/17] virtio_console: Remove unused used length

[Xie Yongji]

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

[RFC PATCH 04/17] crypto: virtio - Remove unused used length

[Xie Yongji]

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

[RFC PATCH 05/17] drm/virtio: Remove unused used length

[Xie Yongji]

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

[RFC PATCH 06/17] caif_virtio: Remove unused used length

[Xie Yongji]

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

[RFC PATCH 07/17] virtio_net: Remove unused used length

[Xie Yongji]

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

[RFC PATCH 08/17] mac80211_hwsim: Remove unused used length

[Xie Yongji]

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

[RFC PATCH 09/17] virtio_pmem: Remove unused used length

[Xie Yongji]

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

[RFC PATCH 10/17] rpmsg: virtio: Remove unused used length

[Xie Yongji]

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

[RFC PATCH 11/17] virtio_scsi: Remove unused used length

[Xie Yongji]

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

[RFC PATCH 12/17] virtio_balloon: Remove unused used length

[Xie Yongji]

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

[RFC PATCH 13/17] virtio_input: Remove unused used length

[Xie Yongji]

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

[RFC PATCH 14/17] virtio_mem: Remove unused used length

[Xie Yongji]

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

[RFC PATCH 15/17] virtiofs: Remove unused used length

[Xie Yongji]

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

[RFC PATCH 16/17] vsock: Remove unused used length

[Xie Yongji]

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

[RFC PATCH 17/17] virtio_ring: Add validation for used length

[Xie Yongji]

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

Re: [RFC PATCH 17/17] virtio_ring: Add validation for used length

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


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

Re: [RFC PATCH 17/17] virtio_ring: Add validation for used length

[Jason Wang]

在 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

Re: Re: [RFC PATCH 17/17] virtio_ring: Add validation for used length

[Yongji Xie]

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

Re: [RFC PATCH 00/17] Add validation for used length

[Michael S. Tsirkin]

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

Re: Re: [RFC PATCH 00/17] Add validation for used length

[Yongji Xie]

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

Re: Re: [RFC PATCH 00/17] Add validation for used length

[Michael S. Tsirkin]

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

Re: Re: Re: [RFC PATCH 00/17] Add validation for used length

[Yongji Xie]

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