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.
Reported-by: Christian Kellner <[email protected]>
Signed-off-by: Mika Westerberg <[email protected]>
---
drivers/thunderbolt/eeprom.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
index 308b6e17c88a..8b63632037da 100644
--- a/drivers/thunderbolt/eeprom.c
+++ b/drivers/thunderbolt/eeprom.c
@@ -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)
--
2.13.2
On Tue, Jul 25, 2017 at 03:06:47PM +0300, Mika Westerberg wrote:
> 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.
On Tue, Jul 25, 2017 at 03:20:51PM +0300, Mika Westerberg wrote:
> On Tue, Jul 25, 2017 at 03:06:47PM +0300, Mika Westerberg wrote:
> > 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... :)
On Tue, Jul 25, 2017 at 03:06:47PM +0300, Mika Westerberg wrote:
> + /*
> + * 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,
Reviewed-by: Lukas Wunner <[email protected]>
Thanks for the report and quick fix!
Lukas