[PATCH v4 0/4] thunderbolt: harden XDomain property parser

[Michael Bommarito]

Style cleanups only on top of v3.  Andy's three nits on 1/4, 2/4,
3/4 are applied; Mika's request to drop the duplicated on-wire
entry struct in 4/4 is applied.  No behavioural change to any
patch; the bug analysis and the gating in patches 1-3 are
unchanged.

Three independent memory-safety defects in drivers/thunderbolt/property.c
are reachable when an untrusted Thunderbolt/USB4 XDomain peer responds
to a PROPERTIES_REQUEST during host-to-host discovery.  The peer
supplies up to TB_XDP_PROPERTIES_MAX_LENGTH (500) dwords of attacker-
controlled property block which the local host passes to
tb_property_parse_dir() as part of the control-plane exchange that
runs before any tunnels are set up.

Patches 1-3 are one bug per patch: u32 overflow in
tb_property_entry_valid(), short-dir_len OOB+underflow in
__tb_property_parse_dir(), and unbounded recursion in the same.
Patch 4 is three KUnit regression cases exercising all three.

All three defects are OOB-read or DoS at worst.  No controlled OOB
write is reachable through the parser; parse_dwdata()'s destination
is a freshly kcalloc'd buffer sized by entry->length.

Operators who do not need XDomain host-to-host discovery can disable
the path entirely with thunderbolt.xdomain=0 on the kernel command
line.

Reproduced on v7.0-rc7 + CONFIG_KASAN=y + CONFIG_USB4_KUNIT_TEST=y
via the KUnit suite in patch 4.  Pre-fix on a v7.0-rc7 + patch 4
kernel: u32_wrap fails with a KASAN use-after-free trace in
__tb_property_parse_dir() (the parser reads ~16 GiB past the
block); recursion fails with KASAN + an Oops on RIP=0 as the
parser exhausts its guard page.  dir_len_underflow returns NULL
on pre-fix because the downstream content_len = dir_len - 4
underflow makes the entry walk bail at tb_property_entry_valid();
the UUID kmemdup over-read is silent here because KASAN-Generic's
slab redzones do not flag a 4-byte over-read into the
kmalloc-chunk tail.  Treat dir_len_underflow as the post-fix
invariant pin; u32_wrap and recursion are the active pre-fix
detectors.

Post-fix (all four patches): all three pass cleanly with KASAN
active.

Changes since v3
----------------

Cosmetic (per v3 review):

  - Patch 1/4 (Andy Shevchenko): drop the redundant (u32) cast on
    entry->length in check_add_overflow().  __builtin_add_overflow
    handles mixed width via implicit promotion; the cast was noise.

  - Patch 2/4 (Andy Shevchenko): insert a blank line between the
    !dir error return and the new INIT_LIST_HEAD(&dir->properties).

  - Patch 3/4 (Andy Shevchenko): keep the four-argument
    tb_property_parse(block, block_len, &entries[i], depth) on a
    single line (>80 col) instead of wrapping the trailing argument.

  - Patch 4/4 (Mika Westerberg): drop the private
    struct tb_test_property_entry overlay.  Populate the crafted
    blocks by writing u32 dwords directly, matching the existing
    root_directory[] style used elsewhere in this file.  Each test's
    kunit_kzalloc is right-sized to the dwords needed to actually
    exercise the bug (0x102 for u32_wrap, 10 for recursion, 7 for
    dir_len_underflow); the 500-dword allocation v3 used has been
    dropped.

    u32_wrap retains length=0x100 / value=0xffffff00 from v3 so
    the entry's length field clears the "entry->length > block_len"
    gate (block_len = 0x102 dwords) and the wrap check is what
    actually fires.  recursion uses length=8 (was 16 in v3) so the
    smaller block can hold both the parent UUID kmemdup and the
    single child entry that drives the recursion.  All three
    pre-fix scenarios are still observable: u32_wrap page-faults
    on the KASAN shadow lookup for the wild OOB offset, recursion
    trips a KASAN out-of-bounds report in __unwind_start as the
    per-task kernel stack is consumed, dir_len_underflow trips
    KASAN slab-out-of-bounds in kmemdup_noprof.  Post-fix all
    three pass.

Changes since v2
----------------

Material:

  - Patch 2/4: move "dir_len < 4" reject before the UUID kmemdup
    in the non-root parse path.  v2 placed it after, so a crafted
    entry with dir_offset near end of block and dir_len in 0..3
    OOB-read up to 4 dwords past the block before the reject ran
    (dir_offset=497, dir_len=3, block_len=500 reads
    block[497..501]).  Both that OOB and the original
    content_len = dir_len - 4 underflow now hit the same gate.

  - Patch 4/4: tighten dir_len_underflow's buffer (7 dwords,
    kmalloc-32) and reposition the entry (e->value=4) to focus the
    UUID kmemdup on the chunk tail.  KASAN-Generic does not flag
    the 4-byte over-read into the tail, so the test remains a
    post-fix invariant pin (documented above); v2's wider buffer
    obscured even the post-fix-pin shape.

  - Patches 1/4, 2/4, 3/4: fix Fixes: SHA.  v2 used e69b6c02b4c3
    ("net: Add support for networking over Thunderbolt cable"),
    the wrong commit.  Correct is cdae7c07e3e3 ("thunderbolt: Add
    support for XDomain properties").

Cosmetic (per v2 review):

  - Lowercase 0xffffff00 in 1/4 and 4/4 commit messages, and 4/4
    code + comments.
  - Patch 4/4: use TB_PROPERTY_TYPE_DATA / TB_PROPERTY_TYPE_DIRECTORY
    constants from <linux/thunderbolt.h> instead of bare 0x64 / 0x44.
    (v4 reverts to bare hex in the u32 dword that packs (type <<
    24), since the type byte is now part of a packed dword literal;
    each dword carries a `/* type=... */` comment.)
  - Patch 4/4: convert all multi-line block comments to put the
    opening "/*" on its own line per the thunderbolt subsystem's
    coding style.


Michael Bommarito (4):
  thunderbolt: property: reject u32 wrap in tb_property_entry_valid()
  thunderbolt: property: reject dir_len < 4 to prevent size_t underflow
  thunderbolt: property: cap recursion depth in
    __tb_property_parse_dir()
  thunderbolt: test: add KUnit regression tests for XDomain property
    parser

 drivers/thunderbolt/property.c |  32 ++++++---
 drivers/thunderbolt/test.c     | 126 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 149 insertions(+), 9 deletions(-)

--
2.53.0