On 20/07/2026 13.42, Philippe Mathieu-Daudé wrote:
On 20/7/26 10:37, Thomas Huth wrote:
On 24/04/2026 16.54, Marc-André Lureau wrote:
Hi
On Fri, Apr 24, 2026 at 4:37 PM Feifan Qian <[email protected]> wrote:
The xHCI endpoint context dword 0 bits 23:16 ("Interval") are written
by the guest and passed directly as the shift amount in:
epctx->interval = 1 << ((ctx[0] >> 16) & 0xff);
The shift amount can be 0-255. Shifting a 32-bit `int` left by >= 32
is undefined behaviour under C11 §6.5.7p4. With UBSan
(halt_on_error=1) this causes QEMU to abort; with aggressive compiler
optimisations that assume UB is unreachable the result is
unpredictable.
Clamp the exponent to [0, 31] with MIN() before the shift, and use
`1u` (unsigned) to avoid shifting a signed integer. The xHCI
specification defines a maximum meaningful Interval value of 15 for
most endpoint types; clamping to 31 is the minimal safe fix that
preserves the full unsigned 32-bit range for any compliant value.
I think we should clamp it to 15, to comply with the spec.
Otherwise, the patch looks fine.
Some endpoint types seem also to allow up to 18, so I'm going to pick up
Which ones? I agree with Marc-André they are using illegal values.
According to the XHCI spec, the "FS Isoch" endpoints are using values up to 18.
this patch with 31 replaced by 18.
If we don't want to report the illegal value as GUEST_ERROR, I'd
just clamp to the spec and call it a day:
epctx->interval = 1u << extract32(ctx[0], 16, 4);
The spec says that the interval field is 8 bit, so this is certainly also
not right.
I'd say let's fix now the undefined behavior with the current patch, and if
someone feels like implementing the spec more precisely, this can be done
with a separate patch later.
Thomas
