Page MenuHomeFreeBSD

bhyve xhci: Cache the value of MaxPStreams when initializing an endpoint.
ClosedPublic

Authored by jhb on Aug 12 2022, 7:07 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 1 2024, 9:02 PM
Unknown Object (File)
Jan 12 2024, 3:10 AM
Unknown Object (File)
Dec 23 2023, 10:53 AM
Unknown Object (File)
Nov 18 2023, 6:35 PM
Unknown Object (File)
Nov 18 2023, 2:39 AM
Unknown Object (File)
Jul 8 2023, 5:43 AM
Unknown Object (File)
Jul 8 2023, 5:43 AM
Unknown Object (File)
Jul 8 2023, 5:42 AM

Details

Summary

This avoids type confusion where a malicious guest could rewrite the
MaxPStreams field in an endpoint context after the endpoint was
initialized causing the device model to interpret a guest provided
address (stored in ep_ringaddr of the "software" endpoint state) as a
bhyve host process address (ep_sctx_trbs). It also prevents a malicious
guest from triggering overflows of ep_sctx_trbs[] by increasing the
number of streams after the endpoint has been initialized.

Rather than re-reading the MaxPStreams value out of the endpoint context
in guest memory on subsequent operations, cache the value in the software
endpoint state. Possibly the device model should raise errors if the
value of MaxPStreams changes while an endpoint is running. This approach
simply ignores any such changes by the guest.

PR: 264294, 264347
Reported by: Robert Morris <rtm@lcs.mit.edu>
Sponsored by: The FreeBSD Foundation

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 46908
Build 43797: arc lint + arc unit

Event Timeline

jhb requested review of this revision.Aug 12 2022, 7:07 PM
usr.sbin/bhyve/pci_xhci.c
173

It'd be helpful to have a comment indicating that this is a shadow of the max_pstreams field of epctx0.

1201

I don't quite understand this line, shouldn't it be adding streamid * sizeof(*sctx) or something like that?

1269

It looks like this code is supposed to be using sctx instead of devep->ep_sctx[streamid].

usr.sbin/bhyve/pci_xhci.c
1201

Hmm, or we should be casting the result of XHCI_GADDR to the right type, yes.

1269

I bet this then is the other half of the bug you found above (or rather, this makes the bug you found above harmless since the incorrect pointer isn't used). I can clean this up though in a followup commit.

I still think a comment should explain the new field but the change seems ok to me after some quality time with the XHCI spec.

usr.sbin/bhyve/pci_xhci.c
173

(Also note that ep_maxpstreams might be a more accurate name.)

1269

Thanks. The other half will be caught eventually when we bump WARNS (arithmetic on a void pointer), but this should be cleaned up too.

BTW, I think the other half of the bug is not quite harmless since pci_xhci_find_stream() checks some bits in sctx->qwSctx0, but I guess it happens to work out.

This revision is now accepted and ready to land.Aug 15 2022, 8:17 PM
usr.sbin/bhyve/pci_xhci.c
173

I will do both of those. Much of this file is under-commented though and would seem to assume knowledge of the XHCI spec in which case a more properly named field (I agree with ep_maxpstreams or possibly even ep_MaxPStreams to match the spec) is perhaps sufficient? The comment would likely say something like "Cached value of the MaxPStreams field from the endpoint context."

  • Rename field and add a comment.
This revision now requires review to proceed.Aug 16 2022, 7:01 PM
This revision is now accepted and ready to land.Aug 17 2022, 1:20 PM