diff mbox

[dpdk-dev,1/3] vhost: fix dead loop in enqueue path

Message ID 1485074820-8956-2-git-send-email-yuanhan.liu@linux.intel.com (mailing list archive)
State Accepted, archived
Delegated to: Yuanhan Liu
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel compilation success Compilation OK

Commit Message

Yuanhan Liu Jan. 22, 2017, 8:46 a.m. UTC 
  If a malicious guest forges a dead loop desc chain (let desc->next point
to itself) and desc->len is zero, this could lead to a dead loop in
copy_mbuf_to_desc(following is a simplified code to show this issue
clearly):

    while (mbuf_is_not_totally_consumed) {
        if (desc_avail == 0) {
            desc = &descs[desc->next];
            desc_avail = desc->len;
        }

        COPY(desc, mbuf, desc_avail);
    }

I have actually fixed a same issue before: a436f53ebfeb ("vhost: avoid
dead loop chain"); it fixes the dequeue path though, leaving the enqueue
path still vulnerable.

The fix is the same. Add a var nr_desc to avoid the dead loop.

Fixes: f1a519ad981c ("vhost: fix enqueue/dequeue to handle chained vring descriptors")

Cc: stable@dpdk.org
Reported-by: Xieming Katty <katty.xieming@huawei.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 lib/librte_vhost/virtio_net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Maxime Coquelin Jan. 23, 2017, 7:56 a.m. UTC | #1 
On 01/22/2017 09:46 AM, Yuanhan Liu wrote:
> If a malicious guest forges a dead loop desc chain (let desc->next point
> to itself) and desc->len is zero, this could lead to a dead loop in
> copy_mbuf_to_desc(following is a simplified code to show this issue
> clearly):
>
>     while (mbuf_is_not_totally_consumed) {
>         if (desc_avail == 0) {
>             desc = &descs[desc->next];
>             desc_avail = desc->len;
>         }
>
>         COPY(desc, mbuf, desc_avail);
>     }
>
> I have actually fixed a same issue before: a436f53ebfeb ("vhost: avoid
> dead loop chain"); it fixes the dequeue path though, leaving the enqueue
> path still vulnerable.
>
> The fix is the same. Add a var nr_desc to avoid the dead loop.
>
> Fixes: f1a519ad981c ("vhost: fix enqueue/dequeue to handle chained vring descriptors")
>
> Cc: stable@dpdk.org
> Reported-by: Xieming Katty <katty.xieming@huawei.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
>  lib/librte_vhost/virtio_net.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Thanks for the fix:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

  - Maxime
diff mbox

Patch

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index 595f67c..143c0fa 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -195,6 +195,8 @@  static inline int __attribute__((always_inline))
 	struct vring_desc *desc;
 	uint64_t desc_addr;
 	struct virtio_net_hdr_mrg_rxbuf virtio_hdr = {{0, 0, 0, 0, 0, 0}, 0};
+	/* A counter to avoid desc dead loop chain */
+	uint16_t nr_desc = 1;
 
 	desc = &descs[desc_idx];
 	desc_addr = gpa_to_vva(dev, desc->addr);
@@ -233,7 +235,7 @@  static inline int __attribute__((always_inline))
 				/* Room in vring buffer is not enough */
 				return -1;
 			}
-			if (unlikely(desc->next >= size))
+			if (unlikely(desc->next >= size || ++nr_desc > size))
 				return -1;
 
 			desc = &descs[desc->next];