public inbox for linux-usb@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] thunderbolt: harden XDomain property parser
@ 2026-04-15  3:23 Michael Bommarito
  2026-04-15  3:23 ` [PATCH 1/2] thunderbolt: property: harden XDomain property parser against crafted peer Michael Bommarito
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Michael Bommarito @ 2026-04-15  3:23 UTC (permalink / raw)
  To: linux-usb, Mika Westerberg
  Cc: Andreas Noever, Yehezkel Bernat, Greg Kroah-Hartman, linux-kernel,
	stable

Hi all,

Three independent memory-safety defects in drivers/thunderbolt/
property.c, each reproduced on v7.0-rc7 with CONFIG_KASAN=y
under qemu + KUnit.  Pre-fix, crafted XDomain property blocks
oops the kernel inside __tb_property_parse_dir with CR2 in the
KASAN shadow region.  Post-fix, the three KUnit cases in patch
2/2 pass cleanly.  

The defects are reachable when any untrusted Thunderbolt/USB4
XDomain peer responds to a PROPERTIES_REQUEST during host-to-host
discovery.  The peer delivers up to TB_XDP_PROPERTIES_MAX_LENGTH
(500) dwords of property block which the local host parses before
any PCIe tunnel is authorized.  The specific parser sites here
have been sitting here since the original 2017 XDomain support;
the file has had three prior NULL-check fixes in 2019
(106204b56f60, e4dfdd5804cc, 6183d5a51866), none of which touch
the bounds / arithmetic / recursion sites addressed here.

Worst case situation is either small OOB reads or DoS for
someone with physical access.

Defects
-------

A. u32 overflow in tb_property_entry_valid() (property.c:61).
   entry->value (u32) + entry->length (u16->u32) is performed in
   u32 and wraps.  With block_len = 500 an attacker picks
   value = 0xFFFFFF00, length = 0x100; the u32 sum 0x100000000
   wraps to 0, passing the > block_len check.  The subsequent
   parse_dwdata() at :132/:143 reads entry->length*4 bytes from
   block + entry->value (pointer arithmetic promoted to size_t,
   ~16 GiB past the allocation) into a freshly kcalloc'd
   destination.

   Exfil path: TB_PROPERTY_TYPE_TEXT on the "deviceid" or
   "vendorid" keys has its value.text copied into xd->device_name
   / xd->vendor_name via kstrdup() (xdomain.c:1157-1162), which
   are read back via the per-XDomain device_name and vendor_name
   sysfs attribute show() functions (xdomain.c:1730, 1763).
   kstrdup stops at the first NUL byte, so the usable leak is a
   NUL-bounded prefix of attacker-directed kernel memory.  This
   is not an RCE or arbitrary-write primitive; it is an
   OOB-read / info-leak class, untargeted (the attacker does not
   know block's KASLR/slab placement) and bounded by per-read NUL
   termination.  Fix: check_add_overflow() on value + length.

B. Unbounded recursion in __tb_property_parse_dir().
   DIRECTORY entries are parsed recursively with no depth counter;
   a peer that crafts a back-reference chain drives the parser
   until the 16 KiB kernel stack is exhausted and the guard page
   fires (pre-authentication remote DoS).  Fix: bound recursion
   to TB_PROPERTY_MAX_DEPTH = 8.

C. size_t underflow on dir_len - 4 (property.c:184).
   dir_len arrives as size_t sourced from entry->length (u16) on
   the non-root path; length < 4 underflows to ~SIZE_MAX,
   nentries = SIZE_MAX/4, loop walks entries past the block.
   OOB read + potential kernel oops.  Fix: reject dir_len < 4 on
   the non-root path.

Additional hardening: move INIT_LIST_HEAD(&dir->properties) to
immediately after dir allocation so every error-return path that
calls tb_property_free_dir() sees a walkable empty list rather
than the zero-initialized NULL next/prev that would oops
list_for_each_entry_safe().  This also closes a pre-existing
latent bug in the dir->uuid kmemdup-failure path at
property.c:180.

No controlled OOB-write is reachable through the parser;
parse_dwdata's destination is a freshly kcalloc'd buffer sized by
entry->length.

Attacker model
--------------

Malicious Thunderbolt/USB4 XDomain peer (cable, dock, in-line
inspector, adjacent host).  Discovery fires during link training;
PCIe tunnel authorization (the thunderbolt/.../authorized sysfs
gate) does not guard the control-plane PROPERTIES_REQUEST /
RESPONSE path.  Host IOMMU does not mitigate because the data
arrives as a control-plane payload the driver willingly copies
into its own buffer before parsing.  No user interaction beyond
the link-up event.

Reproduction
------------

Patch 2/2 adds three KUnit regression tests
(tb_test_property_parse_u32_wrap / _recursion /
_dir_len_underflow) to drivers/thunderbolt/test.c.  Tested
end-to-end on v7.0-rc7 + CONFIG_USB4_KUNIT_TEST=y + CONFIG_KASAN=y
under tools/testing/kunit/kunit.py run --arch=x86_64:

Pre-fix, each test oopses inside __tb_property_parse_dir:

  Oops: SMP KASAN PTI
  RIP: __tb_property_parse_dir+0x3ce
  CR2: 0xffffed108045eb80     # KASAN shadow region
  Code: ... 48 c1 ea 03 <0f b6 0c 2a>    # KASAN shadow byte load
  R8:  0x100                  # crafted entry->length
  [FAILED] tb_test_property_parse_u32_wrap
  [FAILED] tb_test_property_parse_recursion
  [FAILED] tb_test_property_parse_dir_len_underflow

Post-fix:

  [PASSED] tb_test_property_parse_u32_wrap
  [PASSED] tb_test_property_parse_recursion
  [PASSED] tb_test_property_parse_dir_len_underflow

Full run command:

  ./tools/testing/kunit/kunit.py run --arch=x86_64 \
    --kconfig_add CONFIG_PCI=y --kconfig_add CONFIG_NVMEM=y \
    --kconfig_add CONFIG_USB4=y --kconfig_add CONFIG_USB4_KUNIT_TEST=y \
    --kconfig_add CONFIG_KASAN=y 'thunderbolt.tb_test_property_parse_*'

Series
------

  [PATCH 1/2] thunderbolt: property: harden XDomain property
    parser against crafted peer
  [PATCH 2/2] thunderbolt: test: add KUnit regression tests for
    XDomain property parser

Patches apply to v7.0-rc7 (commit a8ee600e7aff).

Thanks,
Michael

Michael Bommarito (2):
  thunderbolt: property: harden XDomain property parser against crafted
    peer
  thunderbolt: test: add KUnit regression tests for XDomain property
    parser

 drivers/thunderbolt/property.c |  67 ++++++++++++++---
 drivers/thunderbolt/test.c     | 127 +++++++++++++++++++++++++++++++++
 2 files changed, 185 insertions(+), 9 deletions(-)

--
2.53.0

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] thunderbolt: property: harden XDomain property parser against crafted peer
  2026-04-15  3:23 [PATCH 0/2] thunderbolt: harden XDomain property parser Michael Bommarito
@ 2026-04-15  3:23 ` Michael Bommarito
  2026-04-15  4:52   ` Mika Westerberg
  2026-04-15  3:23 ` [PATCH 2/2] thunderbolt: test: add KUnit regression tests for XDomain property parser Michael Bommarito
  2026-04-15 12:32 ` [PATCH v2 0/4] thunderbolt: harden " Michael Bommarito
  2 siblings, 1 reply; 10+ messages in thread
From: Michael Bommarito @ 2026-04-15  3:23 UTC (permalink / raw)
  To: linux-usb, Mika Westerberg
  Cc: Andreas Noever, Yehezkel Bernat, Greg Kroah-Hartman, linux-kernel,
	stable

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()
before PCIe tunnel authorization.

Bug A - u32 overflow in tb_property_entry_valid() (property.c:61).

    if (entry->value + entry->length > block_len)
            return false;

entry->value is u32, entry->length is u16.  The sum is computed in
u32 and wraps.  With block_len = 500 an attacker picks
value = 0xFFFFFF00, length = 0x100; the u32 sum 0x100000000 wraps
to 0, passing the > block_len check.  tb_property_parse() then
runs

    parse_dwdata(property->value.data, block + entry->value,
                 entry->length);

where block is const u32 * and entry->value is promoted to size_t
for the pointer arithmetic.  The read source is block +
(u64)value * 4, tens of GiB past the property-block allocation.
Up to entry->length * 4 bytes are read from there into a freshly
kcalloc'd property->value.data or property->value.text buffer.

Exfiltration path for TB_PROPERTY_TYPE_TEXT on the "deviceid" or
"vendorid" keys: populate_properties() (xdomain.c:1157,1162) runs
kstrdup(p->value.text, ...) into xd->device_name / xd->vendor_name,
which are read back via the per-XDomain device_name and vendor_name
sysfs attributes (xdomain.c:1730, 1763).  kstrdup stops at the
first NUL byte in the OOB region, so the usable leak is the prefix
up to the first zero byte at an attacker-chosen offset past the
property block.  The attacker does not know block's KASLR/slab
placement, so the read is untargeted in absolute terms - they pick
a delta, not an address.  There is no generic "properties" sysfs
blob; DATA-typed properties are parsed into property->value.data
but never generically surfaced to userspace, so only the TEXT path
with the two named keys is exfil-reachable.  NUL-bounded,
untargeted, but still an attacker-directed OOB read.

Replace the u32 addition with check_add_overflow() so a wrapped
sum is rejected.

Bug B - unbounded recursion in __tb_property_parse_dir().

A DIRECTORY entry's value field is used as dir_offset for a
recursive call with no depth counter.  A peer that crafts a back-
reference chain drives the parser until the 16 KiB kernel stack
is exhausted and the guard page fires - pre-authentication remote
DoS.  Bound the recursion to TB_PROPERTY_MAX_DEPTH = 8,
comfortably larger than any legitimate XDomain property layout.

Bug C - size_t underflow on dir_len - 4 (property.c:184).

    content_offset = dir_offset + 4;
    content_len = dir_len - 4; /* Length includes UUID */

dir_len arrives as a size_t sourced from entry->length (u16) on
the non-root path.  If entry->length < 4, the subtraction
underflows size_t to ~SIZE_MAX, nentries becomes SIZE_MAX / 4,
and the loop walks entries past the property block on each
iteration, reading OOB until either an entry fails validation or
the kernel oopses on an unmapped page.  Reject dir_len < 4
explicitly on the non-root path.

Additional hardening: move INIT_LIST_HEAD(&dir->properties) to
immediately after dir allocation so every error-return path that
calls tb_property_free_dir() (including the new dir_len path and
the pre-existing dir->uuid alloc-failure path at property.c:180)
sees a walkable empty list rather than the zero-initialized NULL
next/prev that would oops list_for_each_entry_safe().

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

Attacker model: malicious Thunderbolt/USB4 XDomain peer (cable,
dock, in-line inspector, adjacent host).  Discovery runs as soon
as the link is trained; PCIe tunnel authorization does not gate
the control-plane PROPERTIES_REQUEST/RESPONSE path, and the host
IOMMU does not mitigate because the data arrives as a control-
plane payload the driver willingly copies into its own buffer
before parsing.

Reproduced on v7.0-rc7 + CONFIG_KASAN=y + CONFIG_USB4_KUNIT_TEST=y
via the companion KUnit suite in the sibling patch.  Pre-fix, each
of the three cases oopses inside __tb_property_parse_dir (Bug A
hits a KASAN shadow-memory fault, Bug B trips the stack guard,
Bug C OOB-reads past the property block).  Post-fix, all three
tests return NULL cleanly and pass.

The parser sites fixed here have not been touched since the
initial 2017 XDomain landing, per git log -p.  property.c has had
three prior fixes by Kangjie Lu in 2019 (106204b56f60,
e4dfdd5804cc, 6183d5a51866) for NULL-check omissions on
kzalloc/kmemdup returns, and a 2025 documentation cleanup by Alan
Borzeszkowski (d015642ad36d); none of those touch the bounds /
arithmetic / recursion sites this patch addresses.  Verified via
lei queries against lore.kernel.org/linux-usb/ for
dfn:drivers/thunderbolt/property.c,
dfhh:tb_property_parse_dir, dfhh:tb_property_entry_valid (0 hits
beyond the doc cleanup); Patchwork linux-usb "thunderbolt
property" query (0 in-flight patches); and Mika Westerberg's
westeri/thunderbolt.git next/fixes/master branches (no pending
bounds work on this file).

Fixes: e69b6c02b4c3 ("thunderbolt: Add functions for parsing and creating XDomain property blocks")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-6
Assisted-by: Codex:gpt-5-4
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 drivers/thunderbolt/property.c | 67 +++++++++++++++++++++++++++++-----
 1 file changed, 58 insertions(+), 9 deletions(-)

diff --git a/drivers/thunderbolt/property.c b/drivers/thunderbolt/property.c
index 50cbfc92fe65..57ec742ed210 100644
--- a/drivers/thunderbolt/property.c
+++ b/drivers/thunderbolt/property.c
@@ -8,11 +8,21 @@
  */
 
 #include <linux/err.h>
+#include <linux/overflow.h>
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/uuid.h>
 #include <linux/thunderbolt.h>
 
+/*
+ * Bounds recursion depth when parsing a malicious XDomain property
+ * block whose DIRECTORY entries are crafted to self-refer.  The
+ * XDomain spec gives no hard limit; 8 is comfortably larger than any
+ * legitimate property layout observed in practice and leaves the
+ * kernel stack headroom.
+ */
+#define TB_PROPERTY_MAX_DEPTH	8
+
 struct tb_property_entry {
 	u32 key_hi;
 	u32 key_lo;
@@ -37,7 +47,7 @@ struct tb_property_dir_entry {
 
 static struct tb_property_dir *__tb_property_parse_dir(const u32 *block,
 	size_t block_len, unsigned int dir_offset, size_t dir_len,
-	bool is_root);
+	bool is_root, unsigned int depth);
 
 static inline void parse_dwdata(void *dst, const void *src, size_t dwords)
 {
@@ -52,13 +62,23 @@ static inline void format_dwdata(void *dst, const void *src, size_t dwords)
 static bool tb_property_entry_valid(const struct tb_property_entry *entry,
 				  size_t block_len)
 {
+	u32 end;
+
 	switch (entry->type) {
 	case TB_PROPERTY_TYPE_DIRECTORY:
 	case TB_PROPERTY_TYPE_DATA:
 	case TB_PROPERTY_TYPE_TEXT:
 		if (entry->length > block_len)
 			return false;
-		if (entry->value + entry->length > block_len)
+		/*
+		 * entry->value is u32 and entry->length is u16; the sum is
+		 * performed in u32 and wraps for crafted inputs.  Use an
+		 * overflow-aware check so a wrapped sum is rejected instead
+		 * of appearing to satisfy the bound.
+		 */
+		if (check_add_overflow(entry->value, (u32)entry->length, &end))
+			return false;
+		if (end > block_len)
 			return false;
 		break;
 
@@ -93,7 +113,8 @@ tb_property_alloc(const char *key, enum tb_property_type type)
 }
 
 static struct tb_property *tb_property_parse(const u32 *block, size_t block_len,
-					const struct tb_property_entry *entry)
+					const struct tb_property_entry *entry,
+					unsigned int depth)
 {
 	char key[TB_PROPERTY_KEY_SIZE + 1];
 	struct tb_property *property;
@@ -114,7 +135,8 @@ static struct tb_property *tb_property_parse(const u32 *block, size_t block_len,
 	switch (property->type) {
 	case TB_PROPERTY_TYPE_DIRECTORY:
 		dir = __tb_property_parse_dir(block, block_len, entry->value,
-					      entry->length, false);
+					      entry->length, false,
+					      depth + 1);
 		if (!dir) {
 			kfree(property);
 			return NULL;
@@ -159,16 +181,33 @@ static struct tb_property *tb_property_parse(const u32 *block, size_t block_len,
 }
 
 static struct tb_property_dir *__tb_property_parse_dir(const u32 *block,
-	size_t block_len, unsigned int dir_offset, size_t dir_len, bool is_root)
+	size_t block_len, unsigned int dir_offset, size_t dir_len, bool is_root,
+	unsigned int depth)
 {
 	const struct tb_property_entry *entries;
 	size_t i, content_len, nentries;
 	unsigned int content_offset;
 	struct tb_property_dir *dir;
 
+	/*
+	 * A malicious XDomain peer can craft DIRECTORY entries whose
+	 * offsets point back at their own container, making the recursion
+	 * unbounded without this gate.
+	 */
+	if (depth > TB_PROPERTY_MAX_DEPTH)
+		return NULL;
+
 	dir = kzalloc_obj(*dir);
 	if (!dir)
 		return NULL;
+	/*
+	 * Initialize the list head immediately so every error-return path
+	 * that calls tb_property_free_dir() (the new dir_len reject and
+	 * the existing uuid-alloc failure path) sees a walkable empty
+	 * list rather than the zero-initialized NULL next/prev that
+	 * would oops list_for_each_entry_safe().
+	 */
+	INIT_LIST_HEAD(&dir->properties);
 
 	if (is_root) {
 		content_offset = dir_offset + 2;
@@ -181,18 +220,28 @@ static struct tb_property_dir *__tb_property_parse_dir(const u32 *block,
 			return NULL;
 		}
 		content_offset = dir_offset + 4;
+		/*
+		 * dir_len arrives here as the u16 entry->length widened to
+		 * size_t; values below 4 underflow size_t on the subtraction
+		 * below and produce a gigantic content_len, driving the
+		 * nentries loop off the block with OOB reads on each
+		 * iteration.
+		 */
+		if (dir_len < 4) {
+			tb_property_free_dir(dir);
+			return NULL;
+		}
 		content_len = dir_len - 4; /* Length includes UUID */
 	}
 
 	entries = (const struct tb_property_entry *)&block[content_offset];
 	nentries = content_len / (sizeof(*entries) / 4);
 
-	INIT_LIST_HEAD(&dir->properties);
-
 	for (i = 0; i < nentries; i++) {
 		struct tb_property *property;
 
-		property = tb_property_parse(block, block_len, &entries[i]);
+		property = tb_property_parse(block, block_len, &entries[i],
+					     depth);
 		if (!property) {
 			tb_property_free_dir(dir);
 			return NULL;
@@ -231,7 +280,7 @@ struct tb_property_dir *tb_property_parse_dir(const u32 *block,
 		return NULL;
 
 	return __tb_property_parse_dir(block, block_len, 0, rootdir->length,
-				       true);
+				       true, 0);
 }
 
 /**
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] thunderbolt: test: add KUnit regression tests for XDomain property parser
  2026-04-15  3:23 [PATCH 0/2] thunderbolt: harden XDomain property parser Michael Bommarito
  2026-04-15  3:23 ` [PATCH 1/2] thunderbolt: property: harden XDomain property parser against crafted peer Michael Bommarito
