Re: [PATCH 5.15.y] Input: aiptek - validate raw macro indices before updating state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] A race condition between sysfs attribute accesses and `aiptek_disconnect()` can lead to a NULL pointer dereference, causing an unprivileged local Denial of Service.
--

commit a32cea658c5c9deb2bbaf0f172438e8f70443904
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

Input: aiptek - validate raw macro indices before updating state
    
aiptek_irq() derives macro key indices directly from tablet reports and
then uses them to index macroKeyEvents[]. Report types 4 and 5 also save
the derived value in aiptek->lastMacro and later use that state to
release the previous key.

Validate the raw macro index once before it enters that state machine, so
lastMacro only ever stores an in-range macro key. Keep direct bounds
checks for report type 6, which reads the macro number from the packet
body and uses it immediately.

Link: https://patch.msgid.link/20260329001711.88076-1-pengpeng@iscas.ac.cn

> diff --git a/drivers/input/tablet/aiptek.c b/drivers/input/tablet/aiptek.c
> index 1581f6ef09279..64113810e16dc 100644
> --- a/drivers/input/tablet/aiptek.c
> +++ b/drivers/input/tablet/aiptek.c

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the driver I
noticed a pre-existing issue in the sysfs removal path during disconnect:

drivers/input/tablet/aiptek.c:aiptek_disconnect() {
	...
	usb_set_intfdata(intf, NULL);
	if (aiptek != NULL) {
		/* Free & unhook everything from the system.
		 */
		usb_kill_urb(aiptek->urb);
		input_unregister_device(aiptek->inputdev);
		sysfs_remove_group(&intf->dev.kobj, &aiptek_attribute_group);
	...
}

Does clearing the driver data with usb_set_intfdata() before removing the
sysfs group create a race window?

If a concurrent read accesses the sysfs file during this window:

drivers/input/tablet/aiptek.c:show_tabletSize() {
	struct aiptek *aiptek = dev_get_drvdata(dev);

	return sysfs_emit(buf, "%dx%d\n",
			  input_abs_get_max(aiptek->inputdev, ABS_X) + 1,
			  input_abs_get_max(aiptek->inputdev, ABS_Y) + 1);
}

Could dev_get_drvdata() return NULL here and cause a kernel panic when
aiptek is dereferenced?

Similarly, if a sysfs write occurs in the same window:

drivers/input/tablet/aiptek.c:store_tabletExecute() {
	struct aiptek *aiptek = dev_get_drvdata(dev);

	/* We do not care what you write to this file. Merely the action
	 * of writing to this file triggers a tablet reprogramming.
	 */
	memcpy(&aiptek->curSetting, &aiptek->newSetting,
	       sizeof(struct aiptek_settings));
	...
}

Can a concurrent sysfs write also fetch a NULL pointer and use it as the
destination for the memcpy()?

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