| Message ID | 20230901122323.66183-1-aesteve@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/virtio/vhost: check nvqs at dev_start | expand |
On Fri, Sep 01, 2023 at 02:23:23PM +0200, Albert Esteve wrote: > While this is not expected to happen, it could still > be that a vhost_dev did not set its nvqs member. > > Since `vhost_dev_start` access the device's vqs array > later without checking its size, it would cause a > Segmentation fault when nvqs is 0. > > To avoid this `rare` case and made the code safer, > add a clause that ensures nvqs has been set, and > warn the user if it has not. > > Signed-off-by: Albert Esteve <aesteve@redhat.com> > --- > hw/virtio/vhost.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index e2f6ffb446..78805fe5b7 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -1935,6 +1935,11 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings) > hdev->started = true; > hdev->vdev = vdev; > > + if (!hdev->nvqs) { > + error_report("device nvqs not set"); > + goto fail_nvqs; > + } > + > r = vhost_dev_set_features(hdev, hdev->log_enabled); > if (r < 0) { > goto fail_features; > @@ -2028,6 +2033,7 @@ fail_mem: > if (vhost_dev_has_iommu(hdev)) { > memory_listener_unregister(&hdev->iommu_listener); > } > +fail_nvqs: > fail_features: > vdev->vhost_started = false; > hdev->started = false; What do we want to return in this case? ATM the value we return (r) will be uninitialized. > -- > 2.41.0
Ah I see, I wanted to move the fail check as early as possible, and went a bit too far ahead, before initialisation. But is ok, it needs its own value either way. What about returning -EFAULT? Or maybe -EINVAL? I think they would fit for this error. And then I can use `VHOST_OPS_DEBUG` to make it consistent and print the error number. On Mon, Oct 2, 2023 at 11:27 AM Michael S. Tsirkin <mst@redhat.com> wrote: > On Fri, Sep 01, 2023 at 02:23:23PM +0200, Albert Esteve wrote: > > While this is not expected to happen, it could still > > be that a vhost_dev did not set its nvqs member. > > > > Since `vhost_dev_start` access the device's vqs array > > later without checking its size, it would cause a > > Segmentation fault when nvqs is 0. > > > > To avoid this `rare` case and made the code safer, > > add a clause that ensures nvqs has been set, and > > warn the user if it has not. > > > > Signed-off-by: Albert Esteve <aesteve@redhat.com> > > --- > > hw/virtio/vhost.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > index e2f6ffb446..78805fe5b7 100644 > > --- a/hw/virtio/vhost.c > > +++ b/hw/virtio/vhost.c > > @@ -1935,6 +1935,11 @@ int vhost_dev_start(struct vhost_dev *hdev, > VirtIODevice *vdev, bool vrings) > > hdev->started = true; > > hdev->vdev = vdev; > > > > + if (!hdev->nvqs) { > > + error_report("device nvqs not set"); > > + goto fail_nvqs; > > + } > > + > > r = vhost_dev_set_features(hdev, hdev->log_enabled); > > if (r < 0) { > > goto fail_features; > > @@ -2028,6 +2033,7 @@ fail_mem: > > if (vhost_dev_has_iommu(hdev)) { > > memory_listener_unregister(&hdev->iommu_listener); > > } > > +fail_nvqs: > > fail_features: > > vdev->vhost_started = false; > > hdev->started = false; > > What do we want to return in this case? > ATM the value we return (r) will be uninitialized. > > > -- > > 2.41.0 > >
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index e2f6ffb446..78805fe5b7 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1935,6 +1935,11 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings) hdev->started = true; hdev->vdev = vdev; + if (!hdev->nvqs) { + error_report("device nvqs not set"); + goto fail_nvqs; + } + r = vhost_dev_set_features(hdev, hdev->log_enabled); if (r < 0) { goto fail_features; @@ -2028,6 +2033,7 @@ fail_mem: if (vhost_dev_has_iommu(hdev)) { memory_listener_unregister(&hdev->iommu_listener); } +fail_nvqs: fail_features: vdev->vhost_started = false; hdev->started = false;
While this is not expected to happen, it could still be that a vhost_dev did not set its nvqs member. Since `vhost_dev_start` access the device's vqs array later without checking its size, it would cause a Segmentation fault when nvqs is 0. To avoid this `rare` case and made the code safer, add a clause that ensures nvqs has been set, and warn the user if it has not. Signed-off-by: Albert Esteve <aesteve@redhat.com> --- hw/virtio/vhost.c | 6 ++++++ 1 file changed, 6 insertions(+)