@ 2026-04-15  3:23 ` Michael Bommarito
  2026-04-15 12:32 ` [PATCH v2 0/4] thunderbolt: harden " Michael Bommarito
  2 siblings, 0 replies; 10+ messages in thread
From: Michael Bommarito @ 2026-04-15  3:23 UTC (permalink / raw)
  To: linux-usb, Mika Westerberg
  Cc: Andreas Noever, Yehezkel Bernat, Greg Kroah-Hartman, linux-kernel,
	stable

Add three KUnit cases that exercise the defects fixed by the parent
commit by feeding crafted XDomain property blocks to
tb_property_parse_dir():

  tb_test_property_parse_u32_wrap - entry->value = 0xFFFFFF00 and
    entry->length = 0x100 so their u32 sum 0x100000000 wraps to 0
    under the block_len guard; without the fix the subsequent
    parse_dwdata() reads attacker-directed OOB memory.

  tb_test_property_parse_recursion - two DIRECTORY entries pointing
    at each other, driving __tb_property_parse_dir() recursion;
    without the fix the kernel stack is exhausted.

  tb_test_property_parse_dir_len_underflow - a DIRECTORY entry with
    length < 4 so non-root content_len = dir_len - 4 wraps size_t;
    without the fix nentries is huge and the entry walk runs OOB.

