Re: [PATCH v3] hw/s390x: Fix a possible crash with passed-through virtio devices

[Thomas Huth <thuth@redhat.com>]

On 19/11/2025 11.02, Cédric Le Goater wrote:
> On 11/18/25 18:40, Thomas Huth wrote:
>> From: Thomas Huth <thuth@redhat.com>
>>
>> Consider the following nested setup: An L1 host uses some virtio device
>> (e.g. virtio-keyboard) for the L2 guest, and this L2 guest passes this
>> device through to the L3 guest. Since the L3 guest sees a virtio device,
>> it might send virtio notifications to the QEMU in L2 for that device.
>> But since the QEMU in L2 defined this device as vfio-ccw, the function
>> handle_virtio_ccw_notify() cannot handle this and crashes: It calls
>> virtio_ccw_get_vdev() that casts sch->driver_data into a VirtioCcwDevice,
>> but since "sch" belongs to a vfio-ccw device, that driver_data rather
>> points to a CcwDevice instead. So as soon as QEMU tries to use some
>> VirtioCcwDevice specific data from that device, we've lost.
>>
>> We must not take virtio notifications for such devices. Thus fix the
>> issue by adding a check to the handle_virtio_ccw_notify() handler to
>> refuse all devices that are not our own virtio devices. Like in the
>> other branches that detect wrong settings, we return -EINVAL from the
>> function, which will later be placed in GPR2 to inform the guest about
>> the error.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   v3: Print the subchannel number to ease debugging
>>
>>   hw/s390x/s390-hypercall.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/hw/s390x/s390-hypercall.c b/hw/s390x/s390-hypercall.c
>> index ac1b08b2cd5..508dd97ca0d 100644
>> --- a/hw/s390x/s390-hypercall.c
>> +++ b/hw/s390x/s390-hypercall.c
>> @@ -10,6 +10,7 @@
>>    */
>>   #include "qemu/osdep.h"
>> +#include "qemu/error-report.h"
>>   #include "cpu.h"
>>   #include "hw/s390x/s390-virtio-ccw.h"
>>   #include "hw/s390x/s390-hypercall.h"
>> @@ -42,6 +43,19 @@ static int handle_virtio_ccw_notify(uint64_t subch_id, 
>> uint64_t data)
>>       if (!sch || !css_subch_visible(sch)) {
>>           return -EINVAL;
>>       }
>> +    if (sch->id.cu_type != VIRTIO_CCW_CU_TYPE) {
>> +        /*
>> +         * This might happen in nested setups: If the L1 host defined the
>> +         * L2 guest with a virtio device (e.g. virtio-keyboard), and the
>> +         * L2 guest passes this device through to the L3 guest, the L3 guest
>> +         * might send virtio notifications to the QEMU in L2 for that 
>> device.
>> +         * But since the QEMU in L2 defined this device as vfio-ccw, it's 
>> not
>> +         * a VirtIODevice that we can handle here!
>> +         */
>> +        warn_report_once("Got virtio notification for unsupported device "
>> +                         "on subchannel %02x.%1x.%04x!", cssid, ssid, 
>> schid);
>> +        return -EINVAL;
>> +    }
>>       vdev = virtio_ccw_get_vdev(sch);
> 
> While at it, it would be good to test 'vdev' and return -EINVAL as in
> virtio_ccw_set_vqs().
> 
> In virtio-ccw.c, this needs some care I think :
> 
>     static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>     {
>         ...
>         VirtioCcwDevice *dev = sch->driver_data;
>         VirtIODevice *vdev = virtio_ccw_get_vdev(sch);
>         ...
>         if (!dev) {                <-- vdev ?
>            return -EINVAL;
>         }

I wonder whether this can happen at all? We check for a a valid virtio-ccw 
device
now, and all virtio-ccw devices should have their driver_data set up in their
realize function, so I think we should always get a valid pointer here and 
ifit's NULL, there must be a bug somewhere. So maybe an assert() would be the
better idea? Or just let it crash by dereferencing the NULL pointer...?

  Thomas