* [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
* 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 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
* [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