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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
+
+/*
+ * 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