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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.