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

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
-- PMM