diff mbox series

NULL pointer dereferences in hid-mcp2221

Message ID 79152feb-bcbc-9e3e-e776-13170ae4ef40@vigem.de
State New
Headers show
Series NULL pointer dereferences in hid-mcp2221 | expand

Commit Message

Sven Zühlsdorf Oct. 25, 2022, 7:39 a.m. UTC 
Hi,

I've run into two NULL pointer dereferences when loading the MCP2221 driver.
Initially I observed them running the kernel used by yocto kirkstone
(currently 5.15.68) but can reproduce them with a vanilla 6.1-rc1 as well.
All line numbers below are for hid-mcp2221.c, taken from 6.1-rc1.

The first one was easy to identify, in mcp2221_probe line 874 `hdev->hidraw`
was NULL since I compiled the kernel without CONFIG_HIDRAW enabled. Should
CONFIG_HID_MCP2221 perhaps depend on or imply CONFIG_HIDRAW?

For the second one however, I'm stuck. The dereference happens at the top of
mcp_smbus_xfer since i2c_get_adapdata in line 307 returned NULL. Looking at
the call trace (see [dmesg output] below), mcp_smbus_xfer gets called
indirectly from mcp2221_probe via i2c_add_adapter in line 876, directly
before a call to i2c_set_adapdata. Since I couldn't identify another call to
i2c_set_adapdata or a write to `mcp->adapter.dev.driver_data` that could
potentially have initialized that field, I attempted to swap the order of
calling i2c_set_adapdata and i2c_add_adapter, see [attempted fix] below. While
the driver now loads without issue, no devices appear on the i2c bus:
# i2cdetect -l
i2c-0   smbus           SMBus I801 adapter at 5000              SMBus adapter
i2c-1   i2c             MCP2221 usb-i2c bridge on hidraw0       I2C adapter
# i2cdetect -y 1
      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:                         -- -- -- -- -- -- -- -- 
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
70: -- -- -- -- -- -- -- --


When booting a distribution kernel (Ubunutu 22.04 with kernel 5.15.39) the bus
is populated as expected:
# i2cdetect -l
i2c-0   smbus           SMBus CMI adapter cmi                   SMBus adapter
i2c-1   smbus           SMBus iSMT adapter at 20fffab9000       SMBus adapter
i2c-2   smbus           SMBus I801 adapter at 5000              SMBus adapter
i2c-3   i2c             MCP2221 usb-i2c bridge on hidraw0       I2C adapter
# i2cdetect -y 3
      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:                         -- -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
20: 20 -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
30: -- -- -- -- -- -- -- -- -- -- -- 3b -- -- -- --
40: -- -- -- -- -- -- -- -- UU -- -- -- UU -- -- --
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
70: 70 -- -- -- -- -- -- --

In the patches applied by Ubuntu I couldn't find anything that'd explain this
change in behavior. Regarding their kernel configuration the situation is
similar: Even incorporating almost all of Ubuntu's additions (minus some
signing and integrity stuff) into the config I've been using results in the
NULL pointer dereference in mcp_smbus_xfer. Since this still might be a case of
me missing some config options, I'll post my config in a response to this mail.


There seem to be two recent patch series for this driver:
https://lore.kernel.org/all/20221001005208.8010-1-matt.ranostay@konsulko.com/
https://lore.kernel.org/all/20220926202239.16379-1-Enrik.Berkhan@inka.de/
I tested both, but the behavior stays the same.

[attempted fix]

