The series aims at improving virtqueue metadata pointers dereferencing robust. First patch fixes a real issue reported by our QE. Five next patches validates queue index passed as input of the Vhost API. They are split in 5 patches to ease backporting to LTSes. Last patch is not mandatory now that patch 1 ensures there won't be holes in the array of virtqueue metadata pointers, but I think it is a nice to have anyway. Maxime Coquelin (7): vhost: fix virtqueues metadata allocation vhost: validate index in available entries API vhost: validate index in guest notification API vhost: validate index in live-migration API vhost: validate index in inflight API vhost: validate index in async API vhost: check virtqueue metadata pointer lib/librte_vhost/vhost.c | 76 ++++++++++++++++++++++++++++------- lib/librte_vhost/vhost_user.c | 12 ++++++ 2 files changed, 74 insertions(+), 14 deletions(-) -- 2.26.2
The Vhost-user backend implementation assumes there will be no holes in the device's array of virtqueues metadata pointers. It can happen though, and would cause segmentation faults, memory leaks or undefined behaviour. This patch keep the assumption that there is no holes in this array, and allocate all uninitialized virtqueues metadata up to requested index. Fixes: 160cbc815b41 ("vhost: remove a hack on queue allocation") Cc: stable@dpdk.org Suggested-by: Adrian Moreno <amorenoz@redhat.com> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- lib/librte_vhost/vhost.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c index 6068c38ec6..0c9ba3b3af 100644 --- a/lib/librte_vhost/vhost.c +++ b/lib/librte_vhost/vhost.c @@ -579,22 +579,29 @@ int alloc_vring_queue(struct virtio_net *dev, uint32_t vring_idx) { struct vhost_virtqueue *vq; + uint32_t i; - vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0); - if (vq == NULL) { - VHOST_LOG_CONFIG(ERR, - "Failed to allocate memory for vring:%u.\n", vring_idx); - return -1; - } + /* Also allocate holes, if any, up to requested vring index. */ + for (i = 0; i <= vring_idx; i++) { + if (dev->virtqueue[i]) + continue; - dev->virtqueue[vring_idx] = vq; - init_vring_queue(dev, vring_idx); - rte_spinlock_init(&vq->access_lock); - vq->avail_wrap_counter = 1; - vq->used_wrap_counter = 1; - vq->signalled_used_valid = false; + vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0); + if (vq == NULL) { + VHOST_LOG_CONFIG(ERR, + "Failed to allocate memory for vring:%u.\n", i); + return -1; + } + + dev->virtqueue[i] = vq; + init_vring_queue(dev, vring_idx); + rte_spinlock_init(&vq->access_lock); + vq->avail_wrap_counter = 1; + vq->used_wrap_counter = 1; + vq->signalled_used_valid = false; + } - dev->nr_vring += 1; + dev->nr_vring = RTE_MAX(dev->nr_vring, vring_idx + 1); return 0; } -- 2.26.2
This patch validates the queue index parameter, in order to ensure neither out-of-bound accesses nor NULL pointer dereferencing happen. Fixes: a67f286a6596 ("vhost: export queue free entries") Cc: stable@dpdk.org Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- lib/librte_vhost/vhost.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c index 0c9ba3b3af..193dafc369 100644 --- a/lib/librte_vhost/vhost.c +++ b/lib/librte_vhost/vhost.c @@ -1260,7 +1260,12 @@ rte_vhost_avail_entries(int vid, uint16_t queue_id) if (!dev) return 0; + if (queue_id >= VHOST_MAX_VRING) + return 0; + vq = dev->virtqueue[queue_id]; + if (!vq) + return 0; rte_spinlock_lock(&vq->access_lock); -- 2.26.2
This patch validates the queue index parameter, in order to ensure neither out-of-bound accesses nor NULL pointer dereferencing happen. Fixes: 9eed6bfd2efb ("vhost: allow to enable or disable features") Cc: stable@dpdk.org Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- lib/librte_vhost/vhost.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c index 193dafc369..801a1a5098 100644 --- a/lib/librte_vhost/vhost.c +++ b/lib/librte_vhost/vhost.c @@ -1352,7 +1352,12 @@ rte_vhost_enable_guest_notification(int vid, uint16_t queue_id, int enable) if (!dev) return -1; + if (queue_id >= VHOST_MAX_VRING) + return -1; + vq = dev->virtqueue[queue_id]; + if (!vq) + return -1; rte_spinlock_lock(&vq->access_lock); -- 2.26.2
This patch validates the queue index parameter, in order to ensure no out-of-bound accesses happen. Fixes: bd2e0c3fe5ac ("vhost: add APIs for live migration") Cc: stable@dpdk.org Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- lib/librte_vhost/vhost.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c index 801a1a5098..b9afe46ca2 100644 --- a/lib/librte_vhost/vhost.c +++ b/lib/librte_vhost/vhost.c @@ -1467,6 +1467,9 @@ int rte_vhost_get_vring_base(int vid, uint16_t queue_id, if (dev == NULL || last_avail_idx == NULL || last_used_idx == NULL) return -1; + if (queue_id >= VHOST_MAX_VRING) + return -1; + vq = dev->virtqueue[queue_id]; if (!vq) return -1; @@ -1493,6 +1496,9 @@ int rte_vhost_set_vring_base(int vid, uint16_t queue_id, if (!dev) return -1; + if (queue_id >= VHOST_MAX_VRING) + return -1; + vq = dev->virtqueue[queue_id]; if (!vq) return -1; -- 2.26.2
This patch validates the queue index parameter, in order to ensure neither out-of-bound accesses nor NULL pointer dereferencing happen. Fixes: 4d891f77ddfa ("vhost: add APIs to get inflight ring") Cc: stable@dpdk.org Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- lib/librte_vhost/vhost.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c index b9afe46ca2..f78bdfcc94 100644 --- a/lib/librte_vhost/vhost.c +++ b/lib/librte_vhost/vhost.c @@ -1523,15 +1523,23 @@ rte_vhost_get_vring_base_from_inflight(int vid, uint16_t *last_used_idx) { struct rte_vhost_inflight_info_packed *inflight_info; + struct vhost_virtqueue *vq; struct virtio_net *dev = get_device(vid); if (dev == NULL || last_avail_idx == NULL || last_used_idx == NULL) return -1; + if (queue_id >= VHOST_MAX_VRING) + return -1; + + vq = dev->virtqueue[queue_id]; + if (!vq) + return -1; + if (!vq_is_packed(dev)) return -1; - inflight_info = dev->virtqueue[queue_id]->inflight_packed; + inflight_info = vq->inflight_packed; if (!inflight_info) return -1; -- 2.26.2
This patch validates the queue index parameter, in order to ensure no out-of-bound accesses happen. Fixes: 9eed6bfd2efb ("vhost: allow to enable or disable features") Cc: stable@dpdk.org Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- lib/librte_vhost/vhost.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c index f78bdfcc94..e92ff618ac 100644 --- a/lib/librte_vhost/vhost.c +++ b/lib/librte_vhost/vhost.c @@ -1577,6 +1577,9 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id, f.intval = features; + if (queue_id >= VHOST_MAX_VRING) + return -1; + vq = dev->virtqueue[queue_id]; if (unlikely(vq == NULL || !dev->async_copy)) @@ -1658,6 +1661,9 @@ int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id) if (dev == NULL) return ret; + if (queue_id >= VHOST_MAX_VRING) + return ret; + vq = dev->virtqueue[queue_id]; if (vq == NULL) -- 2.26.2
This patch checks whether the virtqueue metadata pointer is valid before dereferencing it. It is not considered a fix as earlier patch ensures there are no holes in the array of virtqueue metadata pointers. Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- lib/librte_vhost/vhost.c | 11 +++++++++++ lib/librte_vhost/vhost_user.c | 12 ++++++++++++ 2 files changed, 23 insertions(+) diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c index e92ff618ac..8a151a9c1d 100644 --- a/lib/librte_vhost/vhost.c +++ b/lib/librte_vhost/vhost.c @@ -544,6 +544,11 @@ init_vring_queue(struct virtio_net *dev, uint32_t vring_idx) } vq = dev->virtqueue[vring_idx]; + if (!vq) { + VHOST_LOG_CONFIG(ERR, "Virtqueue not allocated (%d)\n", + vring_idx); + return; + } memset(vq, 0, sizeof(struct vhost_virtqueue)); @@ -570,6 +575,12 @@ reset_vring_queue(struct virtio_net *dev, uint32_t vring_idx) } vq = dev->virtqueue[vring_idx]; + if (!vq) { + VHOST_LOG_CONFIG(ERR, "Virtqueue not allocated (%d)\n", + vring_idx); + return; + } + callfd = vq->callfd; init_vring_queue(dev, vring_idx); vq->callfd = callfd; diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c index d20c8c57ad..8a8726f8b8 100644 --- a/lib/librte_vhost/vhost_user.c +++ b/lib/librte_vhost/vhost_user.c @@ -1235,6 +1235,9 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg, for (i = 0; i < dev->nr_vring; i++) { struct vhost_virtqueue *vq = dev->virtqueue[i]; + if (!vq) + continue; + if (vq->desc || vq->avail || vq->used) { /* * If the memory table got updated, the ring addresses @@ -1556,6 +1559,9 @@ vhost_user_set_inflight_fd(struct virtio_net **pdev, VhostUserMsg *msg, for (i = 0; i < num_queues; i++) { vq = dev->virtqueue[i]; + if (!vq) + continue; + if (vq_is_packed(dev)) { vq->inflight_packed = addr; vq->inflight_packed->desc_num = queue_size; @@ -2310,6 +2316,9 @@ vhost_user_iotlb_msg(struct virtio_net **pdev, struct VhostUserMsg *msg, for (i = 0; i < dev->nr_vring; i++) { struct vhost_virtqueue *vq = dev->virtqueue[i]; + if (!vq) + continue; + vhost_user_iotlb_cache_insert(vq, imsg->iova, vva, len, imsg->perm); @@ -2321,6 +2330,9 @@ vhost_user_iotlb_msg(struct virtio_net **pdev, struct VhostUserMsg *msg, for (i = 0; i < dev->nr_vring; i++) { struct vhost_virtqueue *vq = dev->virtqueue[i]; + if (!vq) + continue; + vhost_user_iotlb_cache_remove(vq, imsg->iova, imsg->size); -- 2.26.2
Hi Maxime, > -----Original Message----- > From: Maxime Coquelin <maxime.coquelin@redhat.com> > Sent: Tuesday, October 20, 2020 1:34 AM > To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com > Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org > Subject: [PATCH 1/7] vhost: fix virtqueues metadata allocation > > The Vhost-user backend implementation assumes there will be > no holes in the device's array of virtqueues metadata > pointers. > > It can happen though, and would cause segmentation faults, > memory leaks or undefined behaviour. Could I ask when will this happen? When QEMU does not configure all virtqueues? I'm not very sure. Could you point that out for me? Thanks! Chenbo > > This patch keep the assumption that there is no holes in this > array, and allocate all uninitialized virtqueues metadata up > to requested index. > > Fixes: 160cbc815b41 ("vhost: remove a hack on queue allocation") > Cc: stable@dpdk.org > > Suggested-by: Adrian Moreno <amorenoz@redhat.com> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > --- > lib/librte_vhost/vhost.c | 33 ++++++++++++++++++++------------- > 1 file changed, 20 insertions(+), 13 deletions(-) > > diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c > index 6068c38ec6..0c9ba3b3af 100644 > --- a/lib/librte_vhost/vhost.c > +++ b/lib/librte_vhost/vhost.c > @@ -579,22 +579,29 @@ int > alloc_vring_queue(struct virtio_net *dev, uint32_t vring_idx) > { > struct vhost_virtqueue *vq; > + uint32_t i; > > - vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0); > - if (vq == NULL) { > - VHOST_LOG_CONFIG(ERR, > - "Failed to allocate memory for vring:%u.\n", vring_idx); > - return -1; > - } > + /* Also allocate holes, if any, up to requested vring index. */ > + for (i = 0; i <= vring_idx; i++) { > + if (dev->virtqueue[i]) > + continue; > > - dev->virtqueue[vring_idx] = vq; > - init_vring_queue(dev, vring_idx); > - rte_spinlock_init(&vq->access_lock); > - vq->avail_wrap_counter = 1; > - vq->used_wrap_counter = 1; > - vq->signalled_used_valid = false; > + vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0); > + if (vq == NULL) { > + VHOST_LOG_CONFIG(ERR, > + "Failed to allocate memory for vring:%u.\n", i); > + return -1; > + } > + > + dev->virtqueue[i] = vq; > + init_vring_queue(dev, vring_idx); > + rte_spinlock_init(&vq->access_lock); > + vq->avail_wrap_counter = 1; > + vq->used_wrap_counter = 1; > + vq->signalled_used_valid = false; > + } > > - dev->nr_vring += 1; > + dev->nr_vring = RTE_MAX(dev->nr_vring, vring_idx + 1); > > return 0; > } > -- > 2.26.2
Hi Maxime,
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, October 20, 2020 1:34 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> Subject: [PATCH 2/7] vhost: validate index in available entries API
>
> This patch validates the queue index parameter, in order
> to ensure neither out-of-bound accesses nor NULL pointer
> dereferencing happen.
>
> Fixes: a67f286a6596 ("vhost: export queue free entries")
> Cc: stable@dpdk.org
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> lib/librte_vhost/vhost.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index 0c9ba3b3af..193dafc369 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -1260,7 +1260,12 @@ rte_vhost_avail_entries(int vid, uint16_t queue_id)
> if (!dev)
> return 0;
>
> + if (queue_id >= VHOST_MAX_VRING)
> + return 0;
> +
> vq = dev->virtqueue[queue_id];
> + if (!vq)
> + return 0;
>
> rte_spinlock_lock(&vq->access_lock);
>
> --
> 2.26.2
Looking at the API again, I don't know if it is good to return 0 when there are no
available entries or other errors.
For this patch:
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, October 20, 2020 1:34 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> Subject: [PATCH 3/7] vhost: validate index in guest notification API
>
> This patch validates the queue index parameter, in order
> to ensure neither out-of-bound accesses nor NULL pointer
> dereferencing happen.
>
> Fixes: 9eed6bfd2efb ("vhost: allow to enable or disable features")
> Cc: stable@dpdk.org
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> lib/librte_vhost/vhost.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index 193dafc369..801a1a5098 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -1352,7 +1352,12 @@ rte_vhost_enable_guest_notification(int vid,
> uint16_t queue_id, int enable)
> if (!dev)
> return -1;
>
> + if (queue_id >= VHOST_MAX_VRING)
> + return -1;
> +
> vq = dev->virtqueue[queue_id];
> + if (!vq)
> + return -1;
>
> rte_spinlock_lock(&vq->access_lock);
>
> --
> 2.26.2
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, October 20, 2020 1:34 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> Subject: [PATCH 4/7] vhost: validate index in live-migration API
>
> This patch validates the queue index parameter, in order
> to ensure no out-of-bound accesses happen.
>
> Fixes: bd2e0c3fe5ac ("vhost: add APIs for live migration")
> Cc: stable@dpdk.org
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> lib/librte_vhost/vhost.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index 801a1a5098..b9afe46ca2 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -1467,6 +1467,9 @@ int rte_vhost_get_vring_base(int vid, uint16_t
> queue_id,
> if (dev == NULL || last_avail_idx == NULL || last_used_idx == NULL)
> return -1;
>
> + if (queue_id >= VHOST_MAX_VRING)
> + return -1;
> +
> vq = dev->virtqueue[queue_id];
> if (!vq)
> return -1;
> @@ -1493,6 +1496,9 @@ int rte_vhost_set_vring_base(int vid, uint16_t
> queue_id,
> if (!dev)
> return -1;
>
> + if (queue_id >= VHOST_MAX_VRING)
> + return -1;
> +
> vq = dev->virtqueue[queue_id];
> if (!vq)
> return -1;
> --
> 2.26.2
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, October 20, 2020 1:34 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> Subject: [PATCH 5/7] vhost: validate index in inflight API
>
> This patch validates the queue index parameter, in order
> to ensure neither out-of-bound accesses nor NULL pointer
> dereferencing happen.
>
> Fixes: 4d891f77ddfa ("vhost: add APIs to get inflight ring")
> Cc: stable@dpdk.org
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> lib/librte_vhost/vhost.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index b9afe46ca2..f78bdfcc94 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -1523,15 +1523,23 @@ rte_vhost_get_vring_base_from_inflight(int vid,
> uint16_t *last_used_idx)
> {
> struct rte_vhost_inflight_info_packed *inflight_info;
> + struct vhost_virtqueue *vq;
> struct virtio_net *dev = get_device(vid);
>
> if (dev == NULL || last_avail_idx == NULL || last_used_idx == NULL)
> return -1;
>
> + if (queue_id >= VHOST_MAX_VRING)
> + return -1;
> +
> + vq = dev->virtqueue[queue_id];
> + if (!vq)
> + return -1;
> +
> if (!vq_is_packed(dev))
> return -1;
>
> - inflight_info = dev->virtqueue[queue_id]->inflight_packed;
> + inflight_info = vq->inflight_packed;
> if (!inflight_info)
> return -1;
>
> --
> 2.26.2
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, October 20, 2020 1:34 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> Subject: [PATCH 6/7] vhost: validate index in async API
>
> This patch validates the queue index parameter, in order
> to ensure no out-of-bound accesses happen.
>
> Fixes: 9eed6bfd2efb ("vhost: allow to enable or disable features")
> Cc: stable@dpdk.org
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> lib/librte_vhost/vhost.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index f78bdfcc94..e92ff618ac 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -1577,6 +1577,9 @@ int rte_vhost_async_channel_register(int vid,
> uint16_t queue_id,
>
> f.intval = features;
>
> + if (queue_id >= VHOST_MAX_VRING)
> + return -1;
> +
> vq = dev->virtqueue[queue_id];
>
> if (unlikely(vq == NULL || !dev->async_copy))
> @@ -1658,6 +1661,9 @@ int rte_vhost_async_channel_unregister(int vid,
> uint16_t queue_id)
> if (dev == NULL)
> return ret;
>
> + if (queue_id >= VHOST_MAX_VRING)
> + return ret;
> +
> vq = dev->virtqueue[queue_id];
>
> if (vq == NULL)
> --
> 2.26.2
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, October 20, 2020 1:34 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [PATCH 7/7] vhost: check virtqueue metadata pointer
>
> This patch checks whether the virtqueue metadata pointer
> is valid before dereferencing it. It is not considered
> a fix as earlier patch ensures there are no holes in the
> array of virtqueue metadata pointers.
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> lib/librte_vhost/vhost.c | 11 +++++++++++
> lib/librte_vhost/vhost_user.c | 12 ++++++++++++
> 2 files changed, 23 insertions(+)
>
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index e92ff618ac..8a151a9c1d 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -544,6 +544,11 @@ init_vring_queue(struct virtio_net *dev, uint32_t
> vring_idx)
> }
>
> vq = dev->virtqueue[vring_idx];
> + if (!vq) {
> + VHOST_LOG_CONFIG(ERR, "Virtqueue not allocated (%d)\n",
> + vring_idx);
> + return;
> + }
>
> memset(vq, 0, sizeof(struct vhost_virtqueue));
>
> @@ -570,6 +575,12 @@ reset_vring_queue(struct virtio_net *dev, uint32_t
> vring_idx)
> }
>
> vq = dev->virtqueue[vring_idx];
> + if (!vq) {
> + VHOST_LOG_CONFIG(ERR, "Virtqueue not allocated (%d)\n",
> + vring_idx);
> + return;
> + }
> +
> callfd = vq->callfd;
> init_vring_queue(dev, vring_idx);
> vq->callfd = callfd;
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index d20c8c57ad..8a8726f8b8 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1235,6 +1235,9 @@ vhost_user_set_mem_table(struct virtio_net **pdev,
> struct VhostUserMsg *msg,
> for (i = 0; i < dev->nr_vring; i++) {
> struct vhost_virtqueue *vq = dev->virtqueue[i];
>
> + if (!vq)
> + continue;
> +
> if (vq->desc || vq->avail || vq->used) {
> /*
> * If the memory table got updated, the ring addresses
> @@ -1556,6 +1559,9 @@ vhost_user_set_inflight_fd(struct virtio_net **pdev,
> VhostUserMsg *msg,
>
> for (i = 0; i < num_queues; i++) {
> vq = dev->virtqueue[i];
> + if (!vq)
> + continue;
> +
> if (vq_is_packed(dev)) {
> vq->inflight_packed = addr;
> vq->inflight_packed->desc_num = queue_size;
> @@ -2310,6 +2316,9 @@ vhost_user_iotlb_msg(struct virtio_net **pdev,
> struct VhostUserMsg *msg,
> for (i = 0; i < dev->nr_vring; i++) {
> struct vhost_virtqueue *vq = dev->virtqueue[i];
>
> + if (!vq)
> + continue;
> +
> vhost_user_iotlb_cache_insert(vq, imsg->iova, vva,
> len, imsg->perm);
>
> @@ -2321,6 +2330,9 @@ vhost_user_iotlb_msg(struct virtio_net **pdev,
> struct VhostUserMsg *msg,
> for (i = 0; i < dev->nr_vring; i++) {
> struct vhost_virtqueue *vq = dev->virtqueue[i];
>
> + if (!vq)
> + continue;
> +
> vhost_user_iotlb_cache_remove(vq, imsg->iova,
> imsg->size);
>
> --
> 2.26.2
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
Hi Chenbon On 10/21/20 1:10 PM, Xia, Chenbo wrote: > Hi Maxime, > >> -----Original Message----- >> From: Maxime Coquelin <maxime.coquelin@redhat.com> >> Sent: Tuesday, October 20, 2020 1:34 AM >> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com >> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org >> Subject: [PATCH 1/7] vhost: fix virtqueues metadata allocation >> >> The Vhost-user backend implementation assumes there will be >> no holes in the device's array of virtqueues metadata >> pointers. >> >> It can happen though, and would cause segmentation faults, >> memory leaks or undefined behaviour. > > Could I ask when will this happen? > > When QEMU does not configure all virtqueues? I'm not very sure. > Could you point that out for me? It has been reported by our QE when doing reconnect with multiqueue with vIOMMU enabled: https://bugzilla.redhat.com/show_bug.cgi?id=1880299 Regards, Maxime > Thanks! > Chenbo > >> >> This patch keep the assumption that there is no holes in this >> array, and allocate all uninitialized virtqueues metadata up >> to requested index. >> >> Fixes: 160cbc815b41 ("vhost: remove a hack on queue allocation") >> Cc: stable@dpdk.org >> >> Suggested-by: Adrian Moreno <amorenoz@redhat.com> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >> --- >> lib/librte_vhost/vhost.c | 33 ++++++++++++++++++++------------- >> 1 file changed, 20 insertions(+), 13 deletions(-) >> >> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c >> index 6068c38ec6..0c9ba3b3af 100644 >> --- a/lib/librte_vhost/vhost.c >> +++ b/lib/librte_vhost/vhost.c >> @@ -579,22 +579,29 @@ int >> alloc_vring_queue(struct virtio_net *dev, uint32_t vring_idx) >> { >> struct vhost_virtqueue *vq; >> + uint32_t i; >> >> - vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0); >> - if (vq == NULL) { >> - VHOST_LOG_CONFIG(ERR, >> - "Failed to allocate memory for vring:%u.\n", vring_idx); >> - return -1; >> - } >> + /* Also allocate holes, if any, up to requested vring index. */ >> + for (i = 0; i <= vring_idx; i++) { >> + if (dev->virtqueue[i]) >> + continue; >> >> - dev->virtqueue[vring_idx] = vq; >> - init_vring_queue(dev, vring_idx); >> - rte_spinlock_init(&vq->access_lock); >> - vq->avail_wrap_counter = 1; >> - vq->used_wrap_counter = 1; >> - vq->signalled_used_valid = false; >> + vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0); >> + if (vq == NULL) { >> + VHOST_LOG_CONFIG(ERR, >> + "Failed to allocate memory for vring:%u.\n", i); >> + return -1; >> + } >> + >> + dev->virtqueue[i] = vq; >> + init_vring_queue(dev, vring_idx); >> + rte_spinlock_init(&vq->access_lock); >> + vq->avail_wrap_counter = 1; >> + vq->used_wrap_counter = 1; >> + vq->signalled_used_valid = false; >> + } >> >> - dev->nr_vring += 1; >> + dev->nr_vring = RTE_MAX(dev->nr_vring, vring_idx + 1); >> >> return 0; >> } >> -- >> 2.26.2 >
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Wednesday, October 21, 2020 8:07 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org; amorenoz@redhat.com
> Cc: stable@dpdk.org
> Subject: Re: [PATCH 1/7] vhost: fix virtqueues metadata allocation
>
> Hi Chenbon
>
> On 10/21/20 1:10 PM, Xia, Chenbo wrote:
> > Hi Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Tuesday, October 20, 2020 1:34 AM
> >> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>;
> amorenoz@redhat.com
> >> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> >> Subject: [PATCH 1/7] vhost: fix virtqueues metadata allocation
> >>
> >> The Vhost-user backend implementation assumes there will be
> >> no holes in the device's array of virtqueues metadata
> >> pointers.
> >>
> >> It can happen though, and would cause segmentation faults,
> >> memory leaks or undefined behaviour.
> >
> > Could I ask when will this happen?
> >
> > When QEMU does not configure all virtqueues? I'm not very sure.
> > Could you point that out for me?
>
> It has been reported by our QE when doing reconnect with multiqueue with
> vIOMMU enabled:
> https://bugzilla.redhat.com/show_bug.cgi?id=1880299
>
> Regards,
> Maxime
>
> > Thanks!
> > Chenbo
> >
> >>
> >> This patch keep the assumption that there is no holes in this
> >> array, and allocate all uninitialized virtqueues metadata up
> >> to requested index.
> >>
> >> Fixes: 160cbc815b41 ("vhost: remove a hack on queue allocation")
> >> Cc: stable@dpdk.org
> >>
> >> Suggested-by: Adrian Moreno <amorenoz@redhat.com>
> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> ---
> >> lib/librte_vhost/vhost.c | 33 ++++++++++++++++++++-------------
> >> 1 file changed, 20 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> >> index 6068c38ec6..0c9ba3b3af 100644
> >> --- a/lib/librte_vhost/vhost.c
> >> +++ b/lib/librte_vhost/vhost.c
> >> @@ -579,22 +579,29 @@ int
> >> alloc_vring_queue(struct virtio_net *dev, uint32_t vring_idx)
> >> {
> >> struct vhost_virtqueue *vq;
> >> + uint32_t i;
> >>
> >> - vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
> >> - if (vq == NULL) {
> >> - VHOST_LOG_CONFIG(ERR,
> >> - "Failed to allocate memory for vring:%u.\n", vring_idx);
> >> - return -1;
> >> - }
> >> + /* Also allocate holes, if any, up to requested vring index. */
> >> + for (i = 0; i <= vring_idx; i++) {
> >> + if (dev->virtqueue[i])
> >> + continue;
> >>
> >> - dev->virtqueue[vring_idx] = vq;
> >> - init_vring_queue(dev, vring_idx);
> >> - rte_spinlock_init(&vq->access_lock);
> >> - vq->avail_wrap_counter = 1;
> >> - vq->used_wrap_counter = 1;
> >> - vq->signalled_used_valid = false;
> >> + vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
> >> + if (vq == NULL) {
> >> + VHOST_LOG_CONFIG(ERR,
> >> + "Failed to allocate memory for vring:%u.\n", i);
> >> + return -1;
> >> + }
> >> +
> >> + dev->virtqueue[i] = vq;
> >> + init_vring_queue(dev, vring_idx);
> >> + rte_spinlock_init(&vq->access_lock);
> >> + vq->avail_wrap_counter = 1;
> >> + vq->used_wrap_counter = 1;
> >> + vq->signalled_used_valid = false;
> >> + }
> >>
> >> - dev->nr_vring += 1;
> >> + dev->nr_vring = RTE_MAX(dev->nr_vring, vring_idx + 1);
> >>
> >> return 0;
> >> }
> >> --
> >> 2.26.2
> >
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
On 10/19/20 7:34 PM, Maxime Coquelin wrote:
> The series aims at improving virtqueue metadata pointers
> dereferencing robust.
>
> First patch fixes a real issue reported by our QE.
>
> Five next patches validates queue index passed as input
> of the Vhost API. They are split in 5 patches to ease
> backporting to LTSes.
>
> Last patch is not mandatory now that patch 1 ensures there
> won't be holes in the array of virtqueue metadata pointers,
> but I think it is a nice to have anyway.
>
> Maxime Coquelin (7):
> vhost: fix virtqueues metadata allocation
> vhost: validate index in available entries API
> vhost: validate index in guest notification API
> vhost: validate index in live-migration API
> vhost: validate index in inflight API
> vhost: validate index in async API
> vhost: check virtqueue metadata pointer
>
> lib/librte_vhost/vhost.c | 76 ++++++++++++++++++++++++++++-------
> lib/librte_vhost/vhost_user.c | 12 ++++++
> 2 files changed, 74 insertions(+), 14 deletions(-)
>
Applied to dpdk-next-virtio/main.
Thanks,
Maxime