--- 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");