[dmesg output]
Shortened to just the trace, I'll post the full dmesg output in a response to
this mail.
[    1.898107] usb 1-3.4: new full-speed USB device number 5 using xhci_hcd
[    1.990607] cdc_acm 1-3.4:1.0: ttyACM1: USB ACM device
[    1.991509] hid-generic 0003:04D8:00DD.0003: hiddev96,hidraw2: USB HID v1.11 Device [Microchip Technology Inc. MCP2221 USB-I2C/UART Combo] on usb-0000:00:1e.0-3.4/input2
[    1.998390] mcp2221 0003:04D8:00DD.0003: hidraw2: USB HID v1.11 Device [Microchip Technology Inc. MCP2221 USB-I2C/UART Combo] on usb-0000:00:1e.0-3.4/input2
[    2.050149] i2c_dev: adapter [MCP2221 usb-i2c bridge on hidraw2] registered as minor 1
[    2.050166] i2c i2c-1: adapter [MCP2221 usb-i2c bridge on hidraw2] registered
[    2.050173] i2c i2c-1: found normal entry for adapter 1, addr 0x48
[    2.050179] BUG: kernel NULL pointer dereference, address: 0000000000000000
[    2.057180] #PF: supervisor read access in kernel mode
[    2.062347] #PF: error_code(0x0000) - not-present page
[    2.067517] PGD 0 P4D 0 
[    2.070069] Oops: 0000 [#1] PREEMPT SMP
[    2.073931] CPU: 7 PID: 287 Comm: systemd-udevd Not tainted 6.1.0-rc1-yocto-standard-upstream #1
[    2.082761] Hardware name: Default string Default string/Default string, BIOS 1.01.10.AR01 08/05/2022
[    2.092025] RIP: 0010:mcp_smbus_xfer+0x29/0x348 [hid_mcp2221]
[    2.097811] Code: 00 0f 1f 44 00 00 55 48 89 e5 41 57 41 56 41 55 45 89 cd 41 54 45 89 c4 53 48 83 ec 08 48 8b 9f e8 00 00 00 89 75 d4 89 4d d0 <48> 8b 3b 48 8b 87 b8 1b 00 00 48 8b 40 20 48 85 c0 74 07 be 20 00
[    2.116673] RSP: 0018:ffffa6c4409ab748 EFLAGS: 00010296
[    2.121929] RAX: ffffffffc0509fc0 RBX: 0000000000000000 RCX: 0000000000000000
[    2.129104] RDX: 0000000000000000 RSI: 0000000000000048 RDI: ffff9957c69b5830
[    2.136278] RBP: ffffa6c4409ab778 R08: 0000000000000000 R09: 0000000000000000
[    2.143450] R10: ffff995b3fede000 R11: ffff995b3ff8e000 R12: 0000000000000000
[    2.150625] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[    2.157797] FS:  00007f5b4ee65600(0000) GS:ffff995b301c0000(0000) knlGS:0000000000000000
[    2.165932] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    2.171710] CR2: 0000000000000000 CR3: 0000000108c9b002 CR4: 0000000000770ee0
[    2.178884] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[    2.186059] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[    2.193232] PKRU: 55555554
[    2.195959] Call Trace:
[    2.198423]  <TASK>
[    2.200540]  __i2c_smbus_xfer+0x105/0x3d0
[    2.204578]  ? mcp_i2c_xfer+0x160/0x160 [hid_mcp2221]
[    2.209663]  i2c_smbus_xfer+0x62/0xe0
[    2.213350]  i2c_default_probe+0xf1/0x130
[    2.217384]  i2c_detect.isra.0.cold+0xf4/0x220
[    2.229989]  ? kernfs_add_one+0xe8/0x130
[    2.233941]  ? preempt_count_add+0x7a/0xc0
[    2.238063]  ? _raw_spin_lock_irqsave+0x1e/0x50
[    2.242623]  ? __process_new_driver+0x30/0x30
[    2.247007]  __process_new_adapter+0x18/0x20
[    2.251304]  bus_for_each_drv+0x82/0xd0
[    2.255167]  i2c_register_adapter.cold+0x133/0x1f4
[    2.259990]  i2c_add_adapter+0x5c/0x80
[    2.263764]  mcp2221_probe+0x138/0x250 [hid_mcp2221]
[    2.268762]  ? hid_lookup_quirk+0x7f/0x190
[    2.272884]  hid_device_probe+0xf5/0x160
[    2.285748]  really_probe+0xdf/0x290
[    2.298964]  ? pm_runtime_barrier+0x55/0x90
[    2.311392]  __driver_probe_device+0x78/0xe0
[    2.323310]  driver_probe_device+0x24/0xe0
[    2.327431]  __device_attach_driver+0x7d/0x100
[    2.337243]  ? driver_allows_async_probing+0x60/0x60
[    2.348108]  bus_for_each_drv+0x82/0xd0
[    2.351969]  ? __hid_register_driver+0x90/0x90
[    2.356439]  __device_attach+0xc1/0x1f0
[    2.360302]  ? __hid_register_driver+0x90/0x90
[    2.364773]  device_attach+0x10/0x20
[    2.368372]  device_reprobe+0x4a/0x90
[    2.372059]  __hid_bus_reprobe_drivers+0x56/0x60
[    2.376704]  bus_for_each_dev+0x7c/0xc0
[    2.386344]  ? hid_destroy_device+0x60/0x60
[    2.390554]  __hid_bus_driver_added+0x2c/0x40
[    2.394937]  bus_for_each_drv+0x82/0xd0
[    2.398800]  __hid_register_driver+0x7d/0x90
[    2.403096]  ? 0xffffffffc07c2000
[    2.406434]  mcp2221_driver_init+0x23/0x1000 [hid_mcp2221]
[    2.411955]  do_one_initcall+0x4f/0x210
[    2.415817]  ? kmalloc_trace+0x2a/0xa0
[    2.419593]  do_init_module+0x52/0x200
[    2.423367]  load_module+0x1a78/0x1c50
[    2.427142]  __do_sys_finit_module+0xbb/0x110
[    2.431525]  ? __do_sys_finit_module+0xbb/0x110
[    2.436086]  __x64_sys_finit_module+0x18/0x20
[    2.440469]  do_syscall_64+0x3d/0x90
[    2.444070]  entry_SYSCALL_64_after_hwframe+0x46/0xb0
[    2.449154] RIP: 0033:0x7f5b4efc202d
[    2.452755] Code: c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c3 4d 0e 00 f7 d8 64 89 01 48
[    2.471614] RSP: 002b:00007ffe16a04a18 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[    2.479225] RAX: ffffffffffffffda RBX: 00005651bb4c0670 RCX: 00007f5b4efc202d
[    2.479226] RDX: 0000000000000000 RSI: 00007f5b4f13636b RDI: 0000000000000006
[    2.479227] RBP: 00007ffe16a04a40 R08: 0000000000000000 R09: 0000000000000000
[    2.491744] R10: 0000000000000006 R11: 0000000000000246 R12: 00007f5b4f13636b
[    2.491745] R13: 0000000000000000 R14: 0000000000020000 R15: 00005651bb4c0670
[    2.491748]  </TASK>
[    2.506091] Modules linked in: hid_mcp2221(+) ice(+) igc
[    2.519128]  sch_fq_codel tmp421 configfs
[    2.519132] CR2: 0000000000000000
[    2.528505] ---[ end trace 0000000000000000 ]---
[    2.566155] RIP: 0010:mcp_smbus_xfer+0x29/0x348 [hid_mcp2221]
[    2.587646] Code: 00 0f 1f 44 00 00 55 48 89 e5 41 57 41 56 41 55 45 89 cd 41 54 45 89 c4 53 48 83 ec 08 48 8b 9f e8 00 00 00 89 75 d4 89 4d d0 <48> 8b 3b 48 8b 87 b8 1b 00 00 48 8b 40 20 48 85 c0 74 07 be 20 00
[    2.587648] RSP: 0018:ffffa6c4409ab748 EFLAGS: 00010296
[    2.598855] RAX: ffffffffc0509fc0 RBX: 0000000000000000 RCX: 0000000000000000
[    2.598856] RDX: 0000000000000000 RSI: 0000000000000048 RDI: ffff9957c69b5830
[    2.608406] RBP: ffffa6c4409ab778 R08: 0000000000000000 R09: 0000000000000000
[    2.608407] R10: ffff995b3fede000 R11: ffff995b3ff8e000 R12: 0000000000000000
[    2.608408] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[    2.618830] FS:  00007f5b4ee65600(0000) GS:ffff995b301c0000(0000) knlGS:0000000000000000
[    2.618832] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    2.642332] CR2: 0000000000000000 CR3: 0000000108c9b002 CR4: 0000000000770ee0
[    2.642333] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[    2.642334] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[    2.642335] PKRU: 55555554

