bhyve: Fix handling of the xhci event ring
ClosedPublic
Actions

Authored by markj on Mon, Oct 12, 6:39 PM.

Details

Reviewers

aokblast
kevans

Group Reviewers

bhyve

Commits

rG32b2ee469ee2: bhyve: Fix handling of the xhci event ring
rGc9f3526248da: bhyve: Fix handling of the xhci event ring
rG409554009e48: bhyve: Fix handling of the xhci event ring
rG9596b25e3412: bhyve: Fix handling of the xhci event ring
rG719b159a1985: bhyve: Fix handling of the xhci event ring
rG1d14ab14dd72: bhyve: Fix handling of the xhci event ring

Summary

rts->erst_p and rts->erstba_p are pointers to guest-controlled memory
which describe the event rings and pending event array, respectively.
The device model was not validating the size nor saving the value.
As a result, the bounds checks in pci_xhci_insert_event() were
susceptible to TOCTOU races.

Fix pci_xhci_rtsregs_write() to save the ring size in a softc field.
Validate that the size isn't larger than the maximum allowed by the XHCI
specification (64KB, or 4096 TRBs).

When processing TRBs, make sure we don't load fields more than once.
Make sure that pci_xhci_trb_next() doesn't return a pointer to host
memory. When handling ERDP writes, make sure that the guest-supplied
address is actually a pointer into the TRB array, and ignore the write
if not.

Reported by: Brian Delgado from Intel using Anthropic Opus 4.8

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 78095
Build 74978: arc lint + arc unit

Event Timeline

markj created this revision.Mon, Oct 12, 6:39 PM

Herald added subscribers: bcran, rgrimes, imp. · View Herald TranscriptMon, Oct 12, 6:39 PM

markj requested review of this revision.Mon, Oct 12, 6:39 PM

Harbormaster completed remote builds in B78095: Diff 189656.Mon, Oct 12, 6:39 PM

markj added a child revision: D60678: bhyve: Improve HDA stream descriptor validation.Mon, Oct 12, 6:39 PM

markj added reviewers: aokblast, kevans.Mon, Oct 12, 6:41 PM

For context: keelOS runs Windows, macOS and Linux guests on bhyve with the tablet and keyboard on this model, plus our own remote-USB path through it, so we keep a patched copy of pci_xhci.c and have been through the same exercise of not trusting what the guest hands us. The change looks right to me; a few notes from what actually happened to us with real guests, in case they are useful here:

The ring fill count. With er_events_cnt == er_size - 1 the code decides "full" from the enqueue slot's cycle bit, which in the normal case is the previous lap's and never matches event_pcs; so the event goes into the last free slot without er_events_cnt++, the count now says one less than the ring holds, and the next event overwrites a TRB the guest has not consumed. In the Event Ring Full Error branch er_enq_idx wraps to 0 without toggling event_pcs. We saw this as Windows 10 stopping the endpoint and resetting the controller under fast mouse input (together with the unlocked pci_xhci_dev_intr() writer, which D60432 is about). What worked for us is the 4.9.4 rule as written: keep one slot, cnt == size - 1 -> post the Event Ring Full Error there (with the cycle toggle on wrap), cnt++; otherwise post the event, cnt++. Not part of this change -- happy to send it as a follow-up if you prefer that.
Guest-reachable asserts and NULL mappings: in our experience every pointer this model takes from a context or a TRB needs the "unmapped" case handled, because guests do hit them during their own controller resets (we had assert(dev != NULL) in Address Device fire from a guest). The inline notes below are the spots where the new code can still fault on a NULL from XHCI_GADDR().

(inline notes attached)

wanpengqian_gmail.com added inline comments.Wed, Oct 14, 12:05 AM

usr.sbin/bhyve/pci_xhci.c
756	With `erstba_p` now NULL after a bad ERSTBA, this line faults on the command-ring path (`pci_xhci_complete_commands()` -> here); only `pci_xhci_dev_intr()` checks `erstba_p` before inserting. `erdp_idx` only feeds the DPRINTF, so it could go, and an `erst_p == NULL -> return` at the top of `insert_event()` covers the other callers.
1787	On the first pass `trb` is `pci_xhci_device_doorbell()`'s `XHCI_GADDR(ringaddr)`, which can be NULL as well (a ring the guest pointed outside its memory); the `if (trb == NULL) break;` at the bottom of the loop doesn't reach it.
1840	Same family, not new here: `XHCI_GADDR()` only checks that the first page is mapped (`XHCI_GADDR_SIZE()` runs to the end of the page), while the block length goes up to 0x1FFFF. A buffer ending past the end of a guest memory segment gives the device a host pointer with a too-long `blen`. The emulated mouse and keyboard copy only their small descriptors and reports, so it is bounded there; with real devices behind it (D52166's passthrough, or our remote-USB path -- we have the same gap and will close it on our side) it becomes an out-of-bounds copy. Mapping with the TRB's length (`paddr_guest2host(ctx, qwTrb0, len)`) and turning NULL into a TRB error, instead of handing it to `usb_data_xfer_append()`, covers both.
2127	`XHCI_GADDR()` returns NULL when the ERSTBA the guest wrote is outside guest memory, and this load then faults bhyve. Setting `erstba_p = erst_p = NULL, er_size = 0` and returning before reading the table keeps the "not configured" state consistent.

In D60677#1392333, @wanpengqian_gmail.com wrote:

For context: keelOS runs Windows, macOS and Linux guests on bhyve with the tablet and keyboard on this model, plus our own remote-USB path through it, so we keep a patched copy of pci_xhci.c and have been through the same exercise of not trusting what the guest hands us. The change looks right to me; a few notes from what actually happened to us with real guests, in case they are useful here:

The ring fill count. With er_events_cnt == er_size - 1 the code decides "full" from the enqueue slot's cycle bit, which in the normal case is the previous lap's and never matches event_pcs; so the event goes into the last free slot without er_events_cnt++, the count now says one less than the ring holds, and the next event overwrites a TRB the guest has not consumed. In the Event Ring Full Error branch er_enq_idx wraps to 0 without toggling event_pcs. We saw this as Windows 10 stopping the endpoint and resetting the controller under fast mouse input (together with the unlocked pci_xhci_dev_intr() writer, which D60432 is about). What worked for us is the 4.9.4 rule as written: keep one slot, cnt == size - 1 -> post the Event Ring Full Error there (with the cycle toggle on wrap), cnt++; otherwise post the event, cnt++. Not part of this change -- happy to send it as a follow-up if you prefer that.

Please, that'd be great. I will go through your other patches soon, I am just behind on reviews.

Here I am trying to fix some fairly specific guest-triggerable races, anything else should be handled in a separate patch.

Guest-reachable asserts and NULL mappings: in our experience every pointer this model takes from a context or a TRB needs the "unmapped" case handled, because guests do hit them during their own controller resets (we had assert(dev != NULL) in Address Device fire from a guest). The inline notes below are the spots where the new code can still fault on a NULL from XHCI_GADDR().

Right. I deliberately did not fix them in this patch, since there are quite a few instances of this type of bug both here and in other device models, and at worst they allow a guest to deliberately kill a bhyve instance.

I agree that these are bugs and need to be fixed; as a goal we should ensure that a guest cannot trigger null pointer dereferences or assertion failures. It can be hard to decide what to do in such cases though.