Each test asserts tb_property_parse_dir() returns NULL on the
crafted input.  With CONFIG_KASAN=y, running these on the pre-fix
kernel reproduces an oops inside __tb_property_parse_dir (KASAN
shadow-memory fault for the u32_wrap case, stack-guard trip for
recursion, OOB read past block for dir_len underflow).  Post-fix
they pass cleanly.

Run with:
  ./tools/testing/kunit/kunit.py run --arch=x86_64 \\
    --kconfig_add CONFIG_PCI=y --kconfig_add CONFIG_NVMEM=y \\
    --kconfig_add CONFIG_USB4=y --kconfig_add CONFIG_USB4_KUNIT_TEST=y \\
    --kconfig_add CONFIG_KASAN=y 'thunderbolt.tb_test_property_parse_*'

Assisted-by: Claude:claude-opus-4-6
Assisted-by: Codex:gpt-5-4
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 drivers/thunderbolt/test.c | 127 +++++++++++++++++++++++++++++++++++++
 1 file changed, 127 insertions(+)

diff --git a/drivers/thunderbolt/test.c b/drivers/thunderbolt/test.c
index 1f4318249c22..22f4107fcb8d 100644
--- a/drivers/thunderbolt/test.c
+++ b/drivers/thunderbolt/test.c
@@ -2852,7 +2852,134 @@ static void tb_test_property_copy(struct kunit *test)
 	tb_property_free_dir(src);
 }
 
