Re: [PATCH 1/2] thunderbolt: property: harden XDomain property parser against crafted peer

[Mika Westerberg <mika.westerberg@linux.intel.com>]

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