Comments

Randy Dunlap Nov. 3, 2022, 12:31 a.m. UTC | #1 
Hi--

[adding linux-input mailing list]

On 10/25/22 00:39, Sven Zühlsdorf wrote:
> Hi,
> 
> I've run into two NULL pointer dereferences when loading the MCP2221 driver.
> Initially I observed them running the kernel used by yocto kirkstone
> (currently 5.15.68) but can reproduce them with a vanilla 6.1-rc1 as well.
> All line numbers below are for hid-mcp2221.c, taken from 6.1-rc1.
> 
> The first one was easy to identify, in mcp2221_probe line 874 `hdev->hidraw`
> was NULL since I compiled the kernel without CONFIG_HIDRAW enabled. Should
> CONFIG_HID_MCP2221 perhaps depend on or imply CONFIG_HIDRAW?

Looks to me like it should. Hopefully the HID people can chime in here.

I can't help with the second issue. Rishi?

> For the second one however, I'm stuck. The dereference happens at the top of
> mcp_smbus_xfer since i2c_get_adapdata in line 307 returned NULL. Looking at
> the call trace (see [dmesg output] below), mcp_smbus_xfer gets called
> indirectly from mcp2221_probe via i2c_add_adapter in line 876, directly
> before a call to i2c_set_adapdata. Since I couldn't identify another call to
> i2c_set_adapdata or a write to `mcp->adapter.dev.driver_data` that could
> potentially have initialized that field, I attempted to swap the order of
> calling i2c_set_adapdata and i2c_add_adapter, see [attempted fix] below. While
> the driver now loads without issue, no devices appear on the i2c bus:
> # i2cdetect -l
> i2c-0   smbus           SMBus I801 adapter at 5000              SMBus adapter
> i2c-1   i2c             MCP2221 usb-i2c bridge on hidraw0       I2C adapter
> # i2cdetect -y 1
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> 00:                         -- -- -- -- -- -- -- -- 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 70: -- -- -- -- -- -- -- --
> 
> 
> When booting a distribution kernel (Ubunutu 22.04 with kernel 5.15.39) the bus
> is populated as expected:
> # i2cdetect -l
> i2c-0   smbus           SMBus CMI adapter cmi                   SMBus adapter
> i2c-1   smbus           SMBus iSMT adapter at 20fffab9000       SMBus adapter
> i2c-2   smbus           SMBus I801 adapter at 5000              SMBus adapter
> i2c-3   i2c             MCP2221 usb-i2c bridge on hidraw0       I2C adapter
> # i2cdetect -y 3
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> 00:                         -- -- -- -- -- -- -- --
> 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 20: 20 -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 30: -- -- -- -- -- -- -- -- -- -- -- 3b -- -- -- --
> 40: -- -- -- -- -- -- -- -- UU -- -- -- UU -- -- --
> 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 70: 70 -- -- -- -- -- -- --
> 
> In the patches applied by Ubuntu I couldn't find anything that'd explain this
> change in behavior. Regarding their kernel configuration the situation is
> similar: Even incorporating almost all of Ubuntu's additions (minus some
> signing and integrity stuff) into the config I've been using results in the
> NULL pointer dereference in mcp_smbus_xfer. Since this still might be a case of
> me missing some config options, I'll post my config in a response to this mail.
> 
> 
> There seem to be two recent patch series for this driver:
> https://lore.kernel.org/all/20221001005208.8010-1-matt.ranostay@konsulko.com/
> https://lore.kernel.org/all/20220926202239.16379-1-Enrik.Berkhan@inka.de/
> I tested both, but the behavior stays the same.
> 
> [attempted fix]
> --- a/drivers/hid/hid-mcp2221.c    2022-09-19 09:31:22.539691089 +0200
> +++ b/drivers/hid/hid-mcp2221.c    2022-09-20 15:21:37.576196331 +0200
> @@ -873,12 +873,22 @@
>              "MCP2221 usb-i2c bridge on hidraw%d",
>              ((struct hidraw *)hdev->hidraw)->minor);
> 
> +    int adapdata_change = 0;
> +    if(!i2c_get_adapdata(&mcp->adapter))
> +    {
> +        adapdata_change = 1;
> +        hid_warn(hdev, "got NULL adapdata\n");
> +        i2c_set_adapdata(&mcp->adapter, mcp);
> +    }
>      ret = i2c_add_adapter(&mcp->adapter);
>      if (ret) {
> +        if(adapdata_change)
> +        {
> +            i2c_set_adapdata(&mcp->adapter, NULL);
> +        }
>          hid_err(hdev, "can't add usb-i2c adapter: %d\n", ret);
>          goto err_i2c;
>      }
> -    i2c_set_adapdata(&mcp->adapter, mcp);
> 
>      /* Setup GPIO chip */
>      mcp->gc = devm_kzalloc(&hdev->dev, sizeof(*mcp->gc), GFP_KERNEL);
> 
> [dmesg output]
> Shortened to just the trace, I'll post the full dmesg output in a response to
> this mail.
> [    1.898107] usb 1-3.4: new full-speed USB device number 5 using xhci_hcd
> [    1.990607] cdc_acm 1-3.4:1.0: ttyACM1: USB ACM device
> [    1.991509] hid-generic 0003:04D8:00DD.0003: hiddev96,hidraw2: USB HID v1.11 Device [Microchip Technology Inc. MCP2221 USB-I2C/UART Combo] on usb-0000:00:1e.0-3.4/input2
> [    1.998390] mcp2221 0003:04D8:00DD.0003: hidraw2: USB HID v1.11 Device [Microchip Technology Inc. MCP2221 USB-I2C/UART Combo] on usb-0000:00:1e.0-3.4/input2
> [    2.050149] i2c_dev: adapter [MCP2221 usb-i2c bridge on hidraw2] registered as minor 1
> [    2.050166] i2c i2c-1: adapter [MCP2221 usb-i2c bridge on hidraw2] registered
> [    2.050173] i2c i2c-1: found normal entry for adapter 1, addr 0x48
> [    2.050179] BUG: kernel NULL pointer dereference, address: 0000000000000000
> [    2.057180] #PF: supervisor read access in kernel mode
> [    2.062347] #PF: error_code(0x0000) - not-present page
> [    2.067517] PGD 0 P4D 0 [    2.070069] Oops: 0000 [#1] PREEMPT SMP
> [    2.073931] CPU: 7 PID: 287 Comm: systemd-udevd Not tainted 6.1.0-rc1-yocto-standard-upstream #1
> [    2.082761] Hardware name: Default string Default string/Default string, BIOS 1.01.10.AR01 08/05/2022
> [    2.092025] RIP: 0010:mcp_smbus_xfer+0x29/0x348 [hid_mcp2221]
> [    2.097811] Code: 00 0f 1f 44 00 00 55 48 89 e5 41 57 41 56 41 55 45 89 cd 41 54 45 89 c4 53 48 83 ec 08 48 8b 9f e8 00 00 00 89 75 d4 89 4d d0 <48> 8b 3b 48 8b 87 b8 1b 00 00 48 8b 40 20 48 85 c0 74 07 be 20 00
> [    2.116673] RSP: 0018:ffffa6c4409ab748 EFLAGS: 00010296
> [    2.121929] RAX: ffffffffc0509fc0 RBX: 0000000000000000 RCX: 0000000000000000
> [    2.129104] RDX: 0000000000000000 RSI: 0000000000000048 RDI: ffff9957c69b5830
> [    2.136278] RBP: ffffa6c4409ab778 R08: 0000000000000000 R09: 0000000000000000
> [    2.143450] R10: ffff995b3fede000 R11: ffff995b3ff8e000 R12: 0000000000000000
> [    2.150625] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> [    2.157797] FS:  00007f5b4ee65600(0000) GS:ffff995b301c0000(0000) knlGS:0000000000000000
> [    2.165932] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    2.171710] CR2: 0000000000000000 CR3: 0000000108c9b002 CR4: 0000000000770ee0
> [    2.178884] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [    2.186059] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [    2.193232] PKRU: 55555554
> [    2.195959] Call Trace:
> [    2.198423]  <TASK>
> [    2.200540]  __i2c_smbus_xfer+0x105/0x3d0
> [    2.204578]  ? mcp_i2c_xfer+0x160/0x160 [hid_mcp2221]
> [    2.209663]  i2c_smbus_xfer+0x62/0xe0
> [    2.213350]  i2c_default_probe+0xf1/0x130
> [    2.217384]  i2c_detect.isra.0.cold+0xf4/0x220
> [    2.229989]  ? kernfs_add_one+0xe8/0x130
> [    2.233941]  ? preempt_count_add+0x7a/0xc0
> [    2.238063]  ? _raw_spin_lock_irqsave+0x1e/0x50
> [    2.242623]  ? __process_new_driver+0x30/0x30
> [    2.247007]  __process_new_adapter+0x18/0x20
> [    2.251304]  bus_for_each_drv+0x82/0xd0
> [    2.255167]  i2c_register_adapter.cold+0x133/0x1f4
> [    2.259990]  i2c_add_adapter+0x5c/0x80
> [    2.263764]  mcp2221_probe+0x138/0x250 [hid_mcp2221]
> [    2.268762]  ? hid_lookup_quirk+0x7f/0x190
> [    2.272884]  hid_device_probe+0xf5/0x160
> [    2.285748]  really_probe+0xdf/0x290
> [    2.298964]  ? pm_runtime_barrier+0x55/0x90
> [    2.311392]  __driver_probe_device+0x78/0xe0
> [    2.323310]  driver_probe_device+0x24/0xe0
> [    2.327431]  __device_attach_driver+0x7d/0x100
> [    2.337243]  ? driver_allows_async_probing+0x60/0x60
> [    2.348108]  bus_for_each_drv+0x82/0xd0
> [    2.351969]  ? __hid_register_driver+0x90/0x90
> [    2.356439]  __device_attach+0xc1/0x1f0
> [    2.360302]  ? __hid_register_driver+0x90/0x90
> [    2.364773]  device_attach+0x10/0x20
> [    2.368372]  device_reprobe+0x4a/0x90
> [    2.372059]  __hid_bus_reprobe_drivers+0x56/0x60
> [    2.376704]  bus_for_each_dev+0x7c/0xc0
> [    2.386344]  ? hid_destroy_device+0x60/0x60
> [    2.390554]  __hid_bus_driver_added+0x2c/0x40
> [    2.394937]  bus_for_each_drv+0x82/0xd0
> [    2.398800]  __hid_register_driver+0x7d/0x90
> [    2.403096]  ? 0xffffffffc07c2000
> [    2.406434]  mcp2221_driver_init+0x23/0x1000 [hid_mcp2221]
> [    2.411955]  do_one_initcall+0x4f/0x210
> [    2.415817]  ? kmalloc_trace+0x2a/0xa0
> [    2.419593]  do_init_module+0x52/0x200
> [    2.423367]  load_module+0x1a78/0x1c50
> [    2.427142]  __do_sys_finit_module+0xbb/0x110
> [    2.431525]  ? __do_sys_finit_module+0xbb/0x110
> [    2.436086]  __x64_sys_finit_module+0x18/0x20
> [    2.440469]  do_syscall_64+0x3d/0x90
> [    2.444070]  entry_SYSCALL_64_after_hwframe+0x46/0xb0
> [    2.449154] RIP: 0033:0x7f5b4efc202d
> [    2.452755] Code: c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c3 4d 0e 00 f7 d8 64 89 01 48
> [    2.471614] RSP: 002b:00007ffe16a04a18 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
> [    2.479225] RAX: ffffffffffffffda RBX: 00005651bb4c0670 RCX: 00007f5b4efc202d
> [    2.479226] RDX: 0000000000000000 RSI: 00007f5b4f13636b RDI: 0000000000000006
> [    2.479227] RBP: 00007ffe16a04a40 R08: 0000000000000000 R09: 0000000000000000
> [    2.491744] R10: 0000000000000006 R11: 0000000000000246 R12: 00007f5b4f13636b
> [    2.491745] R13: 0000000000000000 R14: 0000000000020000 R15: 00005651bb4c0670
> [    2.491748]  </TASK>
> [    2.506091] Modules linked in: hid_mcp2221(+) ice(+) igc
> [    2.519128]  sch_fq_codel tmp421 configfs
> [    2.519132] CR2: 0000000000000000
> [    2.528505] ---[ end trace 0000000000000000 ]---
> [    2.566155] RIP: 0010:mcp_smbus_xfer+0x29/0x348 [hid_mcp2221]
> [    2.587646] Code: 00 0f 1f 44 00 00 55 48 89 e5 41 57 41 56 41 55 45 89 cd 41 54 45 89 c4 53 48 83 ec 08 48 8b 9f e8 00 00 00 89 75 d4 89 4d d0 <48> 8b 3b 48 8b 87 b8 1b 00 00 48 8b 40 20 48 85 c0 74 07 be 20 00
> [    2.587648] RSP: 0018:ffffa6c4409ab748 EFLAGS: 00010296
> [    2.598855] RAX: ffffffffc0509fc0 RBX: 0000000000000000 RCX: 0000000000000000
> [    2.598856] RDX: 0000000000000000 RSI: 0000000000000048 RDI: ffff9957c69b5830
> [    2.608406] RBP: ffffa6c4409ab778 R08: 0000000000000000 R09: 0000000000000000
> [    2.608407] R10: ffff995b3fede000 R11: ffff995b3ff8e000 R12: 0000000000000000
> [    2.608408] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> [    2.618830] FS:  00007f5b4ee65600(0000) GS:ffff995b301c0000(0000) knlGS:0000000000000000
> [    2.618832] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    2.642332] CR2: 0000000000000000 CR3: 0000000108c9b002 CR4: 0000000000770ee0
> [    2.642333] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [    2.642334] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [    2.642335] PKRU: 55555554
>
Enrik Berkhan Nov. 3, 2022, 3:22 p.m. UTC | #2 
On Thu, 2022-11-03 at 13:16 +0100, Benjamin Tissoires wrote:
> On Nov 02 2022, Randy Dunlap wrote:
> > Hi--
> > 
> > [adding linux-input mailing list]
> > 
> > On 10/25/22 00:39, Sven Zühlsdorf wrote:
> > > Hi,
> > > 
> > > I've run into two NULL pointer dereferences when loading the MCP2221 driver.
> > > Initially I observed them running the kernel used by yocto kirkstone
> > > (currently 5.15.68) but can reproduce them with a vanilla 6.1-rc1 as well.
> > > All line numbers below are for hid-mcp2221.c, taken from 6.1-rc1.
> > > 
> > > The first one was easy to identify, in mcp2221_probe line 874 `hdev->hidraw`
> > > was NULL since I compiled the kernel without CONFIG_HIDRAW enabled. Should
> > > CONFIG_HID_MCP2221 perhaps depend on or imply CONFIG_HIDRAW?
> > 
> > Looks to me like it should. Hopefully the HID people can chime in here.
> 
> I actually don't see why this driver (and hid-cp2112.c FWIW) should
> depend on hidraw. To me, the reference to hidraw is just a nicer logging
> message, but I have a hard time understanding how hidraw should be
> involved in the driver, and if it were, how it could not break
> everything.
> 
> So IMO, we should probably change that line from the 2 drivers and
> replace the hidraw part with the hid->id number which is unique.

