⬢ DragonFlyBSD Kernel Audit
DF-4623 / fix.diff
← back to finding ↓ download raw
--- a/sys/dev/virtual/virtio/virtio/virtqueue.c
+++ b/sys/dev/virtual/virtio/virtio/virtqueue.c
@@ -544,6 +544,7 @@
 virtqueue_dequeue(struct virtqueue *vq, uint32_t *len)
 {
 	struct vring_used_elem *uep;
+	struct vq_desc_extra *dxp;
 	void *cookie;
 	uint16_t used_idx, desc_idx;
 
@@ -557,14 +558,31 @@
 	uep = &vq->vq_ring.used->ring[used_idx];
 
 	desc_idx = (uint16_t) uep->id;
+	/*
+	 * The used ring is device-writable shared memory; never trust
+	 * the completion id. It must be bounded AND must refer to a
+	 * descriptor chain we actually enqueued (cookie set, non-zero
+	 * descriptor count). Otherwise a malicious or buggy host can
+	 * double-free a chain: vq_ring_free_chain() underflows ndescs,
+	 * creates free-list cycles, aliases descriptors across in-flight
+	 * requests (mis-attributed completions), or spins forever on a
+	 * forged cyclic chain.
+	 */
+	dxp = &vq->vq_descx[desc_idx];
+	if (desc_idx >= vq->vq_nentries ||
+	    dxp->cookie == NULL || dxp->ndescs == 0) {
+		device_printf(vq->vq_dev, "%s: dropping bogus used entry "
+		    "id=%u\n", vq->vq_name, desc_idx);
+		return (NULL);
+	}
 	if (len != NULL)
 		*len = uep->len;
 
 	vq_ring_free_chain(vq, desc_idx);
 
-	cookie = vq->vq_descx[desc_idx].cookie;
+	cookie = dxp->cookie;
 	VQASSERT(vq, cookie != NULL, "no cookie for index %d", desc_idx);
-	vq->vq_descx[desc_idx].cookie = NULL;
+	dxp->cookie = NULL;
 
 	return (cookie);
 }
@@ -787,10 +805,21 @@
 	dxp->ndescs--;
 
 	if ((dp->flags & VRING_DESC_F_INDIRECT) == 0) {
+		uint16_t walked = 0;
 		while (dp->flags & VRING_DESC_F_NEXT) {
 			VQ_RING_ASSERT_VALID_IDX(vq, dp->next);
 			dp = &vq->vq_ring.desc[dp->next];
 			dxp->ndescs--;
+			/*
+			 * Defensive: a cyclic chain written by the host
+			 * would loop forever; stop instead of spinning.
+			 */
+			if (++walked > vq->vq_nentries) {
+				device_printf(vq->vq_dev, "%s: descriptor "
+				    "chain cycle at %u\n", vq->vq_name,
+				    desc_idx);
+				return;
+			}
 		}
 	}
 	VQASSERT(vq, dxp->ndescs == 0, "failed to free entire desc chain");