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

qemu-devel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From:	Jason Wang
Subject:	Re: [PATCH v5] virtio-pci: Fix the use of an uninitialized irqfd
Date:	Tue, 23 Jul 2024 14:06:05 +0800

On Mon, Jul 22, 2024 at 4:42 PM Cindy Lu <lulu@redhat.com> wrote:
>
>
>
> On Mon, 22 Jul 2024 at 15:24, Jason Wang <jasowang@redhat.com> wrote:
> >
> > Hi Cindy
> >
> > On Fri, Jul 19, 2024 at 1:25 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
> > >
> > > In this bug, they are using the virtio_input device. The guest notifier 
> > > was
> > > not supported for this device, The function 
> > > virtio_pci_set_guest_notifiers()
> > > was not called, and the vector_irqfd was not initialized.
> > >
> > > So the fix is adding the check for vector_irqfd in 
> > > virtio_pci_get_notifier()
> > >
> > > But The function virtio_pci_get_notifier(),it can also be used in all 
> > > kinds of device.
> > > If the vector_irqfd still didn't initial after the 
> > > VIRTIO_CONFIG_S_DRIVER_OK is set
> > > means this device is not using guest notifier. We can let the check fail 
> > > here
> > >
> > > 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 | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > index 592fdaa10f..dc31a37ec0 100644
> > > --- a/hw/virtio/virtio-pci.c
> > > +++ b/hw/virtio/virtio-pci.c
> > > @@ -860,6 +860,9 @@ static int virtio_pci_get_notifier(VirtIOPCIProxy 
> > > *proxy, int queue_no,
> > >      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> > >      VirtQueue *vq;
> > >
> > > +    if (!proxy->vector_irqfd && vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)
> > > +        return -1;
> > > +
> >
> > Did this mean !proxy->vector_irqfd && !(vdev->status &
> > VIRTIO_CONFIG_S_DRIVER_OK)) is legal?
> >
> > Thanks
> >
> yes, for my test, I didn't meet this kind of environment.

This needs to be checked, we can use qtest to poc this condition if you wish.

But I meant we need to explain why there is a check for DRIVER_OK
here. For example, did the current Qemu guarantee that?

> However, since this function is used widely in many environments, I cannot 
> cover all devices for testing.
>
> I think we can change the patch back to the first version, which is to check 
> the proxy->vector_irqfd in virtio_pci_set_vector(). This will have a lower 
> risk and make more sense
> thanks
> cindy

Thanks

>
>
> On Mon, 22 Jul 2024 at 15:24, Jason Wang <jasowang@redhat.com> wrote:
>>
>> Hi Cindy
>>
>> On Fri, Jul 19, 2024 at 1:25 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
>> >
>> > In this bug, they are using the virtio_input device. The guest notifier was
>> > not supported for this device, The function 
>> > virtio_pci_set_guest_notifiers()
>> > was not called, and the vector_irqfd was not initialized.
>> >
>> > So the fix is adding the check for vector_irqfd in 
>> > virtio_pci_get_notifier()
>> >
>> > But The function virtio_pci_get_notifier(),it can also be used in all 
>> > kinds of device.
>> > If the vector_irqfd still didn't initial after the 
>> > VIRTIO_CONFIG_S_DRIVER_OK is set
>> > means this device is not using guest notifier. We can let the check fail 
>> > here
>> >
>> > 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 | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> > index 592fdaa10f..dc31a37ec0 100644
>> > --- a/hw/virtio/virtio-pci.c
>> > +++ b/hw/virtio/virtio-pci.c
>> > @@ -860,6 +860,9 @@ static int virtio_pci_get_notifier(VirtIOPCIProxy 
>> > *proxy, int queue_no,
>> >      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>> >      VirtQueue *vq;
>> >
>> > +    if (!proxy->vector_irqfd && vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)
>> > +        return -1;
>> > +
>>
>> Did this mean !proxy->vector_irqfd && !(vdev->status &
>> VIRTIO_CONFIG_S_DRIVER_OK)) is legal?
>>
>> Thanks
>>
>> >      if (queue_no == VIRTIO_CONFIG_IRQ_IDX) {
>> >          *n = virtio_config_get_guest_notifier(vdev);
>> >          *vector = vdev->config_vector;
>> > --
>> > 2.45.0
>> >
>>

[Prev in Thread]

Current Thread

[Next in Thread]

[PATCH v5] virtio-pci: Fix the use of an uninitialized irqfd, Cindy Lu, 2024/07/19
- Re: [PATCH v5] virtio-pci: Fix the use of an uninitialized irqfd, Jason Wang, 2024/07/22
  - Re: [PATCH v5] virtio-pci: Fix the use of an uninitialized irqfd, Cindy Lu, 2024/07/22
    - Re: [PATCH v5] virtio-pci: Fix the use of an uninitialized irqfd, Jason Wang <=

Prev by Date: Re: [PATCH 1/1] target/riscv: Remove redundant insn length check for zama16b
Next by Date: Re: [PATCH v6 5/9] vfio/iommufd: Probe and request hwpt dirty tracking capability
Previous by thread: Re: [PATCH v5] virtio-pci: Fix the use of an uninitialized irqfd
Next by thread: [PATCH v6 0/4] docs/interop/firmware.json: scripts/qapi-gen.py compatibility
Index(es):
- Date
- Thread