Date: Sun, 30 Apr 2023 06:58:03 GMT From: Hans Petter Selasky <hselasky@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org Subject: git: decbf52b0b52 - stable/13 - xhci: Rework 64-byte context support to avoid pointer abuse Message-ID: <202304300658.33U6w3Jx071719@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch stable/13 has been updated by hselasky: URL: https://cgit.FreeBSD.org/src/commit/?id=decbf52b0b52bf84415a5960561db834c87740bf commit decbf52b0b52bf84415a5960561db834c87740bf Author: Jessica Clarke <jrtc27@FreeBSD.org> AuthorDate: 2021-10-24 18:48:46 +0000 Commit: Hans Petter Selasky <hselasky@FreeBSD.org> CommitDate: 2023-04-30 06:56:18 +0000 xhci: Rework 64-byte context support to avoid pointer abuse Currently, to support 64-byte contexts, xhci_ctx_[gs]et_le(32|64) take a pointer to the field within a 32-byte context and, if 64-byte contexts are in use, compute where the 64-byte context field is and use that instead by deriving a pointer from the 32-byte field pointer. This is done by exploiting a combination of 64-byte contexts being the same layout as their 32-byte counterparts, just with 32 bytes of padding at the end, and that all individual contexts are either in a device context or an input context which itself is page-aligned. By masking out the low 4 bits (which is the offset of the field within the 32-byte contxt) of the offset within the page, the offset of the invididual context within the containing device/input context can be determined, which is itself 32 times the number of preceding contexts. Thus, adding this value to the pointer again gets 64 times the number of preceding contexts plus the field offset, which gives the offset of the 64-byte context plus the field offset, which is the address of the field in the 64-byte context. However, this involves a fair amount of lying to the compiler when constructing these intermediate pointers, and is rather difficult to reason about. In particular, this is problematic for CHERI, where we compile the kernel with subobject bounds enabled; that is, unless annotated to opt out (e.g. for C struct inheritance reasons where you need to be able to downcast, or containerof idioms), a pointer to a member of a struct is a capability whose bounds only cover that field, and any attempt to dereference outside those bounds will fault, protecting against intra-object buffer overflows. Thus the pointer given to xhci_ctx_[gs]et_le(32|64) is a capability whose bounds only cover the field in the 32-byte context, and computing the pointer to the 64-byte context field takes the address out of bounds, resulting in a fault when later dereferenced. This can be cleaned up by using a different abstraction. Instead of doing the 32-byte to 64-byte conversion on access to the field, we can do the conversion when getting a pointer to the context itself, and define proper 64-byte versions of contexts in order to let the compiler do all the necessary arithmetic rather than do it manually ourselves. This provides a cleaner implementation, works for CHERI and may even be slightly more performant as it avoids the need to mess with masking pointers (which cannot in the general case be optimised by compilers to be reused across accesses to different fields within the same context, since it does not know that the contexts are over-aligned compared with the C ABI requirements). Reviewed by: hselasky Differential Revision: https://reviews.freebsd.org/D32554 (cherry picked from commit 29863d1effe20da3cc75ae10bd52d96edafe9e59) --- sys/dev/usb/controller/xhci.c | 160 +++++++++++++++--------------------------- sys/dev/usb/controller/xhci.h | 26 +++++++ 2 files changed, 81 insertions(+), 105 deletions(-) diff --git a/sys/dev/usb/controller/xhci.c b/sys/dev/usb/controller/xhci.c index 02a9a37fbd2d..e28529ca003d 100644 --- a/sys/dev/usb/controller/xhci.c +++ b/sys/dev/usb/controller/xhci.c @@ -88,6 +88,11 @@ #define XHCI_BUS2SC(bus) \ __containerof(bus, struct xhci_softc, sc_bus) +#define XHCI_GET_CTX(sc, which, field, ptr) \ + ((sc)->sc_ctx_is_64_byte ? \ + &((struct which##64 *)(ptr))->field.ctx : \ + &((struct which *)(ptr))->field) + static SYSCTL_NODE(_hw_usb, OID_AUTO, xhci, CTLFLAG_RW | CTLFLAG_MPSAFE, 0, "USB XHCI"); @@ -167,12 +172,6 @@ static usb_error_t xhci_configure_mask(struct usb_device *, static usb_error_t xhci_cmd_evaluate_ctx(struct xhci_softc *, uint64_t, uint8_t); static void xhci_endpoint_doorbell(struct usb_xfer *); -static void xhci_ctx_set_le32(struct xhci_softc *sc, volatile uint32_t *ptr, uint32_t val); -static uint32_t xhci_ctx_get_le32(struct xhci_softc *sc, volatile uint32_t *ptr); -static void xhci_ctx_set_le64(struct xhci_softc *sc, volatile uint64_t *ptr, uint64_t val); -#ifdef USB_DEBUG -static uint64_t xhci_ctx_get_le64(struct xhci_softc *sc, volatile uint64_t *ptr); -#endif static const struct usb_bus_methods xhci_bus_methods; @@ -187,26 +186,26 @@ xhci_dump_trb(struct xhci_trb *trb) } static void -xhci_dump_endpoint(struct xhci_softc *sc, struct xhci_endp_ctx *pep) +xhci_dump_endpoint(struct xhci_endp_ctx *pep) { DPRINTFN(5, "pep = %p\n", pep); - DPRINTFN(5, "dwEpCtx0=0x%08x\n", xhci_ctx_get_le32(sc, &pep->dwEpCtx0)); - DPRINTFN(5, "dwEpCtx1=0x%08x\n", xhci_ctx_get_le32(sc, &pep->dwEpCtx1)); - DPRINTFN(5, "qwEpCtx2=0x%016llx\n", (long long)xhci_ctx_get_le64(sc, &pep->qwEpCtx2)); - DPRINTFN(5, "dwEpCtx4=0x%08x\n", xhci_ctx_get_le32(sc, &pep->dwEpCtx4)); - DPRINTFN(5, "dwEpCtx5=0x%08x\n", xhci_ctx_get_le32(sc, &pep->dwEpCtx5)); - DPRINTFN(5, "dwEpCtx6=0x%08x\n", xhci_ctx_get_le32(sc, &pep->dwEpCtx6)); - DPRINTFN(5, "dwEpCtx7=0x%08x\n", xhci_ctx_get_le32(sc, &pep->dwEpCtx7)); + DPRINTFN(5, "dwEpCtx0=0x%08x\n", le32toh(pep->dwEpCtx0)); + DPRINTFN(5, "dwEpCtx1=0x%08x\n", le32toh(pep->dwEpCtx1)); + DPRINTFN(5, "qwEpCtx2=0x%016llx\n", (long long)le64toh(pep->qwEpCtx2)); + DPRINTFN(5, "dwEpCtx4=0x%08x\n", le32toh(pep->dwEpCtx4)); + DPRINTFN(5, "dwEpCtx5=0x%08x\n", le32toh(pep->dwEpCtx5)); + DPRINTFN(5, "dwEpCtx6=0x%08x\n", le32toh(pep->dwEpCtx6)); + DPRINTFN(5, "dwEpCtx7=0x%08x\n", le32toh(pep->dwEpCtx7)); } static void -xhci_dump_device(struct xhci_softc *sc, struct xhci_slot_ctx *psl) +xhci_dump_device(struct xhci_slot_ctx *psl) { DPRINTFN(5, "psl = %p\n", psl); - DPRINTFN(5, "dwSctx0=0x%08x\n", xhci_ctx_get_le32(sc, &psl->dwSctx0)); - DPRINTFN(5, "dwSctx1=0x%08x\n", xhci_ctx_get_le32(sc, &psl->dwSctx1)); - DPRINTFN(5, "dwSctx2=0x%08x\n", xhci_ctx_get_le32(sc, &psl->dwSctx2)); - DPRINTFN(5, "dwSctx3=0x%08x\n", xhci_ctx_get_le32(sc, &psl->dwSctx3)); + DPRINTFN(5, "dwSctx0=0x%08x\n", le32toh(psl->dwSctx0)); + DPRINTFN(5, "dwSctx1=0x%08x\n", le32toh(psl->dwSctx1)); + DPRINTFN(5, "dwSctx2=0x%08x\n", le32toh(psl->dwSctx2)); + DPRINTFN(5, "dwSctx3=0x%08x\n", le32toh(psl->dwSctx3)); } #endif @@ -238,60 +237,6 @@ xhci_iterate_hw_softc(struct usb_bus *bus, usb_bus_mem_sub_cb_t *cb) } } -static void -xhci_ctx_set_le32(struct xhci_softc *sc, volatile uint32_t *ptr, uint32_t val) -{ - if (sc->sc_ctx_is_64_byte) { - uint32_t offset; - /* exploit the fact that our structures are XHCI_PAGE_SIZE aligned */ - /* all contexts are initially 32-bytes */ - offset = ((uintptr_t)ptr) & ((XHCI_PAGE_SIZE - 1) & ~(31U)); - ptr = (volatile uint32_t *)(((volatile uint8_t *)ptr) + offset); - } - *ptr = htole32(val); -} - -static uint32_t -xhci_ctx_get_le32(struct xhci_softc *sc, volatile uint32_t *ptr) -{ - if (sc->sc_ctx_is_64_byte) { - uint32_t offset; - /* exploit the fact that our structures are XHCI_PAGE_SIZE aligned */ - /* all contexts are initially 32-bytes */ - offset = ((uintptr_t)ptr) & ((XHCI_PAGE_SIZE - 1) & ~(31U)); - ptr = (volatile uint32_t *)(((volatile uint8_t *)ptr) + offset); - } - return (le32toh(*ptr)); -} - -static void -xhci_ctx_set_le64(struct xhci_softc *sc, volatile uint64_t *ptr, uint64_t val) -{ - if (sc->sc_ctx_is_64_byte) { - uint32_t offset; - /* exploit the fact that our structures are XHCI_PAGE_SIZE aligned */ - /* all contexts are initially 32-bytes */ - offset = ((uintptr_t)ptr) & ((XHCI_PAGE_SIZE - 1) & ~(31U)); - ptr = (volatile uint64_t *)(((volatile uint8_t *)ptr) + offset); - } - *ptr = htole64(val); -} - -#ifdef USB_DEBUG -static uint64_t -xhci_ctx_get_le64(struct xhci_softc *sc, volatile uint64_t *ptr) -{ - if (sc->sc_ctx_is_64_byte) { - uint32_t offset; - /* exploit the fact that our structures are XHCI_PAGE_SIZE aligned */ - /* all contexts are initially 32-bytes */ - offset = ((uintptr_t)ptr) & ((XHCI_PAGE_SIZE - 1) & ~(31U)); - ptr = (volatile uint64_t *)(((volatile uint8_t *)ptr) + offset); - } - return (le64toh(*ptr)); -} -#endif - static int xhci_reset_command_queue_locked(struct xhci_softc *sc) { @@ -1400,7 +1345,7 @@ xhci_set_address(struct usb_device *udev, struct mtx *mtx, uint16_t address) struct usb_page_search buf_dev; struct xhci_softc *sc = XHCI_BUS2SC(udev->bus); struct xhci_hw_dev *hdev; - struct xhci_dev_ctx *pdev; + struct xhci_slot_ctx *slot; struct xhci_endpoint_ext *pepext; uint32_t temp; uint16_t mps; @@ -1493,10 +1438,11 @@ xhci_set_address(struct usb_device *udev, struct mtx *mtx, uint16_t address) /* update device address to new value */ usbd_get_page(&hdev->device_pc, 0, &buf_dev); - pdev = buf_dev.buffer; + slot = XHCI_GET_CTX(sc, xhci_dev_ctx, ctx_slot, + buf_dev.buffer); usb_pc_cpu_invalidate(&hdev->device_pc); - temp = xhci_ctx_get_le32(sc, &pdev->ctx_slot.dwSctx3); + temp = le32toh(slot->dwSctx3); udev->address = XHCI_SCTX_3_DEV_ADDR_GET(temp); /* update device state to new value */ @@ -2304,7 +2250,8 @@ xhci_configure_mask(struct usb_device *udev, uint32_t mask, uint8_t drop) { struct xhci_softc *sc = XHCI_BUS2SC(udev->bus); struct usb_page_search buf_inp; - struct xhci_input_dev_ctx *pinp; + struct xhci_input_ctx *input; + struct xhci_slot_ctx *slot; uint32_t temp; uint8_t index; uint8_t x; @@ -2313,22 +2260,23 @@ xhci_configure_mask(struct usb_device *udev, uint32_t mask, uint8_t drop) usbd_get_page(&sc->sc_hw.devs[index].input_pc, 0, &buf_inp); - pinp = buf_inp.buffer; + input = XHCI_GET_CTX(sc, xhci_input_dev_ctx, ctx_input, + buf_inp.buffer); + slot = XHCI_GET_CTX(sc, xhci_input_dev_ctx, ctx_slot, buf_inp.buffer); if (drop) { mask &= XHCI_INCTX_NON_CTRL_MASK; - xhci_ctx_set_le32(sc, &pinp->ctx_input.dwInCtx0, mask); - xhci_ctx_set_le32(sc, &pinp->ctx_input.dwInCtx1, 0); + input->dwInCtx0 = htole32(mask); + input->dwInCtx1 = htole32(0); } else { /* * Some hardware requires that we drop the endpoint * context before adding it again: */ - xhci_ctx_set_le32(sc, &pinp->ctx_input.dwInCtx0, - mask & XHCI_INCTX_NON_CTRL_MASK); + input->dwInCtx0 = htole32(mask & XHCI_INCTX_NON_CTRL_MASK); /* Add new endpoint context */ - xhci_ctx_set_le32(sc, &pinp->ctx_input.dwInCtx1, mask); + input->dwInCtx1 = htole32(mask); /* find most significant set bit */ for (x = 31; x != 1; x--) { @@ -2346,10 +2294,10 @@ xhci_configure_mask(struct usb_device *udev, uint32_t mask, uint8_t drop) x = sc->sc_hw.devs[index].context_num; /* update number of contexts */ - temp = xhci_ctx_get_le32(sc, &pinp->ctx_slot.dwSctx0); + temp = le32toh(slot->dwSctx0); temp &= ~XHCI_SCTX_0_CTX_NUM_SET(31); temp |= XHCI_SCTX_0_CTX_NUM_SET(x + 1); - xhci_ctx_set_le32(sc, &pinp->ctx_slot.dwSctx0, temp); + slot->dwSctx0 = htole32(temp); } usb_pc_cpu_flush(&sc->sc_hw.devs[index].input_pc); return (0); @@ -2364,7 +2312,7 @@ xhci_configure_endpoint(struct usb_device *udev, { struct usb_page_search buf_inp; struct xhci_softc *sc = XHCI_BUS2SC(udev->bus); - struct xhci_input_dev_ctx *pinp; + struct xhci_endp_ctx *endp; uint64_t ring_addr = pepext->physaddr; uint32_t temp; uint8_t index; @@ -2375,8 +2323,6 @@ xhci_configure_endpoint(struct usb_device *udev, usbd_get_page(&sc->sc_hw.devs[index].input_pc, 0, &buf_inp); - pinp = buf_inp.buffer; - epno = edesc->bEndpointAddress; type = edesc->bmAttributes & UE_XFERTYPE; @@ -2396,6 +2342,9 @@ xhci_configure_endpoint(struct usb_device *udev, if (mult == 0) return (USB_ERR_BAD_BUFSIZE); + endp = XHCI_GET_CTX(sc, xhci_input_dev_ctx, ctx_ep[epno - 1], + buf_inp.buffer); + /* store endpoint mode */ pepext->trb_ep_mode = ep_mode; /* store bMaxPacketSize for control endpoints */ @@ -2451,7 +2400,7 @@ xhci_configure_endpoint(struct usb_device *udev, break; } - xhci_ctx_set_le32(sc, &pinp->ctx_ep[epno - 1].dwEpCtx0, temp); + endp->dwEpCtx0 = htole32(temp); temp = XHCI_EPCTX_1_HID_SET(0) | @@ -2486,8 +2435,8 @@ xhci_configure_endpoint(struct usb_device *udev, if (epno & 1) temp |= XHCI_EPCTX_1_EPTYPE_SET(4); - xhci_ctx_set_le32(sc, &pinp->ctx_ep[epno - 1].dwEpCtx1, temp); - xhci_ctx_set_le64(sc, &pinp->ctx_ep[epno - 1].qwEpCtx2, ring_addr); + endp->dwEpCtx1 = htole32(temp); + endp->qwEpCtx2 = htole64(ring_addr); switch (edesc->bmAttributes & UE_XFERTYPE) { case UE_INTERRUPT: @@ -2504,10 +2453,10 @@ xhci_configure_endpoint(struct usb_device *udev, break; } - xhci_ctx_set_le32(sc, &pinp->ctx_ep[epno - 1].dwEpCtx4, temp); + endp->dwEpCtx4 = htole32(temp); #ifdef USB_DEBUG - xhci_dump_endpoint(sc, &pinp->ctx_ep[epno - 1]); + xhci_dump_endpoint(endp); #endif usb_pc_cpu_flush(&sc->sc_hw.devs[index].input_pc); @@ -2563,7 +2512,7 @@ xhci_configure_device(struct usb_device *udev) struct xhci_softc *sc = XHCI_BUS2SC(udev->bus); struct usb_page_search buf_inp; struct usb_page_cache *pcinp; - struct xhci_input_dev_ctx *pinp; + struct xhci_slot_ctx *slot; struct usb_device *hubdev; uint32_t temp; uint32_t route; @@ -2580,7 +2529,7 @@ xhci_configure_device(struct usb_device *udev) usbd_get_page(pcinp, 0, &buf_inp); - pinp = buf_inp.buffer; + slot = XHCI_GET_CTX(sc, xhci_input_dev_ctx, ctx_slot, buf_inp.buffer); rh_port = 0; route = 0; @@ -2655,7 +2604,7 @@ xhci_configure_device(struct usb_device *udev) if (is_hub) temp |= XHCI_SCTX_0_HUB_SET(1); - xhci_ctx_set_le32(sc, &pinp->ctx_slot.dwSctx0, temp); + slot->dwSctx0 = htole32(temp); temp = XHCI_SCTX_1_RH_PORT_SET(rh_port); @@ -2664,7 +2613,7 @@ xhci_configure_device(struct usb_device *udev) sc->sc_hw.devs[index].nports); } - xhci_ctx_set_le32(sc, &pinp->ctx_slot.dwSctx1, temp); + slot->dwSctx1 = htole32(temp); temp = XHCI_SCTX_2_IRQ_TARGET_SET(0); @@ -2690,7 +2639,7 @@ xhci_configure_device(struct usb_device *udev) break; } - xhci_ctx_set_le32(sc, &pinp->ctx_slot.dwSctx2, temp); + slot->dwSctx2 = htole32(temp); /* * These fields should be initialized to zero, according to @@ -2699,10 +2648,10 @@ xhci_configure_device(struct usb_device *udev) temp = XHCI_SCTX_3_DEV_ADDR_SET(0) | XHCI_SCTX_3_SLOT_STATE_SET(0); - xhci_ctx_set_le32(sc, &pinp->ctx_slot.dwSctx3, temp); + slot->dwSctx3 = htole32(temp); #ifdef USB_DEBUG - xhci_dump_device(sc, &pinp->ctx_slot); + xhci_dump_device(slot); #endif usb_pc_cpu_flush(pcinp); @@ -2731,7 +2680,7 @@ xhci_alloc_device_ext(struct usb_device *udev) pc->tag_parent = sc->sc_bus.dma_parent_tag; if (usb_pc_alloc_mem(pc, pg, sc->sc_ctx_is_64_byte ? - (2 * sizeof(struct xhci_dev_ctx)) : + sizeof(struct xhci_dev_ctx64) : sizeof(struct xhci_dev_ctx), XHCI_PAGE_SIZE)) goto error; @@ -2744,7 +2693,7 @@ xhci_alloc_device_ext(struct usb_device *udev) pc->tag_parent = sc->sc_bus.dma_parent_tag; if (usb_pc_alloc_mem(pc, pg, sc->sc_ctx_is_64_byte ? - (2 * sizeof(struct xhci_input_dev_ctx)) : + sizeof(struct xhci_input_dev_ctx64) : sizeof(struct xhci_input_dev_ctx), XHCI_PAGE_SIZE)) { goto error; } @@ -3792,7 +3741,7 @@ xhci_get_endpoint_state(struct usb_device *udev, uint8_t epno) struct xhci_softc *sc = XHCI_BUS2SC(udev->bus); struct usb_page_search buf_dev; struct xhci_hw_dev *hdev; - struct xhci_dev_ctx *pdev; + struct xhci_endp_ctx *endp; uint32_t temp; MPASS(epno != 0); @@ -3800,10 +3749,11 @@ xhci_get_endpoint_state(struct usb_device *udev, uint8_t epno) hdev = &sc->sc_hw.devs[udev->controller_slot_id]; usbd_get_page(&hdev->device_pc, 0, &buf_dev); - pdev = buf_dev.buffer; + endp = XHCI_GET_CTX(sc, xhci_dev_ctx, ctx_ep[epno - 1], + buf_dev.buffer); usb_pc_cpu_invalidate(&hdev->device_pc); - temp = xhci_ctx_get_le32(sc, &pdev->ctx_ep[epno - 1].dwEpCtx0); + temp = le32toh(endp->dwEpCtx0); return (XHCI_EPCTX_0_EPSTATE_GET(temp)); } diff --git a/sys/dev/usb/controller/xhci.h b/sys/dev/usb/controller/xhci.h index d98df2bdf512..4701e62ba3e5 100644 --- a/sys/dev/usb/controller/xhci.h +++ b/sys/dev/usb/controller/xhci.h @@ -111,6 +111,11 @@ struct xhci_slot_ctx { volatile uint32_t dwSctx7; }; +struct xhci_slot_ctx64 { + struct xhci_slot_ctx ctx; + volatile uint8_t padding[32]; +}; + struct xhci_endp_ctx { volatile uint32_t dwEpCtx0; #define XHCI_EPCTX_0_EPSTATE_SET(x) ((x) & 0x7) @@ -156,6 +161,11 @@ struct xhci_endp_ctx { volatile uint32_t dwEpCtx7; }; +struct xhci_endp_ctx64 { + struct xhci_endp_ctx ctx; + volatile uint8_t padding[32]; +}; + struct xhci_input_ctx { #define XHCI_INCTX_NON_CTRL_MASK 0xFFFFFFFCU volatile uint32_t dwInCtx0; @@ -170,17 +180,33 @@ struct xhci_input_ctx { volatile uint32_t dwInCtx7; }; +struct xhci_input_ctx64 { + struct xhci_input_ctx ctx; + volatile uint8_t padding[32]; +}; + struct xhci_input_dev_ctx { struct xhci_input_ctx ctx_input; struct xhci_slot_ctx ctx_slot; struct xhci_endp_ctx ctx_ep[XHCI_MAX_ENDPOINTS - 1]; }; +struct xhci_input_dev_ctx64 { + struct xhci_input_ctx64 ctx_input; + struct xhci_slot_ctx64 ctx_slot; + struct xhci_endp_ctx64 ctx_ep[XHCI_MAX_ENDPOINTS - 1]; +}; + struct xhci_dev_ctx { struct xhci_slot_ctx ctx_slot; struct xhci_endp_ctx ctx_ep[XHCI_MAX_ENDPOINTS - 1]; } __aligned(XHCI_DEV_CTX_ALIGN); +struct xhci_dev_ctx64 { + struct xhci_slot_ctx64 ctx_slot; + struct xhci_endp_ctx64 ctx_ep[XHCI_MAX_ENDPOINTS - 1]; +} __aligned(XHCI_DEV_CTX_ALIGN); + struct xhci_stream_ctx { volatile uint64_t qwSctx0; #define XHCI_SCTX_0_DCS_GET(x) ((x) & 0x1)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202304300658.33U6w3Jx071719>