Re: [PATCH 17/26] sh: maple: implement bus-level probe/remove

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Dangling function pointers in maple_device leading to execution of unloaded module code on hot-unplug.
- [High] Missing `cancel_work_sync()` in `maple_bus_init` error path leads to Use-After-Free.
- [High] Dangling hardware callback in `vmu_connect` error path leads to Use-After-Free.
- [High] NULL pointer dereference and multiple memory leaks in `vmu_disconnect`.
- [High] The driver reports input events but fails to call `input_sync()` at the end of the packet handling.
- [Medium] Memory leak of `card->mtd` array on failure path in `vmu_connect`.
--

commit 8d15bc5545d93a9d1f9f85e0e543bc57384c73cb
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

sh: maple: implement bus-level probe/remove
    
Implement probe() and remove() methods for the maple bus, and update
struct maple_driver to have its own probe() and remove() members that
take struct maple_device * directly. Adjust all maple drivers to
use these new bus-level methods.

> diff --git a/drivers/input/joystick/maplecontrol.c b/drivers/input/joystick/maplecontrol.c
> index 955e01bcfa0c1..6864243b0b4a2 100644
> --- a/drivers/input/joystick/maplecontrol.c
> +++ b/drivers/input/joystick/maplecontrol.c
> @@ -78,7 +78,7 @@ static void dc_pad_close(struct input_dev *dev)
>  }
>  
>  /* allow the controller to be used */
> -static int probe_maple_controller(struct device *dev)
> +static int probe_maple_controller(struct maple_device *mdev)
>  {

[Severity: High]
This is a pre-existing issue, but does dc_pad_callback() fail to call
input_sync() after reporting events?

drivers/input/joystick/maplecontrol.c:dc_pad_callback() {
    ...
    input_report_abs(dev, ABS_RY,     res[15]);
}

The driver reports input events but never calls input_sync() at the end of the
packet handling. As dictated by the Input Subsystem guidelines, every logical
group of events must be followed by a call to input_sync(), otherwise events
may not be properly synchronized and delivered to userspace.

[ ... ]
> diff --git a/drivers/mtd/maps/vmu-flash.c b/drivers/mtd/maps/vmu-flash.c
> index 8f7028ac1b56a..c34c768dfb876 100644
> --- a/drivers/mtd/maps/vmu-flash.c
> +++ b/drivers/mtd/maps/vmu-flash.c
> @@ -769,30 +769,25 @@ static void vmu_file_error(struct maple_device *mdev, void *recvbuf)
>  }
>  
>  
> -static int probe_maple_vmu(struct device *dev)
> +static int probe_maple_vmu(struct maple_device *mdev)
>  {
> -	struct maple_device *mdev = to_maple_dev(dev);
> -
>  	mdev->can_unload = vmu_can_unload;
>  	mdev->fileerr_handler = vmu_file_error;
>  

[Severity: High]
This is a pre-existing issue, but does this leave dangling function pointers
during device hot-unplug?

mdev->can_unload and mdev->fileerr_handler are set here, but remove_maple_vmu()
never clears them. If the module is unbound and unloaded, and the device is
subsequently physically unplugged, maple_response_none() will call the dangling
mdev->can_unload(mdev). This would jump to unmapped memory and cause a kernel
panic.

>  	return vmu_connect(mdev);
>  }

[Severity: Medium]
This is a pre-existing issue, but does vmu_connect() leak the card->mtd array
on the packet addition failure path?

When maple_add_packet() fails, the code jumps to fail_mtd_info:

drivers/mtd/maps/vmu-flash.c:vmu_connect() {
    ...
fail_device_busy:
    kfree(card->mtd);
fail_mtd_info:
    kfree(card->parts);
    ...
}

This bypasses the kfree(card->mtd) cleanup block, causing a memory leak.

[Severity: High]
This is a pre-existing issue, but does vmu_connect() leave a dangling callback
if it fails?

drivers/mtd/maps/vmu-flash.c:vmu_connect() {
    ...
    maple_getcond_callback(mdev, vmu_queryblocks, 0, MAPLE_FUNC_MEMCARD);
    ...
}

If vmu_connect() returns an error (like device busy or maple_add_packet failure),
it frees the 'card' structure but doesn't clear mdev->callback. If the maple
bus receives a MAPLE_RESPONSE_DATATRF, will the core unconditionally execute the
callback and attempt to use the freed 'card' structure?

> -static int remove_maple_vmu(struct device *dev)
> +static void remove_maple_vmu(struct maple_device *mdev)
>  {
> -	struct maple_device *mdev = to_maple_dev(dev);
> -
>  	vmu_disconnect(mdev);
> -	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but can vmu_disconnect() trigger a NULL pointer
dereference?

drivers/mtd/maps/vmu-flash.c:vmu_disconnect() {
    ...
    for (x = 0; x < card->partitions; x++) {
        mpart = ((card->mtd)[x]).priv;
        mpart->mdev = NULL;
    ...
}

Since vmu_connect() creates card->mtd as a zeroed array, card->mtd[x].priv is
NULL until vmu_queryblocks() asynchronously allocates and assigns it. If the
device is disconnected before the callback completes for all partitions, will
this blindly dereference mpart? Additionally, does this fail to free mpart,
pcache, and pcache->buffer completely, causing memory leaks on every successful
bind/unbind?

[ ... ]
> diff --git a/drivers/sh/maple/maple.c b/drivers/sh/maple/maple.c
> index 35aff2e57d2c8..c0715e3ace6f1 100644
> --- a/drivers/sh/maple/maple.c
> +++ b/drivers/sh/maple/maple.c
> @@ -790,7 +816,6 @@ static int __init maple_bus_init(void)
>  	if (retval)
>  		goto cleanup_device;
>  
> -
>  	/* allocate memory for maple bus dma */
>  	retval = maple_get_dma_buffer();

[Severity: High]
This is a pre-existing issue, but does the error path in maple_bus_init()
fail to cancel asynchronous work, leading to a use-after-free?

drivers/sh/maple/maple.c:maple_bus_init() {
    ...
cleanup_irq:
    free_irq(HW_EVENT_MAPLE_DMA, &maple_bus);

cleanup_dma:
    free_pages((unsigned long) maple_sendbuf, MAPLE_DMA_PAGES);
    ...
}

If initialization fails after the DMA or VBLANK IRQs are registered, the error
path frees the IRQs and the maple_sendbuf memory. However, it fails to cancel
the asynchronous work (maple_dma_process and maple_vblank_process). If an IRQ
fires before free_irq(), the scheduled work will execute maple_dma_handler() or
maple_vblank_handler(), which dereferences the freed maple_sendbuf memory via
maple_send().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-b4-maple-cleanup-v1-0-41e424964da5@gmail.com?part=17