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

[sashiko-bot@kernel.org]

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 <unseentactician27062006@gmail.com>

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].

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 628372ab818a2..dcf2811a5c483 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -2619,7 +2619,7 @@ static void wacom_wireless_work(struct work_struct *work)
>  
>  	wacom_destroy_battery(wacom);
>  
> -	if (!usbdev)
> +	if (!usbdev || usbdev->config->desc.bNumInterfaces < 3)
>  		return;

[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