+/*
+ * Reproducers for three memory-safety defects in
+ * drivers/thunderbolt/property.c reached from a crafted XDomain
+ * PROPERTIES_RESPONSE payload.  Without the fix these trip KASAN or
+ * smash the kernel stack; with the fix each returns NULL cleanly.
+ */
+static void tb_test_property_parse_u32_wrap(struct kunit *test)
+{
+	u32 *block = kunit_kzalloc(test, 500 * sizeof(u32), GFP_KERNEL);
+	struct tb_property_dir *dir;
+	struct {
+		u32 key_hi, key_lo;
+		u16 length;
+		u8 reserved;
+		u8 type;
+		u32 value;
+	} *e;
+
+	/* Root header: magic + length=6 (single entry body of 4 dwords +
+	 * 2 slack, keeps walk within block[]). */
+	block[0] = 0x55584401;
+	block[1] = 6;
+
+	/* Crafted DATA entry at block[2..5]: value = 0xFFFFFF00 and
+	 * length = 0x100 are u32/u16 such that the u32 sum 0x100000000
+	 * wraps to 0, passing the sum <= block_len guard even though
+	 * the real offset is block + 0xFFFFFF00 * 4 (~16 GiB past the
+	 * block).  The subsequent parse_dwdata() at property.c:132
+	 * copies entry->length*4 = 1024 bytes from that wild address
+	 * into a fresh kcalloc buffer.
+	 */
+	e = (void *)&block[2];
+	e->key_hi = 0x61616161;
+	e->key_lo = 0x61616161;
+	e->length = 0x100;
+	e->type   = 0x64;		/* TB_PROPERTY_TYPE_DATA */
+	e->value  = 0xFFFFFF00;
+
+	dir = tb_property_parse_dir(block, 500);
+	/* With the fix this returns NULL; without it, KASAN splats in
+	 * be32_to_cpu_array() / memcpy reading block + value*4 out of
+	 * bounds.  Assert on the safe outcome: a NULL dir. */
+	KUNIT_EXPECT_NULL(test, dir);
+	tb_property_free_dir(dir);
+}
+
+static void tb_test_property_parse_recursion(struct kunit *test)
+{
+	u32 *block = kunit_kzalloc(test, 500 * sizeof(u32), GFP_KERNEL);
+	struct tb_property_dir *dir;
+	struct entry {
+		u32 key_hi, key_lo;
+		u16 length;
+		u8 reserved;
+		u8 type;
+		u32 value;
+	} *e, *child_e;
+
+	block[0] = 0x55584401;
+	block[1] = 4;		/* rootdir length = one entry */
+
+	/* DIRECTORY entry pointing at dir_offset=2 with length=16.
+	 * When parsed as non-root: content_offset = 6, content_len = 12,
+	 * nentries = 3.  The child's first entry at block[6] is also
+	 * DIRECTORY pointing at 2, so the recursion oscillates between
+	 * two dir_offsets until the kernel stack is exhausted.
+	 */
+	e = (void *)&block[2];
+	e->key_hi = 0x61616161;
+	e->key_lo = 0x61616161;
+	e->length = 16;
+	e->type   = 0x44;		/* TB_PROPERTY_TYPE_DIRECTORY */
+	e->value  = 2;
+
+	child_e = (void *)&block[6];
+	child_e->key_hi = 0x62626262;
+	child_e->key_lo = 0x62626262;
+	child_e->length = 16;
+	child_e->type   = 0x44;
+	child_e->value  = 2;
+
+	dir = tb_property_parse_dir(block, 500);
+	/* With the fix this returns NULL at TB_PROPERTY_MAX_DEPTH (8).
+	 * Without it, the kernel stack-guard fires ~50-80 frames in
+	 * and the kunit thread oopses. */
+	KUNIT_EXPECT_NULL(test, dir);
+	tb_property_free_dir(dir);
+}
+
+static void tb_test_property_parse_dir_len_underflow(struct kunit *test)
+{
+	u32 *block = kunit_kzalloc(test, 500 * sizeof(u32), GFP_KERNEL);
+	struct tb_property_dir *dir;
+	struct entry {
+		u32 key_hi, key_lo;
+		u16 length;
+		u8 reserved;
+		u8 type;
+		u32 value;
+	} *e;
+
+	block[0] = 0x55584401;
+	block[1] = 4;
+
+	/* DIRECTORY entry with length=3.  When parsed as non-root,
+	 * content_len = dir_len - 4 underflows size_t to ~SIZE_MAX,
+	 * nentries = SIZE_MAX/4.  The for-loop walks entries past the
+	 * block, reading OOB on each iteration.
+	 */
+	e = (void *)&block[2];
+	e->key_hi = 0x61616161;
+	e->key_lo = 0x61616161;
+	e->length = 3;
+	e->type   = 0x44;
+	e->value  = 6;
+
+	dir = tb_property_parse_dir(block, 500);
+	/* With the fix: NULL.  Without: KASAN splat on
+	 * block[content_offset + i*4] for i > 124 (past the 500-dword
+	 * block). */
+	KUNIT_EXPECT_NULL(test, dir);
+	tb_property_free_dir(dir);
+}
+
 static struct kunit_case tb_test_cases[] = {
+	KUNIT_CASE(tb_test_property_parse_u32_wrap),
+	KUNIT_CASE(tb_test_property_parse_recursion),
+	KUNIT_CASE(tb_test_property_parse_dir_len_underflow),
 	KUNIT_CASE(tb_test_path_basic),
 	KUNIT_CASE(tb_test_path_not_connected_walk),
 	KUNIT_CASE(tb_test_path_single_hop_walk),
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] thunderbolt: property: harden XDomain property parser against crafted peer
  2026-04-15  3:23 ` [PATCH 1/2] thunderbolt: property: harden XDomain property parser against crafted peer Michael Bommarito
@ 2026-04-15  4:52   ` Mika Westerberg
  2026-04-15 11:41     ` Michael Bommarito
  0 siblings, 1 reply; 10+ messages in thread
From: Mika Westerberg @ 2026-04-15  4:52 UTC (permalink / raw)
  To: Michael Bommarito
  Cc: linux-usb, Mika Westerberg, Andreas Noever, Yehezkel Bernat,
	Greg Kroah-Hartman, linux-kernel, stable

Hi,

On Tue, Apr 14, 2026 at 11:23:34PM -0400, Michael Bommarito wrote:
> 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()
> before PCIe tunnel authorization.

There is no PCIe tunnel authorization happening here.

> Bug A - u32 overflow in tb_property_entry_valid() (property.c:61).
> 
>     if (entry->value + entry->length > block_len)
>             return false;

Please split this patch into 3 patches that all deal with one issue at the
time.

> 
> entry->value is u32, entry->length is u16.  The sum is computed in
> u32 and wraps.  With block_len = 500 an attacker picks
> value = 0xFFFFFF00, length = 0x100; the u32 sum 0x100000000 wraps
> to 0, passing the > block_len check.  tb_property_parse() then
> runs
> 
>     parse_dwdata(property->value.data, block + entry->value,
>                  entry->length);
> 
> where block is const u32 * and entry->value is promoted to size_t
> for the pointer arithmetic.  The read source is block +
> (u64)value * 4, tens of GiB past the property-block allocation.
> Up to entry->length * 4 bytes are read from there into a freshly
> kcalloc'd property->value.data or property->value.text buffer.
> 
> Exfiltration path for TB_PROPERTY_TYPE_TEXT on the "deviceid" or
> "vendorid" keys: populate_properties() (xdomain.c:1157,1162) runs
> kstrdup(p->value.text, ...) into xd->device_name / xd->vendor_name,
> which are read back via the per-XDomain device_name and vendor_name
> sysfs attributes (xdomain.c:1730, 1763).  kstrdup stops at the
> first NUL byte in the OOB region, so the usable leak is the prefix
> up to the first zero byte at an attacker-chosen offset past the
> property block.  The attacker does not know block's KASLR/slab
> placement, so the read is untargeted in absolute terms - they pick
> a delta, not an address.  There is no generic "properties" sysfs
> blob; DATA-typed properties are parsed into property->value.data
> but never generically surfaced to userspace, so only the TEXT path
> with the two named keys is exfil-reachable.  NUL-bounded,
> untargeted, but still an attacker-directed OOB read.
> 
> Replace the u32 addition with check_add_overflow() so a wrapped
> sum is rejected.
> 
> Bug B - unbounded recursion in __tb_property_parse_dir().
> 
> A DIRECTORY entry's value field is used as dir_offset for a
> recursive call with no depth counter.  A peer that crafts a back-
> reference chain drives the parser until the 16 KiB kernel stack
> is exhausted and the guard page fires - pre-authentication remote
> DoS.  Bound the recursion to TB_PROPERTY_MAX_DEPTH = 8,
> comfortably larger than any legitimate XDomain property layout.
> 
> Bug C - size_t underflow on dir_len - 4 (property.c:184).
> 
>     content_offset = dir_offset + 4;
>     content_len = dir_len - 4; /* Length includes UUID */
> 
> dir_len arrives as a size_t sourced from entry->length (u16) on
> the non-root path.  If entry->length < 4, the subtraction
> underflows size_t to ~SIZE_MAX, nentries becomes SIZE_MAX / 4,
> and the loop walks entries past the property block on each
> iteration, reading OOB until either an entry fails validation or
> the kernel oopses on an unmapped page.  Reject dir_len < 4
> explicitly on the non-root path.
> 
> Additional hardening: move INIT_LIST_HEAD(&dir->properties) to
> immediately after dir allocation so every error-return path that
> calls tb_property_free_dir() (including the new dir_len path and
> the pre-existing dir->uuid alloc-failure path at property.c:180)
> sees a walkable empty list rather than the zero-initialized NULL
> next/prev that would oops list_for_each_entry_safe().
> 
> All three defects are OOB-read plus DoS class.  No controlled OOB
> write is reachable through the parser; parse_dwdata's destination
> is a freshly kcalloc'd buffer sized by entry->length.
> 
> Attacker model: malicious Thunderbolt/USB4 XDomain peer (cable,
> dock, in-line inspector, adjacent host).  Discovery runs as soon
> as the link is trained; PCIe tunnel authorization does not gate
> the control-plane PROPERTIES_REQUEST/RESPONSE path, and the host
> IOMMU does not mitigate because the data arrives as a control-
> plane payload the driver willingly copies into its own buffer
> before parsing.

Here too there is not PCIe tunnel. It's a tunnel but not PCIe.

Also you can disable whole XDomain stuff with passing "xdomain=0" in the
kernel command line.

> Reproduced on v7.0-rc7 + CONFIG_KASAN=y + CONFIG_USB4_KUNIT_TEST=y
> via the companion KUnit suite in the sibling patch.  Pre-fix, each
> of the three cases oopses inside __tb_property_parse_dir (Bug A
> hits a KASAN shadow-memory fault, Bug B trips the stack guard,
> Bug C OOB-reads past the property block).  Post-fix, all three
> tests return NULL cleanly and pass.
> 
> The parser sites fixed here have not been touched since the
> initial 2017 XDomain landing, per git log -p.  property.c has had
> three prior fixes by Kangjie Lu in 2019 (106204b56f60,
> e4dfdd5804cc, 6183d5a51866) for NULL-check omissions on
> kzalloc/kmemdup returns, and a 2025 documentation cleanup by Alan
> Borzeszkowski (d015642ad36d); none of those touch the bounds /
> arithmetic / recursion sites this patch addresses.  Verified via
> lei queries against lore.kernel.org/linux-usb/ for
> dfn:drivers/thunderbolt/property.c,
> dfhh:tb_property_parse_dir, dfhh:tb_property_entry_valid (0 hits
> beyond the doc cleanup); Patchwork linux-usb "thunderbolt
> property" query (0 in-flight patches); and Mika Westerberg's
> westeri/thunderbolt.git next/fixes/master branches (no pending
> bounds work on this file).

Also please tune the commit messages you get from the AI. This one is easy
to read from running git log so no need to duplicate here.

> Fixes: e69b6c02b4c3 ("thunderbolt: Add functions for parsing and creating XDomain property blocks")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-4-6
> Assisted-by: Codex:gpt-5-4
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> ---
>  drivers/thunderbolt/property.c | 67 +++++++++++++++++++++++++++++-----
>  1 file changed, 58 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/thunderbolt/property.c b/drivers/thunderbolt/property.c
> index 50cbfc92fe65..57ec742ed210 100644
> --- a/drivers/thunderbolt/property.c
> +++ b/drivers/thunderbolt/property.c
> @@ -8,11 +8,21 @@
>   */
>  
>  #include <linux/err.h>
> +#include <linux/overflow.h>
>  #include <linux/slab.h>
>  #include <linux/string.h>
>  #include <linux/uuid.h>
>  #include <linux/thunderbolt.h>
>  
> +/*
> + * Bounds recursion depth when parsing a malicious XDomain property
> + * block whose DIRECTORY entries are crafted to self-refer.  The
> + * XDomain spec gives no hard limit; 8 is comfortably larger than any
> + * legitimate property layout observed in practice and leaves the
> + * kernel stack headroom.
> + */
> +#define TB_PROPERTY_MAX_DEPTH	8
> +
>  struct tb_property_entry {
>  	u32 key_hi;
>  	u32 key_lo;
> @@ -37,7 +47,7 @@ struct tb_property_dir_entry {
>  
>  static struct tb_property_dir *__tb_property_parse_dir(const u32 *block,
>  	size_t block_len, unsigned int dir_offset, size_t dir_len,
> -	bool is_root);
> +	bool is_root, unsigned int depth);
>  
>  static inline void parse_dwdata(void *dst, const void *src, size_t dwords)
>  {
> @@ -52,13 +62,23 @@ static inline void format_dwdata(void *dst, const void *src, size_t dwords)
>  static bool tb_property_entry_valid(const struct tb_property_entry *entry,
>  				  size_t block_len)
>  {
> +	u32 end;
> +
>  	switch (entry->type) {
>  	case TB_PROPERTY_TYPE_DIRECTORY:
>  	case TB_PROPERTY_TYPE_DATA:
>  	case TB_PROPERTY_TYPE_TEXT:
>  		if (entry->length > block_len)
>  			return false;
> -		if (entry->value + entry->length > block_len)
> +		/*
> +		 * entry->value is u32 and entry->length is u16; the sum is
> +		 * performed in u32 and wraps for crafted inputs.  Use an
> +		 * overflow-aware check so a wrapped sum is rejected instead
> +		 * of appearing to satisfy the bound.
> +		 */

Also please tidy up this comment.

> +		if (check_add_overflow(entry->value, (u32)entry->length, &end))
> +			return false;
> +		if (end > block_len)
>  			return false;
>  		break;
>  
> @@ -93,7 +113,8 @@ tb_property_alloc(const char *key, enum tb_property_type type)
>  }
>  
>  static struct tb_property *tb_property_parse(const u32 *block, size_t block_len,
> -					const struct tb_property_entry *entry)
> +					const struct tb_property_entry *entry,
> +					unsigned int depth)
>  {
>  	char key[TB_PROPERTY_KEY_SIZE + 1];
>  	struct tb_property *property;
> @@ -114,7 +135,8 @@ static struct tb_property *tb_property_parse(const u32 *block, size_t block_len,
>  	switch (property->type) {
>  	case TB_PROPERTY_TYPE_DIRECTORY:
>  		dir = __tb_property_parse_dir(block, block_len, entry->value,
> -					      entry->length, false);
> +					      entry->length, false,
> +					      depth + 1);
>  		if (!dir) {
>  			kfree(property);
>  			return NULL;
> @@ -159,16 +181,33 @@ static struct tb_property *tb_property_parse(const u32 *block, size_t block_len,
>  }
>  
>  static struct tb_property_dir *__tb_property_parse_dir(const u32 *block,
> -	size_t block_len, unsigned int dir_offset, size_t dir_len, bool is_root)
> +	size_t block_len, unsigned int dir_offset, size_t dir_len, bool is_root,
> +	unsigned int depth)
>  {
>  	const struct tb_property_entry *entries;
>  	size_t i, content_len, nentries;
>  	unsigned int content_offset;
>  	struct tb_property_dir *dir;
>  
> +	/*
> +	 * A malicious XDomain peer can craft DIRECTORY entries whose
> +	 * offsets point back at their own container, making the recursion
> +	 * unbounded without this gate.
> +	 */

This comment is useless.

> +	if (depth > TB_PROPERTY_MAX_DEPTH)
> +		return NULL;
> +
>  	dir = kzalloc_obj(*dir);
>  	if (!dir)
>  		return NULL;
> +	/*
> +	 * Initialize the list head immediately so every error-return path
> +	 * that calls tb_property_free_dir() (the new dir_len reject and
> +	 * the existing uuid-alloc failure path) sees a walkable empty
> +	 * list rather than the zero-initialized NULL next/prev that
> +	 * would oops list_for_each_entry_safe().
> +	 */

So is this.

> +	INIT_LIST_HEAD(&dir->properties);
>  
>  	if (is_root) {
>  		content_offset = dir_offset + 2;
> @@ -181,18 +220,28 @@ static struct tb_property_dir *__tb_property_parse_dir(const u32 *block,
>  			return NULL;
>  		}
>  		content_offset = dir_offset + 4;
> +		/*
> +		 * dir_len arrives here as the u16 entry->length widened to
> +		 * size_t; values below 4 underflow size_t on the subtraction
> +		 * below and produce a gigantic content_len, driving the
> +		 * nentries loop off the block with OOB reads on each
> +		 * iteration.
> +		 */

and this.

> +		if (dir_len < 4) {
> +			tb_property_free_dir(dir);
> +			return NULL;
> +		}
>  		content_len = dir_len - 4; /* Length includes UUID */
>  	}
>  
>  	entries = (const struct tb_property_entry *)&block[content_offset];
>  	nentries = content_len / (sizeof(*entries) / 4);
>  
> -	INIT_LIST_HEAD(&dir->properties);
> -
>  	for (i = 0; i < nentries; i++) {
>  		struct tb_property *property;
>  
> -		property = tb_property_parse(block, block_len, &entries[i]);
> +		property = tb_property_parse(block, block_len, &entries[i],
> +					     depth);
>  		if (!property) {
>  			tb_property_free_dir(dir);
>  			return NULL;
> @@ -231,7 +280,7 @@ struct tb_property_dir *tb_property_parse_dir(const u32 *block,
>  		return NULL;
>  
>  	return __tb_property_parse_dir(block, block_len, 0, rootdir->length,
> -				       true);
> +				       true, 0);
>  }
>  
>  /**
> -- 
> 2.53.0

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] thunderbolt: property: harden XDomain property parser against crafted peer
  2026-04-15  4:52   ` Mika Westerberg
