[PATCH] thunderbolt: Do not enumerate more ports from DROM than the controller has

[PATCH] thunderbolt: Do not enumerate more ports from DROM than the controller has

[Mika Westerberg]

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 <ckellner@redhat.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 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

Re: [PATCH] thunderbolt: Do not enumerate more ports from DROM than the controller has

[Mika Westerberg]

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.

Re: [PATCH] thunderbolt: Do not enumerate more ports from DROM than the controller has

[Greg Kroah-Hartman]

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... :)

Re: [PATCH] thunderbolt: Do not enumerate more ports from DROM than the controller has

[Lukas Wunner]

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 <lukas@wunner.de>

Thanks for the report and quick fix!

Lukas