Re: NULL pointer dereferences in hid-mcp2221
From: Randy Dunlap
Date: Wed Nov 02 2022 - 20:31:30 EST
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@xxxxxxxxxxxx/
> https://lore.kernel.org/all/20220926202239.16379-1-Enrik.Berkhan@xxxxxxx/
> 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
>
--
~Randy