Re: [PATCH v2 1/5] HID: wacom: check the input device in the shared report helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] hid: wacom: touch_input dereferenced before wacom_wac_finger_count_touches()
--

commit 44932063e701ad08bed9376ede6b9b3005174d4c
Author: Jinmo Yang <jinmo44.yang@gmail.com>

HID: wacom: check the input device in the shared report helpers

This commit adds NULL checks for the input device in
wacom_wac_finger_count_touches() and wacom_report_numbered_buttons() to
avoid NULL pointer dereferences when a device interface has no allocated
touch or pad input device.

> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index c4cd87b781c45..567b01882eb1e 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -1263,6 +1263,12 @@ static int wacom_wac_finger_count_touches(struct wacom_wac *wacom)
>  	int count = 0;
>  	int i;
>  
> +	if (!input) {
> +		dev_warn_once(&container_of(wacom, struct wacom, wacom_wac)->hdev->dev,
> +			      "%s: touch_input is NULL\n", __func__);
> +		return 0;
> +	}
> +
>  	if (!touch_max)
>  		return 0;
>  

[Severity: High]
Can this NULL check in wacom_wac_finger_count_touches() actually be
reached when wacom->touch_input is NULL?

Every caller of wacom_wac_finger_count_touches() in drivers/hid/wacom_wac.c
first passes wacom->touch_input to input_mt_sync_frame(), which
immediately dereferences dev->mt:

drivers/input/input-mt.c:input_mt_sync_frame() {
	struct input_mt *mt = dev->mt;
	bool use_count = false;

	if (!mt)
		return;
    ...
}

For example, while a later commit in this series ("HID: wacom: add NULL
input guards to Bamboo touch/button handlers") guards wacom_bpt_touch(),
wacom_24hdt_irq() remains unguarded when wacom->touch_input is NULL (such
as when a WACOM_24HDT or WACOM_27QHDT device probes with a report
descriptor that omits WACOM_FINGER_FIELD usages):

drivers/hid/wacom_wac.c:wacom_24hdt_irq() {
	struct input_dev *input = wacom->touch_input;
    ...
	for (i = 0; i < contacts_to_send; i++) {
		int offset = (byte_per_packet * i) + 1;
		bool touch = (data[offset] & 0x1) && report_touch_events(wacom);
		int slot = input_mt_get_slot_by_key(input, data[offset + 1]);
    ...
	}
	input_mt_sync_frame(input);

	wacom->num_contacts_left -= contacts_to_send;
	if (wacom->num_contacts_left <= 0) {
		wacom->num_contacts_left = 0;
		wacom->shared->touch_down = wacom_wac_finger_count_touches(wacom);
	}
	return 1;
}

Wouldn't wacom_24hdt_irq() dereference a NULL input pointer in
input_mt_get_slot_by_key() and input_mt_sync_frame() before ever reaching
wacom_wac_finger_count_touches()?

Also, when input is non-NULL and touch_max > 1, input_mt_get_slot_by_key(),
input_mt_report_slot_state(), and input_mt_sync_frame() all check for a
NULL dev->mt, whereas wacom_wac_finger_count_touches() dereferences
input->mt->num_slots directly:

drivers/hid/wacom_wac.c:wacom_wac_finger_count_touches() {
    ...
	for (i = 0; i < input->mt->num_slots; i++) {
		struct input_mt_slot *ps = &input->mt->slots[i];
    ...
}

Should wacom_24hdt_irq() check for a NULL input at entry, and should
wacom_wac_finger_count_touches() also guard against input->mt being NULL
(for instance if touch_max > 1 is set via HID_DG_CONTACTMAX without
input_mt_init_slots() having been called)?

[ ... ]

>
>
>

[]

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