Exactly. See also
https://lore.kernel.org/linux-input/20220926202239.16379-2-Enrik.Berkhan@inka.de/

Cheers,
Enrik
Randy Dunlap Nov. 3, 2022, 4:13 p.m. UTC | #3 
On 11/3/22 08:22, Enrik Berkhan wrote:
> On Thu, 2022-11-03 at 13:16 +0100, Benjamin Tissoires wrote:
>> On Nov 02 2022, Randy Dunlap wrote:
>>> Hi--
>>>
>>> [adding linux-input mailing list]
>>>
>>> On 10/25/22 00:39, Sven Zühlsdorf wrote:
>>>> Hi,
>>>>
>>>> I've run into two NULL pointer dereferences when loading the MCP2221 driver.
>>>> Initially I observed them running the kernel used by yocto kirkstone
>>>> (currently 5.15.68) but can reproduce them with a vanilla 6.1-rc1 as well.
>>>> All line numbers below are for hid-mcp2221.c, taken from 6.1-rc1.
>>>>
>>>> The first one was easy to identify, in mcp2221_probe line 874 `hdev->hidraw`
>>>> was NULL since I compiled the kernel without CONFIG_HIDRAW enabled. Should
>>>> CONFIG_HID_MCP2221 perhaps depend on or imply CONFIG_HIDRAW?
>>>
>>> Looks to me like it should. Hopefully the HID people can chime in here.
>>
>> I actually don't see why this driver (and hid-cp2112.c FWIW) should
>> depend on hidraw. To me, the reference to hidraw is just a nicer logging
>> message, but I have a hard time understanding how hidraw should be
>> involved in the driver, and if it were, how it could not break
>> everything.
>>
>> So IMO, we should probably change that line from the 2 drivers and
>> replace the hidraw part with the hid->id number which is unique.
> 
> Exactly. See also
> https://lore.kernel.org/linux-input/20220926202239.16379-2-Enrik.Berkhan@inka.de/

