Re: [dpdk-dev] [PATCH] vhost: avoid buffer overflow in update_secure_len

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Tue, Nov 17, 2015 at 08:39:30AM -0800, Rich Lane wrote:
> 
> I don't think that adding a SIGINT handler is the right solution, though. The
> guest app could be killed with another signal (SIGKILL).

Good point.

> Worse, a malicious or
> buggy guest could write to just that field. vhost should not crash no matter
> what the guest writes into the virtqueues.

Yeah, I agree with you: though we could fix this issue in the source
side, we also should do some defend here.

How about following patch then?

Note that the vec_id overflow check should be done before referencing
it, but not after. Hence I moved it ahead.

	--yliu

---
diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index 9322ce6..08f5942 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -132,6 +132,8 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
 
 		/* Get descriptor from available ring */
 		desc = &vq->desc[head[packet_success]];
+		if (desc->len == 0)
+			break;
 
 		buff = pkts[packet_success];
 
@@ -153,6 +155,8 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
 			/* Buffer address translation. */
 			buff_addr = gpa_to_vva(dev, desc->addr);
 		} else {
+			if (desc->len < vq->vhost_hlen)
+				break;
 			vb_offset += vq->vhost_hlen;
 			hdr = 1;
 		}
@@ -446,6 +450,9 @@ update_secure_len(struct vhost_virtqueue *vq, uint32_t id,
 	uint32_t vec_id = *vec_idx;
 
 	do {
+		if (vec_id >= BUF_VECTOR_MAX)
+			break;
+
 		next_desc = 0;
 		len += vq->desc[idx].len;
 		vq->buf_vec[vec_id].buf_addr = vq->desc[idx].addr;
@@ -519,6 +526,8 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
 					goto merge_rx_exit;
 				} else {
 					update_secure_len(vq, res_cur_idx, &secure_len, &vec_idx);
+					if (secure_len == 0)
+						goto merge_rx_exit;
 					res_cur_idx++;
 				}
 			} while (pkt_len > secure_len);
@@ -631,6 +640,8 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
 		uint8_t alloc_err = 0;
 
 		desc = &vq->desc[head[entry_success]];
+		if (desc->len == 0)
+			break;
 
 		/* Discard first buffer as it is the virtio header */
 		if (desc->flags & VRING_DESC_F_NEXT) {
@@ -638,6 +649,8 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
 			vb_offset = 0;
 			vb_avail = desc->len;
 		} else {
+			if (desc->len < vq->vhost_hlen)
+				break;
 			vb_offset = vq->vhost_hlen;
 			vb_avail = desc->len - vb_offset;
 		}