Re: [PATCH v2] hw/usb/hcd-xhci: Turn guest-triggerable abort() into qemu_log_mask()

On 10/07/2026 17.35, Peter Maydell wrote:
> On Fri, 10 Jul 2026 at 15:44, Thomas Huth <[email protected]> wrote:
>>
>> From: Thomas Huth <[email protected]>
>>
>> The FIXME macros in xhci_alloc_device_streams() can be triggered
>> by a (malicious) guest. Since the macro also contains an abort()
>> statement, this terminates QEMU. Turn the FIXME statements into
>> a qemu_log_mask() instead to avoid that a guest can shoot itself
>> this way.
>>
>> Reported-by: Feifan Qian <[email protected]>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3784
>> Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
>> Signed-off-by: Thomas Huth <[email protected]>
>> ---
>>   hw/usb/hcd-xhci.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
>> index 2cdab3ba0e4..002f801348f 100644
>> --- a/hw/usb/hcd-xhci.c
>> +++ b/hw/usb/hcd-xhci.c
>> @@ -965,11 +965,13 @@ static TRBCCode xhci_alloc_device_streams(XHCIState *xhci, unsigned int slotid,
>>            * together and make an usb_device_alloc_streams call per group.
>>            */
>>           if (epctxs[i]->nr_pstreams != req_nr_streams) {
>> -            FIXME("guest streams config not identical for all eps");
>> +            qemu_log_mask(LOG_GUEST_ERROR,
>> +                          "guest streams config not identical for all eps\n");
>>               return CC_RESOURCE_ERROR;
>>           }
>>           if (eps[i]->max_streams != dev_max_streams) {
>> -            FIXME("device streams config not identical for all eps");
>> +            qemu_log_mask(LOG_GUEST_ERROR,
>> +                          "device streams config not identical for all eps\n");
>>               return CC_RESOURCE_ERROR;
> 
> I get the impression from the comment that these cases are not
> guest errors, but unimplemented QEMU support for a corner case
> of the spec, so should be LOG_UNIMP. I had a quick look through
> the XHCI spec, and it seems to define the streams handling
> purely as a per-endpoint thing, making it valid for a device
> to have multiple endpoints with different max streams settings.
> It's just that it doesn't happen in practice so we haven't
> implemented the "group identical endpoints and call
> usb_device_alloc_streams for each group" solution the comment
> sketches out.
> 
> Incidentally, there is just one remaining use of the FIXME macro
> in this file: the "unhandled USB_RET_*" default case in
> xhci_try_complete_packet(). I think we could (as a separate patch)
> replace that with g_assert_not_reached(), because the function
> handles all the USB_RET_* values except for USB_RET_ADD_TO_QUEUE and
> USB_RET_REMOVE_FROM_QUEUE, which are both internal return
> values for when an async packet needs to be queued or dequeued,
> and which (I believe) shouldn't still be the status by the time we
> get to this function.
> 
> There's also one raw printf that should be a LOG_UNIMP:
>          fprintf(stderr, "xhci: FIXME: secondary streams not implemented yet");
Thanks, Peter, these are good suggestions, I'll send a v3 and patches for 
the other things that you've spotted.

  Thomas