@ 2026-04-15 11:41     ` Michael Bommarito
  0 siblings, 0 replies; 10+ messages in thread
From: Michael Bommarito @ 2026-04-15 11:41 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: linux-usb, Mika Westerberg, Andreas Noever, Yehezkel Bernat,
	Greg Kroah-Hartman, linux-kernel, stable

On Wed, Apr 15, 2026 at 12:52 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> Please split this patch into 3 patches that all deal with one issue at the
> time.

Will do.  Sorry for the verbosity and bus confusion!  Look for another version
shortly.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 0/4] thunderbolt: harden XDomain property parser
  2026-04-15  3:23 [PATCH 0/2] thunderbolt: harden XDomain property parser Michael Bommarito
  2026-04-15  3:23 ` [PATCH 1/2] thunderbolt: property: harden XDomain property parser against crafted peer Michael Bommarito
  2026-04-15  3:23 ` [PATCH 2/2] thunderbolt: test: add KUnit regression tests for XDomain property parser Michael Bommarito
@ 2026-04-15 12:32 ` Michael Bommarito
  2026-04-15 12:32   ` [PATCH v2 1/4] thunderbolt: property: reject u32 wrap in tb_property_entry_valid() Michael Bommarito
                     ` (3 more replies)
  2 siblings, 4 replies; 10+ messages in thread
