diff mbox series

[PATCH-for-9.0,3/4] hw/char/virtio-serial-bus: Protect from DMA re-entrancy bugs

Message ID 20240404191339.5688-4-philmd@linaro.org
State Superseded
Headers show
Series hw/virtio: Protect from more DMA re-entrancy bugs | expand

Commit Message

Philippe Mathieu-Daudé April 4, 2024, 7:13 p.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé April 8, 2024, 7:14 a.m. UTC | #1
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;
>   }
>
Michael S. Tsirkin April 8, 2024, 10:08 a.m. UTC | #2
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;
> >   }
Philippe Mathieu-Daudé April 8, 2024, 11:04 a.m. UTC | #3
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;
>>>    }
>
Michael S. Tsirkin April 8, 2024, 3:20 p.m. UTC | #4
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.
Philippe Mathieu-Daudé April 8, 2024, 6:22 p.m. UTC | #5
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 mbox series

Patch

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;
 }