Great. Thanks.
Sven Zühlsdorf Dec. 13, 2022, 2 p.m. UTC | #4 
On Tue, 25 Oct 2022, Sven Zühlsdorf wrote:
> Hi,
>
> I've run into two NULL pointer dereferences when loading the MCP2221 driver.
> Initially I observed them running the kernel used by yocto kirkstone
> (currently 5.15.68) but can reproduce them with a vanilla 6.1-rc1 as well.
> All line numbers below are for hid-mcp2221.c, taken from 6.1-rc1.
>
> The first one was easy to identify, in mcp2221_probe line 874 `hdev->hidraw`
> was NULL since I compiled the kernel without CONFIG_HIDRAW enabled. Should
> CONFIG_HID_MCP2221 perhaps depend on or imply CONFIG_HIDRAW?
>
> For the second one however, I'm stuck. The dereference happens at the top of
> mcp_smbus_xfer since i2c_get_adapdata in line 307 returned NULL. Looking at
> the call trace (see [dmesg output] below), mcp_smbus_xfer gets called
> indirectly from mcp2221_probe via i2c_add_adapter in line 876, directly
> before a call to i2c_set_adapdata. Since I couldn't identify another call to
> i2c_set_adapdata or a write to `mcp->adapter.dev.driver_data` that could
> potentially have initialized that field, I attempted to swap the order of
> calling i2c_set_adapdata and i2c_add_adapter, see [attempted fix] below. 
> While
> the driver now loads without issue, no devices appear on the i2c bus:
[...]
>
> When booting a distribution kernel (Ubunutu 22.04 with kernel 5.15.39) the 
> bus
> is populated as expected:
[...]
>
> In the patches applied by Ubuntu I couldn't find anything that'd explain this
> change in behavior. Regarding their kernel configuration the situation is
> similar: Even incorporating almost all of Ubuntu's additions (minus some
> signing and integrity stuff) into the config I've been using results in the
> NULL pointer dereference in mcp_smbus_xfer. Since this still might be a case 
> of
> me missing some config options, I'll post my config in a response to this 
> mail.
>
>
> There seem to be two recent patch series for this driver:
> https://lore.kernel.org/all/20221001005208.8010-1-matt.ranostay@konsulko.com/
> https://lore.kernel.org/all/20220926202239.16379-1-Enrik.Berkhan@inka.de/
> I tested both, but the behavior stays the same.
[...]

