* [PATCH] hw/virtio/vhost: check nvqs at dev_start
@ 2023-09-01 12:23 Albert Esteve
2023-10-02 9:27 ` Michael S. Tsirkin
0 siblings, 1 reply; 3+ messages in thread
From: Albert Esteve @ 2023-09-01 12:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael S. Tsirkin, Albert Esteve
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;
--
2.41.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] hw/virtio/vhost: check nvqs at dev_start
2023-09-01 12:23 [PATCH] hw/virtio/vhost: check nvqs at dev_start Albert Esteve
@ 2023-10-02 9:27 ` Michael S. Tsirkin
2023-10-02 10:49 ` Albert Esteve
0 siblings, 1 reply; 3+ messages in thread
From: Michael S. Tsirkin @ 2023-10-02 9:27 UTC (permalink / raw)
To: Albert Esteve; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] hw/virtio/vhost: check nvqs at dev_start
2023-10-02 9:27 ` Michael S. Tsirkin
@ 2023-10-02 10:49 ` Albert Esteve
0 siblings, 0 replies; 3+ messages in thread
From: Albert Esteve @ 2023-10-02 10:49 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2034 bytes --]
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
>
>
[-- Attachment #2: Type: text/html, Size: 2712 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-10-02 10:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-01 12:23 [PATCH] hw/virtio/vhost: check nvqs at dev_start Albert Esteve
2023-10-02 9:27 ` Michael S. Tsirkin
2023-10-02 10:49 ` Albert Esteve
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.