From: Michael Bommarito @ 2026-04-15 12:32 UTC (permalink / raw)
  To: linux-usb, Mika Westerberg
  Cc: Andreas Noever, Yehezkel Bernat, Greg Kroah-Hartman, linux-kernel,
	stable

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(), size_t underflow on dir_len < 4 in
__tb_property_parse_dir(), and unbounded recursion in the same.
Patch 4 is three KUnit regression cases exercising all three.
Let me know if you want me to pair the KUnit cases with each
patch instead.

My assessment is that 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.

As Mika noted, 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, each case oopses inside
__tb_property_parse_dir (KASAN shadow-memory fault for u32-wrap,
stack-guard trip for recursion, OOB read past block for dir_len
underflow).  Post-fix, all three pass without issue.

Changes since v1
----------------

v1 -> v2, addressing Mika's review (msgid
20260415045246.GR3552@black.igk.intel.com):

  - Split the single property.c hardening patch into three, one per
    bug, ordered smallest-diff-first (u32 wrap, then dir_len
    underflow, then recursion cap).  [Mika]
  - Removed the incorrect "PCIe tunnel authorization" framing from
    the commit messages and cover letter.  XDomain discovery runs
    before any tunnel is set up; the path is not PCIe-specific.
    [Mika]
  - Added an explicit operator mitigation note
    (thunderbolt.xdomain=0).  [Mika]
  - Trimmed the commit messages: dropped the per-file prior-fix
    enumeration (Kangjie Lu 2019 series, Alan Borzeszkowski 2025
    cleanup) and the lei / Patchwork / westeri-tree scoop-check
    provenance notes; that content is available via git log -p and
    does not belong in the commit message.  [Mika]
  - Dropped the long inline block comments above check_add_overflow(),
    the TB_PROPERTY_MAX_DEPTH check, the INIT_LIST_HEAD reorder, and
    the dir_len < 4 reject; the code is self-explanatory given the
    commit message.  [Mika]
  - Reworded the recursion DoS description away from "remote" (this
    is a peer-triggered DoS reachable from any adjacent XDomain peer
    over the Thunderbolt/USB4 bus, not network-reachable).
  - KUnit patch unchanged in content; commit message adjusted to say
    "sibling commits" rather than "parent commit" now that the series
    has multiple parent fixes.

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     | 127 +++++++++++++++++++++++++++++++++
 2 files changed, 150 insertions(+), 9 deletions(-)

--
2.53.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/4] thunderbolt: property: reject u32 wrap in tb_property_entry_valid()
  2026-04-15 12:32 ` [PATCH v2 0/4] thunderbolt: harden " Michael Bommarito
@ 2026-04-15 12:32   ` Michael Bommarito
  2026-04-15 12:32   ` [PATCH v2 2/4] thunderbolt: property: reject dir_len < 4 to prevent size_t underflow Michael Bommarito
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Michael Bommarito @ 2026-04-15 12:32 UTC (permalink / raw)
  To: linux-usb, Mika Westerberg
  Cc: Andreas Noever, Yehezkel Bernat, Greg Kroah-Hartman, linux-kernel,
	stable

entry->value is u32 and entry->length is u16; the sum is performed in
u32 and wraps.  A malicious XDomain peer can pick
value = 0xFFFFFF00, length = 0x100 so the sum 0x100000000 wraps to 0
and passes the > block_len check.  tb_property_parse() then passes
entry->value to parse_dwdata() as a dword offset into the property
block, reading attacker-directed memory far past the allocation.

For TEXT-typed entries with the "deviceid" or "vendorid" keys this
lands in xd->device_name / xd->vendor_name and is readable back via
the per-XDomain device_name / vendor_name sysfs attributes; the leak
is NUL-bounded (kstrdup() stops at the first zero byte) and
untargeted (the attacker picks a delta, not an absolute address).
DATA-typed entries are parsed into property->value.data but not
generically surfaced to userspace.

Use check_add_overflow() so a wrapped sum is rejected.

Fixes: e69b6c02b4c3 ("thunderbolt: Add functions for parsing and creating XDomain property blocks")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-6
Assisted-by: Codex:gpt-5-4
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 drivers/thunderbolt/property.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/property.c b/drivers/thunderbolt/property.c
index 50cbfc92fe65..f5ee8f531300 100644
--- a/drivers/thunderbolt/property.c
+++ b/drivers/thunderbolt/property.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/err.h>
+#include <linux/overflow.h>
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/uuid.h>
@@ -52,13 +53,16 @@ static inline void format_dwdata(void *dst, const void *src, size_t dwords)
 static bool tb_property_entry_valid(const struct tb_property_entry *entry,
 				  size_t block_len)
 {
+	u32 end;
+
 	switch (entry->type) {
 	case TB_PROPERTY_TYPE_DIRECTORY:
 	case TB_PROPERTY_TYPE_DATA:
 	case TB_PROPERTY_TYPE_TEXT:
 		if (entry->length > block_len)
 			return false;
-		if (entry->value + entry->length > block_len)
+		if (check_add_overflow(entry->value, (u32)entry->length, &end) ||
+		    end > block_len)
 			return false;
 		break;
 
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/4] thunderbolt: property: reject dir_len < 4 to prevent size_t underflow
  2026-04-15 12:32 ` [PATCH v2 0/4] thunderbolt: harden " Michael Bommarito
  2026-04-15 12:32   ` [PATCH v2 1/4] thunderbolt: property: reject u32 wrap in tb_property_entry_valid() Michael Bommarito
@ 2026-04-15 12:32   ` Michael Bommarito
  2026-04-15 12:32   ` [PATCH v2 3/4] thunderbolt: property: cap recursion depth in __tb_property_parse_dir() Michael Bommarito
  2026-04-15 12:32   ` [PATCH v2 4/4] thunderbolt: test: add KUnit regression tests for XDomain property parser Michael Bommarito
  3 siblings, 0 replies; 10+ messages in thread
From: Michael Bommarito @ 2026-04-15 12:32 UTC (permalink / raw)
  To: linux-usb, Mika Westerberg
  Cc: Andreas Noever, Yehezkel Bernat, Greg Kroah-Hartman, linux-kernel,
	stable

On the non-root path, __tb_property_parse_dir() takes dir_len from
entry->length (u16 widened to size_t) and computes
content_len = dir_len - 4.  If a crafted peer supplies
entry->length < 4, the subtraction underflows size_t to ~SIZE_MAX,
nentries becomes SIZE_MAX / 4, and the entry walk runs off the
property block, reading OOB on each iteration until either an entry
fails validation or the kernel oopses on an unmapped page.

Reject dir_len < 4 explicitly before the subtraction.

Also move INIT_LIST_HEAD(&dir->properties) up to immediately after
the dir allocation so every error-return path that calls
tb_property_free_dir() (the new dir_len reject and the existing
uuid-alloc failure path) sees a walkable list rather than the
zero-initialized NULL next/prev that list_for_each_entry_safe()
would oops on.