We managed to completely fix our second issue. While my initial patch did
fix the observed NULL pointer dereference, the second patch from Enrik
Berkhan's series could have clued me in to what was missing: The hid device
needs to be started, otherwise the responses from the chip aren't processed.
Patch, based on hid.git for-6.2/mcp2221 3d74c9eca1a2, in reply.

The differences to the kernel shipped by Ubuntu turned out to be a red
herring. What actually triggered the NULL dereference was having a driver
loaded that actively probes new I2C busses for suitable devices, thus
triggering the mcp_*_xfer functions before the adapdata was set. Unloading
and reloading hid_mcp2221 once a suitable driver is loaded will happily
trigger the bug on any kernel.
diff mbox series

Patch

--- a/drivers/hid/hid-mcp2221.c	2022-09-19 09:31:22.539691089 +0200
+++ b/drivers/hid/hid-mcp2221.c	2022-09-20 15:21:37.576196331 +0200
@@ -873,12 +873,22 @@ 
  			"MCP2221 usb-i2c bridge on hidraw%d",
  			((struct hidraw *)hdev->hidraw)->minor);

+	int adapdata_change = 0;
+	if(!i2c_get_adapdata(&mcp->adapter))
+	{
+		adapdata_change = 1;
+		hid_warn(hdev, "got NULL adapdata\n");
+		i2c_set_adapdata(&mcp->adapter, mcp);
+	}
  	ret = i2c_add_adapter(&mcp->adapter);
  	if (ret) {
+		if(adapdata_change)
+		{
+			i2c_set_adapdata(&mcp->adapter, NULL);
+		}
  		hid_err(hdev, "can't add usb-i2c adapter: %d\n", ret);
  		goto err_i2c;
  	}
-	i2c_set_adapdata(&mcp->adapter, mcp);

  	/* Setup GPIO chip */
  	mcp->gc = devm_kzalloc(&hdev->dev, sizeof(*mcp->gc), GFP_KERNEL);