Page MenuHomeFreeBSD

xhci: Rework 64-byte context support to avoid pointer abuse
ClosedPublic

Authored by jrtc27 on Oct 18 2021, 7:52 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, May 7, 11:53 PM
Unknown Object (File)
Tue, May 7, 11:53 PM
Unknown Object (File)
Tue, May 7, 11:53 PM
Unknown Object (File)
Tue, May 7, 7:55 PM
Unknown Object (File)
Feb 12 2024, 4:42 AM
Unknown Object (File)
Dec 22 2023, 11:52 AM
Unknown Object (File)
Dec 20 2023, 5:42 AM
Unknown Object (File)
Nov 16 2023, 12:22 PM
Subscribers

Details

Summary

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).

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

QEMU does 32-byte contexts so I haven't actually tested the 64-byte paths here, only that I didn't totally break the 32-byte ones, and I don't have access to the CHERI hardware whose xHCI controller uses 64-byte contexts to be able to verify it myself (though hopefully tomorrow someone will do so).

sys/dev/usb/controller/xhci.c
236

Hm, I thought usb_page_search's was a uint8_t * hence replicated that, but it's a void *. Can switch to that for these arguments (and call it buffer to match...) if you'd rather.

Did you get a chance to test this patch with both 32 and 64 byte contexts?

sys/dev/usb/controller/xhci.c
236

You could almost just write:

return (&((struct xhci_dev_ctx64 *)bytes)->ctx_slot.ctx);

Because the ctx is only used once.

How about using a macro for the basic of these transformations?

#define XHCI_GET_CTX(sc, which, field, ptr) \
((sc)->sc_ctx_is_64_byte ? &((struct which##64 *)(ptr))->field.ctx : &((struct which)(ptr))->field)
sys/dev/usb/controller/xhci.c
185

You can remove the "sc" argument from this function.

198

Ditto.

Did you get a chance to test this patch with both 32 and 64 byte contexts?

Still waiting; person with the hardware is busy with other things this morning but says they will try later today so hopefully they do get a chance to.

sys/dev/usb/controller/xhci.c
185

Ah indeed

236

Yeah I could in fact do that; I didn't do it that way because originally the way I wrote it meant the one-line approach hit the line limit and would be ugly to wrap, but that's not the case any more. The macro seems like a nice way to avoid the repetition.

I can now confirm this is working on the hardware with 64 byte contexts; will try and get a patch incorporating review comments later today

Great Jessica! Will you update the patch with the XHCI_GET_CTX() suggestion, and I'll approve it.

  • Avoid boilerplate with new XHCI_GET_CTX macro
  • Drop now-unused sc argument from dump functions
This revision is now accepted and ready to land.Oct 24 2021, 6:12 PM