Bug 264347 - bhyve guest can cause access beyond end of pci_xhci.c's ep_sctx_trbs[] array
Summary: bhyve guest can cause access beyond end of pci_xhci.c's ep_sctx_trbs[] array
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bhyve (show other bugs)
Version: Unspecified
Hardware: Any Any
: --- Affects Some People
Assignee: John Baldwin
URL:
Keywords: needs-patch, needs-qa
Depends on:
Blocks:
 
Reported: 2022-05-30 09:52 UTC by Robert Morris
Modified: 2022-08-26 15:57 UTC (History)
5 users (show)

See Also:
koobs: maintainer-feedback? (secteam)
koobs: maintainer-feedback? (grehan)
koobs: mfc-stable13?
koobs: mfc-stable12?


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Morris 2022-05-30 09:52:03 UTC
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[].
Comment 1 commit-hook freebsd_committer freebsd_triage 2022-08-17 17:02:32 UTC
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(-)
Comment 2 commit-hook freebsd_committer freebsd_triage 2022-08-25 17:31:59 UTC
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(-)
Comment 3 commit-hook freebsd_committer freebsd_triage 2022-08-25 17:33:02 UTC
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(-)