Re: [PATCH] Input: maplecontrol - set driver data before registering input device

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Dangling pointer to freed memory remains in driver data during the probe error path.

Pre-existing issues:
- [Critical] Invalid pointer derivation leading to NULL pointer dereference on device open.
- [High] Race condition exposing uninitialized driver data to hardware callbacks or userspace.
- [High] Race condition during device unbind leading to a NULL pointer dereference in the callback.
--

commit 32d63e0cc91d90b1d5a10a7764828de58888a240
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Input: maplecontrol - set driver data before registering input device

This commit modifies the maplecontrol driver to set the driver data before
calling input_register_device(). This ensures that the driver data is
available immediately if the device is opened and the callback is triggered.

> diff --git a/drivers/input/joystick/maplecontrol.c b/drivers/input/joystick/maplecontrol.c
> index 7f36f73844a91..6293b6e8148b3 100644
> --- a/drivers/input/joystick/maplecontrol.c
> +++ b/drivers/input/joystick/maplecontrol.c
> @@ -146,7 +148,6 @@ static int probe_maple_controller(struct device *dev)
>  		goto fail;
>  
>  	mdev->driver = mdrv;
> -	maple_set_drvdata(mdev, pad);
>  
>  	return 0;

[Severity: Medium]
Since maple_set_drvdata(mdev, pad) was moved before input_register_device(),
does this leave a dangling pointer to freed memory in the error path?

If input_register_device() fails, it jumps to the fail block:

fail:
        input_free_device(idev);
        kfree(pad);
        maple_set_drvdata(mdev, NULL);
        return error;

This executes kfree(pad) before maple_set_drvdata(mdev, NULL), temporarily
leaving a dangling pointer in driver core structures. Should the order be
updated to clear the driver data before freeing pad?

[Severity: High]
This isn't a bug introduced by this patch, but does device removal race with
the maple DMA workqueue leading to a NULL pointer dereference?

When unbinding the device, remove_maple_controller() clears mdev->callback:

drivers/input/joystick/maplecontrol.c:remove_maple_controller() {
    ...
    mdev->callback = NULL;
    input_unregister_device(pad->dev);
    maple_set_drvdata(mdev, NULL);
    kfree(pad);
    ...
}

If the device is still open, input_unregister_device() synchronously invokes
dc_pad_close(), which inadvertently overwrites mdev->callback back to
dc_pad_callback:

drivers/input/joystick/maplecontrol.c:dc_pad_close() {
    ...
    maple_getcond_callback(pad->mdev, dc_pad_callback, 0, MAPLE_FUNC_CONTROLLER);
    ...
}

The driver then executes maple_set_drvdata(mdev, NULL) and frees memory. A
pending maple DMA response running in the workqueue can then trigger
dc_pad_callback(), which fetches the now-NULL drvdata:

drivers/input/joystick/maplecontrol.c:dc_pad_callback() {
    ...
    struct dc_pad *pad = maple_get_drvdata(mapledev);
    struct input_dev *dev = pad->dev;
    ...
}

Could this result in dereferencing pad->dev and crashing the kernel if
triggered by unbinding the device while it is actively opened by local
userspace?

[Severity: High]
This is a pre-existing issue, but is there a race condition exposing
uninitialized driver data to hardware callbacks or userspace in other maple
drivers?

In drivers/input/keyboard/maple_keyb.c:probe_maple_kbd():

    maple_getcond_callback(mdev, dc_kbd_callback, HZ/50, MAPLE_FUNC_KEYBOARD);
    mdev->driver = mdrv;
    maple_set_drvdata(mdev, kbd);

The callback is armed before the driver data is initialized, allowing the
interrupt handler to run and read a NULL pointer.

Similarly, in drivers/input/mouse/maplemouse.c:probe_maple_mouse():

    error = input_register_device(input_dev);
    if (error)
            goto fail_register;

    mdev->driver = mdrv;
    maple_set_drvdata(mdev, mse);

Driver data for the mouse is set after device registration. If the hardware
callback fires immediately or userspace opens the device concurrently,
maple_get_drvdata() will return NULL and the callback could dereference it.

[Severity: Critical]
This is a pre-existing issue, but can this lead to a NULL pointer dereference
on device open?

Looking at dc_mouse_open() in drivers/input/mouse/maplemouse.c:

    struct dc_mouse *mse = maple_get_drvdata(to_maple_dev(&dev->dev));
    maple_getcond_callback(mse->mdev, dc_mouse_callback, HZ/50, MAPLE_FUNC_MOUSE);

The to_maple_dev macro uses container_of to subtract the offset of dev in
struct maple_device, and maple_get_drvdata immediately adds the offset back
via &mdev->dev. This mathematical cancellation means it effectively calls
dev_get_drvdata(&input_dev->dev).

However, the driver data was actually stored in the maple device (&mdev->dev),
not the input device. Thus, dev_get_drvdata returns NULL, and the subsequent
dereference of mse->mdev results in a direct NULL pointer dereference.

The same issue appears in drivers/input/mouse/maplemouse.c:dc_mouse_close():

    struct dc_mouse *mse = maple_get_drvdata(to_maple_dev(&dev->dev));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/akNYib9hQFNN1fA9@google.com?part=1