1. Patchew
  2. QEMU
  3. View series

[PATCH] virtio-pci: Fix the use of an uninitialized irqfd.

Cindy Lu posted 1 patch 6 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240522051042.985825-1-lulu@redhat.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>
There is a newer version of this series
hw/virtio/virtio-pci.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
[PATCH] virtio-pci: Fix the use of an uninitialized irqfd.
Posted by Cindy Lu 6 months, 1 week ago 
The crash was reported in MAC OS and NixOS, here is the link for this bug
https://gitlab.com/qemu-project/qemu/-/issues/2334
https://gitlab.com/qemu-project/qemu/-/issues/2321

The root cause is that the function virtio_pci_set_guest_notifiers() only
initializes the irqfd when the use_guest_notifier_mask and guest_notifier_mask
are set.
However, this check is missing in virtio_pci_set_vector().
So the fix is to add this check.

This fix is verified in vyatta,MacOS,NixOS,fedora system.

The bt tree for this bug is:
Thread 6 "CPU 0/KVM" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7c817be006c0 (LWP 1269146)]
kvm_virtio_pci_vq_vector_use () at ../qemu-9.0.0/hw/virtio/virtio-pci.c:817
817	    if (irqfd->users == 0) {
(gdb) thread apply all bt
...
Thread 6 (Thread 0x7c817be006c0 (LWP 1269146) "CPU 0/KVM"):
0  kvm_virtio_pci_vq_vector_use () at ../qemu-9.0.0/hw/virtio/virtio-pci.c:817
1  kvm_virtio_pci_vector_use_one () at ../qemu-9.0.0/hw/virtio/virtio-pci.c:893
2  0x00005983657045e2 in memory_region_write_accessor () at ../qemu-9.0.0/system/memory.c:497
3  0x0000598365704ba6 in access_with_adjusted_size () at ../qemu-9.0.0/system/memory.c:573
4  0x0000598365705059 in memory_region_dispatch_write () at ../qemu-9.0.0/system/memory.c:1528
5  0x00005983659b8e1f in flatview_write_continue_step.isra.0 () at ../qemu-9.0.0/system/physmem.c:2713
6  0x000059836570ba7d in flatview_write_continue () at ../qemu-9.0.0/system/physmem.c:2743
7  flatview_write () at ../qemu-9.0.0/system/physmem.c:2774
8  0x000059836570bb76 in address_space_write () at ../qemu-9.0.0/system/physmem.c:2894
9  0x0000598365763afe in address_space_rw () at ../qemu-9.0.0/system/physmem.c:2904
10 kvm_cpu_exec () at ../qemu-9.0.0/accel/kvm/kvm-all.c:2917
11 0x000059836576656e in kvm_vcpu_thread_fn () at ../qemu-9.0.0/accel/kvm/kvm-accel-ops.c:50
12 0x0000598365926ca8 in qemu_thread_start () at ../qemu-9.0.0/util/qemu-thread-posix.c:541
13 0x00007c8185bcd1cf in ??? () at /usr/lib/libc.so.6
14 0x00007c8185c4e504 in clone () at /usr/lib/libc.so.6

Fixes: 2ce6cff94d ("virtio-pci: fix use of a released vector")
Cc: qemu-stable@nongnu.org

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 hw/virtio/virtio-pci.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index b1d02f4b3d..a7faee5b33 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1431,6 +1431,7 @@ static void virtio_pci_set_vector(VirtIODevice *vdev,
 {
     bool kvm_irqfd = (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) &&
         msix_enabled(&proxy->pci_dev) && kvm_msi_via_irqfd_enabled();
+    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
 
     if (new_vector == old_vector) {
         return;
@@ -1441,7 +1442,8 @@ static void virtio_pci_set_vector(VirtIODevice *vdev,
      * set, we need to release the old vector and set up the new one.
      * Otherwise just need to set the new vector on the device.
      */
-    if (kvm_irqfd && old_vector != VIRTIO_NO_VECTOR) {
+    if (kvm_irqfd && old_vector != VIRTIO_NO_VECTOR &&
+        vdev->use_guest_notifier_mask && k->guest_notifier_mask) {
         kvm_virtio_pci_vector_release_one(proxy, queue_no);
     }
     /* Set the new vector on the device. */
@@ -1451,7 +1453,8 @@ static void virtio_pci_set_vector(VirtIODevice *vdev,
         virtio_queue_set_vector(vdev, queue_no, new_vector);
     }
     /* If the new vector changed need to set it up. */
-    if (kvm_irqfd && new_vector != VIRTIO_NO_VECTOR) {
+    if (kvm_irqfd && new_vector != VIRTIO_NO_VECTOR &&
+        vdev->use_guest_notifier_mask && k->guest_notifier_mask) {
         kvm_virtio_pci_vector_use_one(proxy, queue_no);
     }
 }
-- 
2.43.0
Re: [PATCH] virtio-pci: Fix the use of an uninitialized irqfd.
Posted by Fiona Ebner 5 months, 4 weeks ago 
Hi,

Am 22.05.24 um 07:10 schrieb Cindy Lu:
> The crash was reported in MAC OS and NixOS, here is the link for this bug
> https://gitlab.com/qemu-project/qemu/-/issues/2334
> https://gitlab.com/qemu-project/qemu/-/issues/2321
> 
> The root cause is that the function virtio_pci_set_guest_notifiers() only
> initializes the irqfd when the use_guest_notifier_mask and guest_notifier_mask
> are set.

Sorry, I'm just trying to understand the fix and I'm probably missing
something, but in virtio_pci_set_guest_notifiers() there is:

>     bool with_irqfd = msix_enabled(&proxy->pci_dev) &&
>         kvm_msi_via_irqfd_enabled();

and then:

>     if ((with_irqfd ||
>          (vdev->use_guest_notifier_mask && k->guest_notifier_mask)) &&
>         assign) {
>         if (with_irqfd) {
>             proxy->vector_irqfd =
>                 g_malloc0(sizeof(*proxy->vector_irqfd) *
>                           msix_nr_vectors_allocated(&proxy->pci_dev));
>             r = kvm_virtio_pci_vector_vq_use(proxy, nvqs);

Meaning proxy->vector_irqfd is allocated when with_irqfd is true (even
if vdev->use_guest_notifier_mask && k->guest_notifier_mask is false).

> However, this check is missing in virtio_pci_set_vector().
> So the fix is to add this check.
> 
> This fix is verified in vyatta,MacOS,NixOS,fedora system.
> 
> The bt tree for this bug is:
> Thread 6 "CPU 0/KVM" received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7c817be006c0 (LWP 1269146)]
> kvm_virtio_pci_vq_vector_use () at ../qemu-9.0.0/hw/virtio/virtio-pci.c:817
> 817	    if (irqfd->users == 0) {

The crash happens because the irqfd is NULL/invalid here, right?

proxy->vector_irqfd = NULL happens when virtio_pci_set_guest_notifiers()
is called with assign=false or for an unsuccessful call to
virtio_pci_set_guest_notifiers() with assign=true.

AFAIU, the issue is that virtio_pci_set_vector() is called between a
call to virtio_pci_set_guest_notifiers() with assign=false and a
successful virtio_pci_set_guest_notifiers() with assign=true (or before
the first such call).

So I'm trying to understand why adding the check for
vdev->use_guest_notifier_mask && k->guest_notifier_mask is sufficient to
fix the issue. Thanks!

Best Regards,
Fiona
Re: [PATCH] virtio-pci: Fix the use of an uninitialized irqfd.
Posted by Cindy Lu 5 months, 3 weeks ago 
On Wed, May 29, 2024 at 9:54 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>
> Hi,
>
> Am 22.05.24 um 07:10 schrieb Cindy Lu:
> > The crash was reported in MAC OS and NixOS, here is the link for this bug
> > https://gitlab.com/qemu-project/qemu/-/issues/2334
> > https://gitlab.com/qemu-project/qemu/-/issues/2321
> >
> > The root cause is that the function virtio_pci_set_guest_notifiers() only
> > initializes the irqfd when the use_guest_notifier_mask and guest_notifier_mask
> > are set.
>
> Sorry, I'm just trying to understand the fix and I'm probably missing
> something, but in virtio_pci_set_guest_notifiers() there is:
>
> >     bool with_irqfd = msix_enabled(&proxy->pci_dev) &&
> >         kvm_msi_via_irqfd_enabled();
>
> and then:
>
> >     if ((with_irqfd ||
> >          (vdev->use_guest_notifier_mask && k->guest_notifier_mask)) &&
> >         assign) {
> >         if (with_irqfd) {
> >             proxy->vector_irqfd =
> >                 g_malloc0(sizeof(*proxy->vector_irqfd) *
> >                           msix_nr_vectors_allocated(&proxy->pci_dev));
> >             r = kvm_virtio_pci_vector_vq_use(proxy, nvqs);
>
> Meaning proxy->vector_irqfd is allocated when with_irqfd is true (even
> if vdev->use_guest_notifier_mask && k->guest_notifier_mask is false).
>
> > However, this check is missing in virtio_pci_set_vector().
> > So the fix is to add this check.
> >
> > This fix is verified in vyatta,MacOS,NixOS,fedora system.
> >
> > The bt tree for this bug is:
> > Thread 6 "CPU 0/KVM" received signal SIGSEGV, Segmentation fault.
> > [Switching to Thread 0x7c817be006c0 (LWP 1269146)]
> > kvm_virtio_pci_vq_vector_use () at ../qemu-9.0.0/hw/virtio/virtio-pci.c:817
> > 817       if (irqfd->users == 0) {
>
> The crash happens because the irqfd is NULL/invalid here, right?
>
> proxy->vector_irqfd = NULL happens when virtio_pci_set_guest_notifiers()
> is called with assign=false or for an unsuccessful call to
> virtio_pci_set_guest_notifiers() with assign=true.
>
> AFAIU, the issue is that virtio_pci_set_vector() is called between a
> call to virtio_pci_set_guest_notifiers() with assign=false and a
> successful virtio_pci_set_guest_notifiers() with assign=true (or before
> the first such call).
>
> So I'm trying to understand why adding the check for
> vdev->use_guest_notifier_mask && k->guest_notifier_mask is sufficient to
> fix the issue. Thanks!
>
> Best Regards,
> Fiona
>
Thanks, Fiona. You are correct. I have rechecked the call trace, and
it seems that the virtio_pci_set_guest_notifiers was not called at all
for this device. I mistook the calling for another device with the
calling for this one.
I will send a new version
Thanks
Cindy
>
Re: [PATCH] virtio-pci: Fix the use of an uninitialized irqfd.
Posted by Jason Wang 6 months ago 
On Wed, May 22, 2024 at 1:10 PM Cindy Lu <lulu@redhat.com> wrote:
>
> The crash was reported in MAC OS and NixOS, here is the link for this bug
> https://gitlab.com/qemu-project/qemu/-/issues/2334
> https://gitlab.com/qemu-project/qemu/-/issues/2321
>
> The root cause is that the function virtio_pci_set_guest_notifiers() only
> initializes the irqfd when the use_guest_notifier_mask and guest_notifier_mask
> are set.
> However, this check is missing in virtio_pci_set_vector().
> So the fix is to add this check.
>
> This fix is verified in vyatta,MacOS,NixOS,fedora system.
>
> The bt tree for this bug is:
> Thread 6 "CPU 0/KVM" received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7c817be006c0 (LWP 1269146)]
> kvm_virtio_pci_vq_vector_use () at ../qemu-9.0.0/hw/virtio/virtio-pci.c:817
> 817         if (irqfd->users == 0) {
> (gdb) thread apply all bt
> ...
> Thread 6 (Thread 0x7c817be006c0 (LWP 1269146) "CPU 0/KVM"):
> 0  kvm_virtio_pci_vq_vector_use () at ../qemu-9.0.0/hw/virtio/virtio-pci.c:817
> 1  kvm_virtio_pci_vector_use_one () at ../qemu-9.0.0/hw/virtio/virtio-pci.c:893
> 2  0x00005983657045e2 in memory_region_write_accessor () at ../qemu-9.0.0/system/memory.c:497
> 3  0x0000598365704ba6 in access_with_adjusted_size () at ../qemu-9.0.0/system/memory.c:573
> 4  0x0000598365705059 in memory_region_dispatch_write () at ../qemu-9.0.0/system/memory.c:1528
> 5  0x00005983659b8e1f in flatview_write_continue_step.isra.0 () at ../qemu-9.0.0/system/physmem.c:2713
> 6  0x000059836570ba7d in flatview_write_continue () at ../qemu-9.0.0/system/physmem.c:2743
> 7  flatview_write () at ../qemu-9.0.0/system/physmem.c:2774
> 8  0x000059836570bb76 in address_space_write () at ../qemu-9.0.0/system/physmem.c:2894
> 9  0x0000598365763afe in address_space_rw () at ../qemu-9.0.0/system/physmem.c:2904
> 10 kvm_cpu_exec () at ../qemu-9.0.0/accel/kvm/kvm-all.c:2917
> 11 0x000059836576656e in kvm_vcpu_thread_fn () at ../qemu-9.0.0/accel/kvm/kvm-accel-ops.c:50
> 12 0x0000598365926ca8 in qemu_thread_start () at ../qemu-9.0.0/util/qemu-thread-posix.c:541
> 13 0x00007c8185bcd1cf in ??? () at /usr/lib/libc.so.6
> 14 0x00007c8185c4e504 in clone () at /usr/lib/libc.so.6
>
> Fixes: 2ce6cff94d ("virtio-pci: fix use of a released vector")
> Cc: qemu-stable@nongnu.org
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

> ---
>  hw/virtio/virtio-pci.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index b1d02f4b3d..a7faee5b33 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1431,6 +1431,7 @@ static void virtio_pci_set_vector(VirtIODevice *vdev,
>  {
>      bool kvm_irqfd = (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) &&
>          msix_enabled(&proxy->pci_dev) && kvm_msi_via_irqfd_enabled();
> +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>
>      if (new_vector == old_vector) {
>          return;
> @@ -1441,7 +1442,8 @@ static void virtio_pci_set_vector(VirtIODevice *vdev,
>       * set, we need to release the old vector and set up the new one.
>       * Otherwise just need to set the new vector on the device.
>       */
> -    if (kvm_irqfd && old_vector != VIRTIO_NO_VECTOR) {
> +    if (kvm_irqfd && old_vector != VIRTIO_NO_VECTOR &&
> +        vdev->use_guest_notifier_mask && k->guest_notifier_mask) {
>          kvm_virtio_pci_vector_release_one(proxy, queue_no);
>      }
>      /* Set the new vector on the device. */
> @@ -1451,7 +1453,8 @@ static void virtio_pci_set_vector(VirtIODevice *vdev,
>          virtio_queue_set_vector(vdev, queue_no, new_vector);
>      }
>      /* If the new vector changed need to set it up. */
> -    if (kvm_irqfd && new_vector != VIRTIO_NO_VECTOR) {
> +    if (kvm_irqfd && new_vector != VIRTIO_NO_VECTOR &&
> +        vdev->use_guest_notifier_mask && k->guest_notifier_mask) {
>          kvm_virtio_pci_vector_use_one(proxy, queue_no);
>      }
>  }
> --
> 2.43.0
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.