| Message ID | 20240404191339.5688-4-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/virtio: Protect from more DMA re-entrancy bugs | expand |
On 4/4/24 21:13, Philippe Mathieu-Daudé wrote: > Replace qemu_bh_new_guarded() by virtio_bh_new_guarded() > so the bus and device use the same guard. Otherwise the > DMA-reentrancy protection can be bypassed. > > Cc: qemu-stable@nongnu.org > Suggested-by: Alexander Bulekov <alxndr@bu.edu> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/char/virtio-serial-bus.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c > index 016aba6374..cd0e3a11f7 100644 > --- a/hw/char/virtio-serial-bus.c > +++ b/hw/char/virtio-serial-bus.c > @@ -985,8 +985,7 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp) > return; > } > > - port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port, > - &dev->mem_reentrancy_guard); > + port->bh = virtio_bh_new_guarded(vdev, flush_queued_data_bh, port); Missing: -- >8 -- - port->bh = virtio_bh_new_guarded(vdev, flush_queued_data_bh, port); + port->bh = virtio_bh_new_guarded(VIRTIO_DEVICE(dev), + flush_queued_data_bh, port); --- > port->elem = NULL; > } >
On Mon, Apr 08, 2024 at 09:14:39AM +0200, Philippe Mathieu-Daudé wrote: > On 4/4/24 21:13, Philippe Mathieu-Daudé wrote: > > Replace qemu_bh_new_guarded() by virtio_bh_new_guarded() > > so the bus and device use the same guard. Otherwise the > > DMA-reentrancy protection can be bypassed. > > > > Cc: qemu-stable@nongnu.org > > Suggested-by: Alexander Bulekov <alxndr@bu.edu> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > --- > > hw/char/virtio-serial-bus.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c > > index 016aba6374..cd0e3a11f7 100644 > > --- a/hw/char/virtio-serial-bus.c > > +++ b/hw/char/virtio-serial-bus.c > > @@ -985,8 +985,7 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp) > > return; > > } > > - port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port, > > - &dev->mem_reentrancy_guard); > > + port->bh = virtio_bh_new_guarded(vdev, flush_queued_data_bh, port); > > Missing: > -- >8 -- > - port->bh = virtio_bh_new_guarded(vdev, flush_queued_data_bh, port); > + port->bh = virtio_bh_new_guarded(VIRTIO_DEVICE(dev), > + flush_queued_data_bh, port); > --- I don't get it. vdev is already the correct type. Why do you need VIRTIO_DEVICE here? > > port->elem = NULL; > > }
On 8/4/24 12:08, Michael S. Tsirkin wrote: > On Mon, Apr 08, 2024 at 09:14:39AM +0200, Philippe Mathieu-Daudé wrote: >> On 4/4/24 21:13, Philippe Mathieu-Daudé wrote: >>> Replace qemu_bh_new_guarded() by virtio_bh_new_guarded() >>> so the bus and device use the same guard. Otherwise the >>> DMA-reentrancy protection can be bypassed. >>> >>> Cc: qemu-stable@nongnu.org >>> Suggested-by: Alexander Bulekov <alxndr@bu.edu> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> hw/char/virtio-serial-bus.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c >>> index 016aba6374..cd0e3a11f7 100644 >>> --- a/hw/char/virtio-serial-bus.c >>> +++ b/hw/char/virtio-serial-bus.c >>> @@ -985,8 +985,7 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp) >>> return; >>> } >>> - port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port, >>> - &dev->mem_reentrancy_guard); >>> + port->bh = virtio_bh_new_guarded(vdev, flush_queued_data_bh, port); >> >> Missing: >> -- >8 -- >> - port->bh = virtio_bh_new_guarded(vdev, flush_queued_data_bh, port); >> + port->bh = virtio_bh_new_guarded(VIRTIO_DEVICE(dev), >> + flush_queued_data_bh, port); >> --- > > I don't get it. vdev is already the correct type. Why do you need > VIRTIO_DEVICE here? This function doesn't declare vdev. > >>> port->elem = NULL; >>> } >
On Mon, Apr 08, 2024 at 01:04:11PM +0200, Philippe Mathieu-Daudé wrote: > On 8/4/24 12:08, Michael S. Tsirkin wrote: > > On Mon, Apr 08, 2024 at 09:14:39AM +0200, Philippe Mathieu-Daudé wrote: > > > On 4/4/24 21:13, Philippe Mathieu-Daudé wrote: > > > > Replace qemu_bh_new_guarded() by virtio_bh_new_guarded() > > > > so the bus and device use the same guard. Otherwise the > > > > DMA-reentrancy protection can be bypassed. > > > > > > > > Cc: qemu-stable@nongnu.org > > > > Suggested-by: Alexander Bulekov <alxndr@bu.edu> > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > > > --- > > > > hw/char/virtio-serial-bus.c | 3 +-- > > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > > > diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c > > > > index 016aba6374..cd0e3a11f7 100644 > > > > --- a/hw/char/virtio-serial-bus.c > > > > +++ b/hw/char/virtio-serial-bus.c > > > > @@ -985,8 +985,7 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp) > > > > return; > > > > } > > > > - port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port, > > > > - &dev->mem_reentrancy_guard); > > > > + port->bh = virtio_bh_new_guarded(vdev, flush_queued_data_bh, port); > > > > > > Missing: > > > -- >8 -- > > > - port->bh = virtio_bh_new_guarded(vdev, flush_queued_data_bh, port); > > > + port->bh = virtio_bh_new_guarded(VIRTIO_DEVICE(dev), > > > + flush_queued_data_bh, port); > > > --- > > > > I don't get it. vdev is already the correct type. Why do you need > > VIRTIO_DEVICE here? > > This function doesn't declare vdev. > > > > > > > port->elem = NULL; > > > > } > > But it seems clear it wasn't really tested, right? Philipe here's my ack: Acked-by: Michael S. Tsirkin <mst@redhat.com> Feel free to merge these after testing.
On 8/4/24 17:20, Michael S. Tsirkin wrote: > On Mon, Apr 08, 2024 at 01:04:11PM +0200, Philippe Mathieu-Daudé wrote: >> On 8/4/24 12:08, Michael S. Tsirkin wrote: >>> On Mon, Apr 08, 2024 at 09:14:39AM +0200, Philippe Mathieu-Daudé wrote: >>>> On 4/4/24 21:13, Philippe Mathieu-Daudé wrote: >>>>> Replace qemu_bh_new_guarded() by virtio_bh_new_guarded() >>>>> so the bus and device use the same guard. Otherwise the >>>>> DMA-reentrancy protection can be bypassed. >>>>> >>>>> Cc: qemu-stable@nongnu.org >>>>> Suggested-by: Alexander Bulekov <alxndr@bu.edu> >>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>>> --- >>>>> hw/char/virtio-serial-bus.c | 3 +-- >>>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>>> >>>>> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c >>>>> index 016aba6374..cd0e3a11f7 100644 >>>>> --- a/hw/char/virtio-serial-bus.c >>>>> +++ b/hw/char/virtio-serial-bus.c >>>>> @@ -985,8 +985,7 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp) >>>>> return; >>>>> } >>>>> - port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port, >>>>> - &dev->mem_reentrancy_guard); >>>>> + port->bh = virtio_bh_new_guarded(vdev, flush_queued_data_bh, port); >>>> >>>> Missing: >>>> -- >8 -- >>>> - port->bh = virtio_bh_new_guarded(vdev, flush_queued_data_bh, port); >>>> + port->bh = virtio_bh_new_guarded(VIRTIO_DEVICE(dev), >>>> + flush_queued_data_bh, port); >>>> --- >>> >>> I don't get it. vdev is already the correct type. Why do you need >>> VIRTIO_DEVICE here? >> >> This function doesn't declare vdev. >> >>> >>>>> port->elem = NULL; >>>>> } >>> > > > > But it seems clear it wasn't really tested, right? Indeed, I only tested virtio-gpu, then added the other ones Alexander suggested. I don't have virtio-specific tests, I rely on the GitLab CI ones. Hope that's enough. > Philipe here's my ack: > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > Feel free to merge these after testing. Sure, thanks! Phil.
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c index 016aba6374..cd0e3a11f7 100644 --- a/hw/char/virtio-serial-bus.c +++ b/hw/char/virtio-serial-bus.c @@ -985,8 +985,7 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp) return; } - port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port, - &dev->mem_reentrancy_guard); + port->bh = virtio_bh_new_guarded(vdev, flush_queued_data_bh, port); port->elem = NULL; }
Replace qemu_bh_new_guarded() by virtio_bh_new_guarded() so the bus and device use the same guard. Otherwise the DMA-reentrancy protection can be bypassed. Cc: qemu-stable@nongnu.org Suggested-by: Alexander Bulekov <alxndr@bu.edu> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/char/virtio-serial-bus.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)