Re: [PATCH 06/19] thunderbolt: Try reading host DROM from device tree first

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Integer underflow in `tb_drom_parse_entry_generic` allows an attacker-controlled Device Tree or DROM to trigger an unbounded kernel memory read.
- [High] Inadequate buffer allocation size in DROM copy paths allows out-of-bounds reads during header parsing, leading to a heap information leak.
--

commit 579084ce1fd77c747852aa3d8eca1094e3c9a9c1
Author: Sven Peter <sven@kernel.org>

thunderbolt: Try reading host DROM from device tree first

On Apple Silicon SoCs the DROM is provided by a device tree property.
This commit updates the subsystem to try reading from that property first
before falling back to other methods.

> diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
> index 2a13fa6888ba1..389de7df84f65 100644
> --- a/drivers/thunderbolt/eeprom.c
> +++ b/drivers/thunderbolt/eeprom.c
[ ... ]
> @@ -500,6 +499,22 @@ static int tb_drom_copy_efi(struct tb_switch *sw, u16 *size)
>  	return -EINVAL;
>  }
>  
> +/*
> + * tb_drom_copy_of_apple - copy drom supplied by the device tree for Apple Silicon to sw->drom.
> + */
> +static int tb_drom_copy_of_apple(struct tb_switch *sw, u16 *size)
> +{
> +	return tb_drom_copy_property(sw, "apple,thunderbolt-drom", size);
> +}

[Severity: Critical]
This is a pre-existing issue, but does this logic leave us vulnerable to an
unbounded kernel memory read if an attacker-controlled Device Tree or malicious
Thunderbolt device provides a crafted DROM?

In drivers/thunderbolt/eeprom.c:tb_drom_parse_entries(), the validation
incorrectly permits an entry with length 1 to bypass the length checks:

	if (pos + 1 == drom_size || pos + entry->len > drom_size
			|| !entry->len) {
		tb_sw_warn(sw, "DROM buffer overrun\n");

If entry->len is 1, pos + 1 > drom_size evaluates to false, proceeding to
parsing. In tb_drom_parse_entry_generic(), this causes an integer underflow:

	case 1:
		/* Length includes 2 bytes header so remove it before copy */
		sw->vendor_name = kstrndup(entry->data,
			header->len - sizeof(*header), GFP_KERNEL);

Since sizeof(*header) is 2, header->len - sizeof(*header) evaluates to
1 - 2 = -1, which is promoted to SIZE_MAX in kstrndup().

Could this cause an immediate kernel panic when strnlen() hits an unmapped
page?

> +
> +/*
> + * tb_drom_copy_efi - copy drom supplied by EFI to sw->drom if present
> + */
> +static int tb_drom_copy_efi(struct tb_switch *sw, u16 *size)
> +{
> +	return tb_drom_copy_property(sw, "ThunderboltDROM", size);
> +}
> +
>  static int tb_drom_copy_nvm(struct tb_switch *sw, u16 *size)

[Severity: High]
This isn't a bug introduced by this patch, but does the DROM copy path allocate
an undersized buffer that could lead to a heap information leak?

When reading DROM, drivers/thunderbolt/eeprom.c:tb_drom_copy_nvm() (and
similarly usb4_copy_drom()) calculates the size based on a potentially small
DROM data length:

	/* Size includes CRC8 + UID + CRC32 */
	*size += 1 + 8 + 4;
	ret = tb_switch_drom_alloc(sw, *size);

If the data_len is between 0 and 8 (for example, 3), it allocates an undersized
buffer of 16 bytes, which is smaller than sizeof(struct tb_drom_header), which
is 22 bytes.

Later, tb_drom_parse() performs an out-of-bounds read on the undersized buffer
at byte 13:

	tb_sw_dbg(sw, "DROM version: %d\n", header->device_rom_revision);

Additionally, tb_drom_parse_v1() performs out-of-bounds reads at byte offsets
16-19:

	sw->vendor = header->vendor_id;

Could this reliably leak 4 bytes of adjacent heap memory from kmalloc-16 to
userspace via sysfs when sw->vendor and sw->device are exposed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830-b4-apple-soc-tbt-v1-0-44bc9348683c@kernel.org?part=6