Bug 264347
| Summary: | bhyve guest can cause access beyond end of pci_xhci.c's ep_sctx_trbs[] array | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Robert Morris <rtm> |
| Component: | bhyve | Assignee: | John Baldwin <jhb> |
| Status: | Closed FIXED | ||
| Severity: | Affects Some People | CC: | emaste, grehan, jhb, jrm, secteam |
| Priority: | --- | Keywords: | needs-patch, needs-qa |
| Version: | Unspecified | Flags: | koobs:
maintainer-feedback?
(secteam) koobs: maintainer-feedback? (grehan) koobs: mfc-stable13? koobs: mfc-stable12? |
| Hardware: | Any | ||
| OS: | Any | ||
| See Also: | https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=264294 | ||
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=e7439f6aeb235ba3a7e79818c56a63d066c80854 commit e7439f6aeb235ba3a7e79818c56a63d066c80854 Author: John Baldwin <jhb@FreeBSD.org> AuthorDate: 2022-08-17 17:00:36 +0000 Commit: John Baldwin <jhb@FreeBSD.org> CommitDate: 2022-08-17 17:00:36 +0000 bhyve xhci: Cache the value of MaxPStreams when initializing an endpoint. 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> Reviewed by: markj MFC after: 1 week Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D36181 usr.sbin/bhyve/pci_xhci.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=ecfbefc91e75674899b3014ab39720f4a13beca5 commit ecfbefc91e75674899b3014ab39720f4a13beca5 Author: John Baldwin <jhb@FreeBSD.org> AuthorDate: 2022-08-17 17:00:36 +0000 Commit: John Baldwin <jhb@FreeBSD.org> CommitDate: 2022-08-25 16:35:12 +0000 bhyve xhci: Cache the value of MaxPStreams when initializing an endpoint. 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> Reviewed by: markj MFC after: 1 week Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D36181 (cherry picked from commit e7439f6aeb235ba3a7e79818c56a63d066c80854) usr.sbin/bhyve/pci_xhci.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=920489d03cedbe232c843f36fa5f2d954a5aae15 commit 920489d03cedbe232c843f36fa5f2d954a5aae15 Author: John Baldwin <jhb@FreeBSD.org> AuthorDate: 2022-08-17 17:00:36 +0000 Commit: John Baldwin <jhb@FreeBSD.org> CommitDate: 2022-08-25 16:39:22 +0000 bhyve xhci: Cache the value of MaxPStreams when initializing an endpoint. 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> Reviewed by: markj MFC after: 1 week Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D36181 (cherry picked from commit e7439f6aeb235ba3a7e79818c56a63d066c80854) usr.sbin/bhyve/pci_xhci.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) |
Bhyve's pci_xhci_init_ep() allocates space for multiple streams if the guest asks for them: pstreams = XHCI_EPCTX_0_MAXP_STREAMS_GET(ep_ctx->dwEpCtx0); if (pstreams > 0) { ...; devep->ep_sctx_trbs = calloc(pstreams, sizeof(struct pci_xhci_trb_ring)); So if the guest asks for one stream, only devep->ep_sctx_trbs[0] is valid. ep_sctx_trbs[] is used in pci_xhci_device_doorbell(): if (XHCI_EPCTX_0_MAXP_STREAMS_GET(ep_ctx->dwEpCtx0) != 0) { /* * Stream IDs of 0, 65535 (any stream), and 65534 * (prime) are invalid. */ if (streamid == 0 || streamid == 65534 || streamid == 65535) { DPRINTF(("pci_xhci: invalid stream")); return; } ...; sctx_tr = &devep->ep_sctx_trbs[streamid]; ...; ringaddr = sctx_tr->ringaddr; But here, if the guest asks for one stream, the code requires streamid to be 1, and thus indexes beyond the end of ep_sctx_trbs[].