Re: [PATCH] HID: wacom: Prevent NULL dereference in wireless work

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] HID: wacom: incomplete NULL and binding checks in wacom_wireless_work()
--

commit 53aa61197952eeb0781af2deacd78d22c186ee96
Author: Devarjya Purkayastha [off-list ref]

HID: wacom: Prevent NULL dereference in wireless work

This commit checks that usbdev->config->desc.bNumInterfaces is at least 3 in
wacom_wireless_work() before accessing interface[1] and interface[2].

[Severity: High]
Is checking usbdev->config->desc.bNumInterfaces < 3 sufficient to prevent a
NULL pointer dereference in wacom_wireless_work()?

First, the commit message states that this checks the active USB
configuration, but usbdev->config points to the array of all configurations
(&usbdev->config[0]), whereas usbdev->actconfig points to the currently
active configuration populated by usb_set_configuration(). On a malformed
multi-configuration device where config[0] has bNumInterfaces >= 3 while a
different configuration is active in usbdev->actconfig,
usbdev->config->interface[1] and usbdev->config->interface[2] will be NULL,
causing usb_get_intfdata() to dereference a NULL pointer:

	/* Stylus interface */
	hdev1 = usb_get_intfdata(usbdev->config->interface[1]);
	wacom1 = hid_get_drvdata(hdev1);
	wacom_wac1 = &(wacom1->wacom_wac);
	wacom_release_resources(wacom1);

Second, even when interface[1] and interface[2] are non-NULL, can hdev1 or
hdev2 be NULL? During usb_set_configuration(), interfaces are probed
sequentially. When interface[0] is probed, wacom_parse_and_register() opens
the HID device and starts interrupt URB reception before interface[1] and
interface[2] have been probed:

	if (features->device_type & WACOM_DEVICETYPE_WL_MONITOR) {
		error = hid_hw_open(hdev);
		if (error) {
			hid_err(hdev, "hw open failed\n");
			goto fail_hw_stop;
		}
	}

In addition, on a malformed USB device where interface[1] or interface[2]
fails usbhid_probe() (for example, due to missing an interrupt-IN endpoint)
or is unbound, usb_get_intfdata() returns NULL for hdev1 or hdev2, and
hid_get_drvdata() will dereference a NULL pointer.

Third, even if usbhid_probe() succeeds so hdev1 and hdev2 are non-NULL,
wacom_probe() can fail on interface[1] or interface[2] (for example, if the
HID report descriptor is malformed), or the wacom driver can be unbound from
those interfaces. In that case hid_get_drvdata() returns NULL for wacom1 or
wacom2, and wacom_release_resources() immediately dereferences it:

static void wacom_release_resources(struct wacom *wacom)
{
	struct hid_device *hdev = wacom->hdev;

	if (!wacom->resources)
		return;

Should wacom_wireless_work() check usbdev->actconfig as well as verify that
the sibling interfaces and their hdev and wacom driver data pointers are
valid and bound before dereferencing them?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030173108.45408-1-unseentactician27062006@gmail.com?part=1