Fixes: e69b6c02b4c3 ("thunderbolt: Add functions for parsing and creating XDomain property blocks")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-6
Assisted-by: Codex:gpt-5-4
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 drivers/thunderbolt/property.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/thunderbolt/property.c b/drivers/thunderbolt/property.c
index f5ee8f531300..274b555d27c8 100644
--- a/drivers/thunderbolt/property.c
+++ b/drivers/thunderbolt/property.c
@@ -173,6 +173,7 @@ static struct tb_property_dir *__tb_property_parse_dir(const u32 *block,
 	dir = kzalloc_obj(*dir);
 	if (!dir)
 		return NULL;
+	INIT_LIST_HEAD(&dir->properties);
 
 	if (is_root) {
 		content_offset = dir_offset + 2;
@@ -184,6 +185,10 @@ static struct tb_property_dir *__tb_property_parse_dir(const u32 *block,
 			tb_property_free_dir(dir);
 			return NULL;
 		}
+		if (dir_len < 4) {
+			tb_property_free_dir(dir);
+			return NULL;
+		}
 		content_offset = dir_offset + 4;
 		content_len = dir_len - 4; /* Length includes UUID */
 	}
@@ -191,8 +196,6 @@ static struct tb_property_dir *__tb_property_parse_dir(const u32 *block,
 	entries = (const struct tb_property_entry *)&block[content_offset];
 	nentries = content_len / (sizeof(*entries) / 4);
 
-	INIT_LIST_HEAD(&dir->properties);
-
 	for (i = 0; i < nentries; i++) {
 		struct tb_property *property;
 
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 3/4] thunderbolt: property: cap recursion depth in __tb_property_parse_dir()
  2026-04-15 12:32 ` [PATCH v2 0/4] thunderbolt: harden " Michael Bommarito
  2026-04-15 12:32   ` [PATCH v2 1/4] thunderbolt: property: reject u32 wrap in tb_property_entry_valid() Michael Bommarito
  2026-04-15 12:32   ` [PATCH v2 2/4] thunderbolt: property: reject dir_len < 4 to prevent size_t underflow Michael Bommarito
@ 2026-04-15 12:32   ` Michael Bommarito
  2026-04-15 12:32   ` [PATCH v2 4/4] thunderbolt: test: add KUnit regression tests for XDomain property parser Michael Bommarito
  3 siblings, 0 replies; 10+ messages in thread
From: Michael Bommarito @ 2026-04-15 12:32 UTC (permalink / raw)
  To: linux-usb, Mika Westerberg
  Cc: Andreas Noever, Yehezkel Bernat, Greg Kroah-Hartman, linux-kernel,
	stable

A DIRECTORY entry's value field is used as the dir_offset for a
recursive call into __tb_property_parse_dir() with no depth counter.
A crafted peer that chains DIRECTORY entries into a back-reference
loop drives the parser until the kernel stack is exhausted and the
guard page fires.  Any untrusted XDomain peer (cable, dock, in-line
inspector, adjacent host) that reaches the PROPERTIES_REQUEST
control-plane exchange can trigger this without authentication.

Thread a depth counter through tb_property_parse() and
__tb_property_parse_dir(), and reject blocks that exceed
TB_PROPERTY_MAX_DEPTH = 8.  That is comfortably larger than any
observed legitimate XDomain layout.

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

Fixes: e69b6c02b4c3 ("thunderbolt: Add functions for parsing and creating XDomain property blocks")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-6
Assisted-by: Codex:gpt-5-4
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 drivers/thunderbolt/property.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/thunderbolt/property.c b/drivers/thunderbolt/property.c
index 274b555d27c8..99ee7089456c 100644
--- a/drivers/thunderbolt/property.c
+++ b/drivers/thunderbolt/property.c
@@ -35,10 +35,11 @@ struct tb_property_dir_entry {
 };
 
 #define TB_PROPERTY_ROOTDIR_MAGIC	0x55584401
+#define TB_PROPERTY_MAX_DEPTH		8
 
 static struct tb_property_dir *__tb_property_parse_dir(const u32 *block,
 	size_t block_len, unsigned int dir_offset, size_t dir_len,
-	bool is_root);
+	bool is_root, unsigned int depth);
 
 static inline void parse_dwdata(void *dst, const void *src, size_t dwords)
 {
@@ -97,7 +98,8 @@ tb_property_alloc(const char *key, enum tb_property_type type)
 }
 
 static struct tb_property *tb_property_parse(const u32 *block, size_t block_len,
-					const struct tb_property_entry *entry)
+					const struct tb_property_entry *entry,
+					unsigned int depth)
 {
 	char key[TB_PROPERTY_KEY_SIZE + 1];
 	struct tb_property *property;
@@ -118,7 +120,7 @@ static struct tb_property *tb_property_parse(const u32 *block, size_t block_len,
 	switch (property->type) {
 	case TB_PROPERTY_TYPE_DIRECTORY:
 		dir = __tb_property_parse_dir(block, block_len, entry->value,
-					      entry->length, false);
+					      entry->length, false, depth + 1);
 		if (!dir) {
 			kfree(property);
 			return NULL;
@@ -163,13 +165,17 @@ static struct tb_property *tb_property_parse(const u32 *block, size_t block_len,
 }
 
 static struct tb_property_dir *__tb_property_parse_dir(const u32 *block,
-	size_t block_len, unsigned int dir_offset, size_t dir_len, bool is_root)
+	size_t block_len, unsigned int dir_offset, size_t dir_len, bool is_root,
+	unsigned int depth)
 {
 	const struct tb_property_entry *entries;
 	size_t i, content_len, nentries;
 	unsigned int content_offset;
 	struct tb_property_dir *dir;
 
+	if (depth > TB_PROPERTY_MAX_DEPTH)
+		return NULL;
+
 	dir = kzalloc_obj(*dir);
 	if (!dir)
 		return NULL;
@@ -199,7 +205,8 @@ static struct tb_property_dir *__tb_property_parse_dir(const u32 *block,
 	for (i = 0; i < nentries; i++) {
 		struct tb_property *property;
 
-		property = tb_property_parse(block, block_len, &entries[i]);
+		property = tb_property_parse(block, block_len, &entries[i],
+					     depth);
 		if (!property) {
 			tb_property_free_dir(dir);
 			return NULL;
@@ -238,7 +245,7 @@ struct tb_property_dir *tb_property_parse_dir(const u32 *block,
 		return NULL;
 
 	return __tb_property_parse_dir(block, block_len, 0, rootdir->length,
-				       true);
+				       true, 0);
 }
 
 /**
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 4/4] thunderbolt: test: add KUnit regression tests for XDomain property parser
  2026-04-15 12:32 ` [PATCH v2 0/4] thunderbolt: harden " Michael Bommarito
                     ` (2 preceding siblings ...)
  2026-04-15 12:32   ` [PATCH v2 3/4] thunderbolt: property: cap recursion depth in __tb_property_parse_dir() Michael Bommarito
@ 2026-04-15 12:32   ` Michael Bommarito
  3 siblings, 0 replies; 10+ messages in thread
From: Michael Bommarito @ 2026-04-15 12:32 UTC (permalink / raw)
  To: linux-usb, Mika Westerberg
  Cc: Andreas Noever, Yehezkel Bernat, Greg Kroah-Hartman, linux-kernel,
	stable

Add three KUnit cases that exercise the defects fixed by the sibling
commits in this series by feeding crafted XDomain property blocks to
tb_property_parse_dir():

  tb_test_property_parse_u32_wrap - entry->value = 0xFFFFFF00 and
    entry->length = 0x100 so their u32 sum 0x100000000 wraps to 0
    under the block_len guard; without the fix the subsequent
    parse_dwdata() reads attacker-directed OOB memory.

  tb_test_property_parse_recursion - two DIRECTORY entries pointing
    at each other, driving __tb_property_parse_dir() recursion;
    without the fix the kernel stack is exhausted.

  tb_test_property_parse_dir_len_underflow - a DIRECTORY entry with
    length < 4 so non-root content_len = dir_len - 4 wraps size_t;
    without the fix nentries is huge and the entry walk runs OOB.

Each test asserts tb_property_parse_dir() returns NULL on the
crafted input.  With CONFIG_KASAN=y, running these on the pre-fix
kernel reproduces an oops inside __tb_property_parse_dir (KASAN
shadow-memory fault for the u32_wrap case, stack-guard trip for
recursion, OOB read past block for dir_len underflow).  Post-fix
they pass cleanly.

Run with:
  ./tools/testing/kunit/kunit.py run --arch=x86_64 \\
    --kconfig_add CONFIG_PCI=y --kconfig_add CONFIG_NVMEM=y \\
    --kconfig_add CONFIG_USB4=y --kconfig_add CONFIG_USB4_KUNIT_TEST=y \\
    --kconfig_add CONFIG_KASAN=y 'thunderbolt.tb_test_property_parse_*'

Assisted-by: Claude:claude-opus-4-6
Assisted-by: Codex:gpt-5-4
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 drivers/thunderbolt/test.c | 127 +++++++++++++++++++++++++++++++++++++
 1 file changed, 127 insertions(+)

diff --git a/drivers/thunderbolt/test.c b/drivers/thunderbolt/test.c
index 1f4318249c22..22f4107fcb8d 100644
--- a/drivers/thunderbolt/test.c
+++ b/drivers/thunderbolt/test.c
@@ -2852,7 +2852,134 @@ static void tb_test_property_copy(struct kunit *test)
 	tb_property_free_dir(src);
 }
 
+/*
+ * Reproducers for three memory-safety defects in
+ * drivers/thunderbolt/property.c reached from a crafted XDomain
+ * PROPERTIES_RESPONSE payload.  Without the fix these trip KASAN or
+ * smash the kernel stack; with the fix each returns NULL cleanly.
+ */
+static void tb_test_property_parse_u32_wrap(struct kunit *test)
+{
+	u32 *block = kunit_kzalloc(test, 500 * sizeof(u32), GFP_KERNEL);
+	struct tb_property_dir *dir;
+	struct {
+		u32 key_hi, key_lo;
+		u16 length;
+		u8 reserved;
+		u8 type;
+		u32 value;
+	} *e;
+
+	/* Root header: magic + length=6 (single entry body of 4 dwords +
+	 * 2 slack, keeps walk within block[]). */
+	block[0] = 0x55584401;
+	block[1] = 6;
+
+	/* Crafted DATA entry at block[2..5]: value = 0xFFFFFF00 and
+	 * length = 0x100 are u32/u16 such that the u32 sum 0x100000000
+	 * wraps to 0, passing the sum <= block_len guard even though
+	 * the real offset is block + 0xFFFFFF00 * 4 (~16 GiB past the
+	 * block).  The subsequent parse_dwdata() at property.c:132
+	 * copies entry->length*4 = 1024 bytes from that wild address
+	 * into a fresh kcalloc buffer.
+	 */
+	e = (void *)&block[2];
+	e->key_hi = 0x61616161;
+	e->key_lo = 0x61616161;
+	e->length = 0x100;
+	e->type   = 0x64;		/* TB_PROPERTY_TYPE_DATA */
+	e->value  = 0xFFFFFF00;
+
+	dir = tb_property_parse_dir(block, 500);
+	/* With the fix this returns NULL; without it, KASAN splats in
+	 * be32_to_cpu_array() / memcpy reading block + value*4 out of
+	 * bounds.  Assert on the safe outcome: a NULL dir. */
+	KUNIT_EXPECT_NULL(test, dir);
+	tb_property_free_dir(dir);
+}
+
+static void tb_test_property_parse_recursion(struct kunit *test)
+{
+	u32 *block = kunit_kzalloc(test, 500 * sizeof(u32), GFP_KERNEL);
+	struct tb_property_dir *dir;
+	struct entry {
+		u32 key_hi, key_lo;
+		u16 length;
+		u8 reserved;
+		u8 type;
+		u32 value;
+	} *e, *child_e;
+
+	block[0] = 0x55584401;
+	block[1] = 4;		/* rootdir length = one entry */
+
+	/* DIRECTORY entry pointing at dir_offset=2 with length=16.
+	 * When parsed as non-root: content_offset = 6, content_len = 12,
+	 * nentries = 3.  The child's first entry at block[6] is also
+	 * DIRECTORY pointing at 2, so the recursion oscillates between
+	 * two dir_offsets until the kernel stack is exhausted.
+	 */
+	e = (void *)&block[2];
+	e->key_hi = 0x61616161;
+	e->key_lo = 0x61616161;
+	e->length = 16;
+	e->type   = 0x44;		/* TB_PROPERTY_TYPE_DIRECTORY */
+	e->value  = 2;
+
+	child_e = (void *)&block[6];
+	child_e->key_hi = 0x62626262;
+	child_e->key_lo = 0x62626262;
+	child_e->length = 16;
+	child_e->type   = 0x44;
+	child_e->value  = 2;
+
+	dir = tb_property_parse_dir(block, 500);
+	/* With the fix this returns NULL at TB_PROPERTY_MAX_DEPTH (8).
+	 * Without it, the kernel stack-guard fires ~50-80 frames in
+	 * and the kunit thread oopses. */
+	KUNIT_EXPECT_NULL(test, dir);
+	tb_property_free_dir(dir);
+}
+
+static void tb_test_property_parse_dir_len_underflow(struct kunit *test)
+{
+	u32 *block = kunit_kzalloc(test, 500 * sizeof(u32), GFP_KERNEL);
+	struct tb_property_dir *dir;
+	struct entry {
+		u32 key_hi, key_lo;
+		u16 length;
+		u8 reserved;
+		u8 type;
+		u32 value;
+	} *e;
+
+	block[0] = 0x55584401;
+	block[1] = 4;
+
+	/* DIRECTORY entry with length=3.  When parsed as non-root,
+	 * content_len = dir_len - 4 underflows size_t to ~SIZE_MAX,
+	 * nentries = SIZE_MAX/4.  The for-loop walks entries past the
+	 * block, reading OOB on each iteration.
+	 */
+	e = (void *)&block[2];
+	e->key_hi = 0x61616161;
+	e->key_lo = 0x61616161;
+	e->length = 3;
+	e->type   = 0x44;
+	e->value  = 6;
+
+	dir = tb_property_parse_dir(block, 500);
+	/* With the fix: NULL.  Without: KASAN splat on
+	 * block[content_offset + i*4] for i > 124 (past the 500-dword
+	 * block). */
+	KUNIT_EXPECT_NULL(test, dir);
+	tb_property_free_dir(dir);
+}
+
 static struct kunit_case tb_test_cases[] = {
+	KUNIT_CASE(tb_test_property_parse_u32_wrap),
+	KUNIT_CASE(tb_test_property_parse_recursion),
+	KUNIT_CASE(tb_test_property_parse_dir_len_underflow),
 	KUNIT_CASE(tb_test_path_basic),
 	KUNIT_CASE(tb_test_path_not_connected_walk),
 	KUNIT_CASE(tb_test_path_single_hop_walk),
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2026-04-15 12:32 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-15  3:23 [PATCH 0/2] thunderbolt: harden XDomain property parser Michael Bommarito
2026-04-15  3:23 ` [PATCH 1/2] thunderbolt: property: harden XDomain property parser against crafted peer Michael Bommarito
2026-04-15  4:52   ` Mika Westerberg
2026-04-15 11:41     ` Michael Bommarito
2026-04-15  3:23 ` [PATCH 2/2] thunderbolt: test: add KUnit regression tests for XDomain property parser Michael Bommarito
2026-04-15 12:32 ` [PATCH v2 0/4] thunderbolt: harden " Michael Bommarito
2026-04-15 12:32   ` [PATCH v2 1/4] thunderbolt: property: reject u32 wrap in tb_property_entry_valid() Michael Bommarito
2026-04-15 12:32   ` [PATCH v2 2/4] thunderbolt: property: reject dir_len < 4 to prevent size_t underflow Michael Bommarito
2026-04-15 12:32   ` [PATCH v2 3/4] thunderbolt: property: cap recursion depth in __tb_property_parse_dir() Michael Bommarito
2026-04-15 12:32   ` [PATCH v2 4/4] thunderbolt: test: add KUnit regression tests for XDomain property parser Michael Bommarito

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox