thunderbolt: Do not enumerate more ports from DROM than the controller has
PATCH 1/1 thunderbolt: Do not enumerate more ports from DROM than the controller has
Some Alpine Ridge LP DROMs (there might be others) erroneusly list more ports than the controller actually has. Most probably because DROM of the full Dual/Single port Thunderbolt controller was reused for LP version. The current DROM parser does not check the upper bound thus it leads to crash when sw->ports[] is accessed over bounds:
BUG: unable to handle kernel NULL pointer dereference at 00000000000002ec IP: tb_drom_read+0x383/0x890 [thunderbolt] PGD 0 P4D 0 Oops: 0000 [#1] SMP CPU: 3 PID: 12248 Comm: systemd-udevd Not tainted 4.13.0-rc1-next-20170719 #1 Hardware name: LENOVO 20HF000YGE/20HF000YGE, BIOS N1WET32W (1.11 ) 05/23/2017 task: ffff8a293e4bcd80 task.stack: ffffa698027a8000 RIP: 0010:tb_drom_read+0x383/0x890 [thunderbolt] RSP: 0018:ffffa698027ab990 EFLAGS: 00010246 RAX: 0000000000000000 RBX: ffff8a2940af7800 RCX: 0000000000000000 RDX: ffff8a2940ebb400 RSI: 0000000000000000 RDI: ffffa698027ab9a0 RBP: ffffa698027ab9d0 R08: 0000000000000001 R09: 0000000000000002 R10: ffff8a2940ebb5b0 R11: 0000000000000000 R12: ffff8a293bfa968c R13: 000000000000002c R14: 0000000000000056 R15: 0000000000000056 FS: 00007f0a945a38c0(0000) GS:ffff8a2961580000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00000000000002ec CR3: 000000043e785000 CR4: 00000000003606e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: tb_switch_add+0x9d/0x730 [thunderbolt] ? tb_switch_alloc+0x3cd/0x4d0 [thunderbolt] icm_start+0x5a/0xa0 [thunderbolt] tb_domain_add+0xc3/0xf0 [thunderbolt] nhi_probe+0x19e/0x310 [thunderbolt] local_pci_probe+0x42/0xa0 pci_device_probe+0x18d/0x1a0 driver_probe_device+0x2ff/0x450 __driver_attach+0xa4/0xe0 ? driver_probe_device+0x450/0x450 bus_for_each_dev+0x6e/0xb0 driver_attach+0x1e/0x20 bus_add_driver+0x1d0/0x270 ? 0xffffffffc0bbb000 driver_register+0x60/0xe0 ? 0xffffffffc0bbb000 __pci_register_driver+0x4c/0x50 nhi_init+0x28/0x1000 [thunderbolt] do_one_initcall+0x50/0x190 ? __vunmap+0x81/0xb0 ? _cond_resched+0x1a/0x50 ? kmem_cache_alloc_trace+0x15f/0x1c0 ? do_init_module+0x27/0x1e9 do_init_module+0x5f/0x1e9 load_module+0x24e7/0x2a60 ? vfs_read+0x115/0x130 SYSC_finit_module+0xfc/0x120 ? SYSC_finit_module+0xfc/0x120 SyS_finit_module+0xe/0x10 do_syscall_64+0x67/0x170 entry_SYSCALL64_slow_path+0x25/0x25
Fix this bu making sure we only enumerate DROM port entries the hardware actually has.
--- drivers/thunderbolt/eeprom.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
drivers/thunderbolt/eeprom.c +10 -0
@@ -333,6 +333,16 @@ static int tb_drom_parse_entry_port(struct tb_switch *sw,int res;enum tb_port_type type; +/* +* Some DROMs list more ports than the controller actually has +* so we skip those but allow the parser to continue. +*/ +if (header->index > sw->config.max_port_number) { +dev_warn_once (&sw->dev,"DROM has too many port entries %u (expected %u)\n" , + header->index, sw->config.max_port_number); +return 0 ; + } + port = &sw->ports[header->index]; port->disabled = header->port_disabled;if (port->disabled)
Signature · 1 line
2.13.2
Quoted 1 line · Mika Westerberg
Fix this bu making sure we only enumerate DROM port entries the hardware
^^
Yehezkel pointed out that there is a typo in the above line. Should be "by" instead. Let me know if you want me to send an updated version.
Quoted 5 lines · Mika Westerberg
Quoted 1 line · Mika Westerberg
Fix this bu making sure we only enumerate DROM port entries the hardware
^^Yehezkel pointed out that there is a typo in the above line. Should be "by" instead. Let me know if you want me to send an updated version.
A maintainer should never have to edit a patch by hand... :)
drivers/thunderbolt/eeprom.c static int tb_drom_parse_entry_port(struct tb_switch *sw, · line 345
Quoted 10 lines · Mika Westerberg
drivers/thunderbolt/eeprom.c +10
-0+/* +* Some DROMs list more ports than the controller actually has +* so we skip those but allow the parser to continue. +*/ +if (header->index > sw->config.max_port_number) { +dev_warn_once (&sw->dev,"DROM has too many port entries %u (expected %u)\n" , + header->index, sw->config.max_port_number); +return 0 ; + } +
I wouldn't have gotten into bikeshedding here but since Greg is indicating he'd like a repost:
Could you tone down the error to KERN_INFO, it seems harmless and the user will see this on every boot even though they may not be able to do anything about it, short of flashing the EEPROM on the Thunderbolt controller which may not be supported by the vendor.
Also, you're now only reporting the first index of additional unwanted entries, which isn't really helpful. And max_port_number is already reported upon allocation of the switch. I suggest removing the two %u and just reporting the existence of additional superfluous port entries in the Device ROM and that they're being ignored (e.g. "ignoring unnecessary extra entries in DROM").
Apart from these nits,
Thanks for the report and quick fix!
Lukas