Re: [PATCH 18/26] sh: maple: introduce callback_mutex in maple_device

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The newly introduced callback_mutex causes a guaranteed self-deadlock when enumerating vmu-flash devices.
- [Medium] The newly introduced callback_mutex is initialized dynamically but is not destroyed during the standard device release lifecycle.

Pre-existing issues:
- [High] The patch fails to prevent the Use-After-Free race condition in the vmu-flash driver because vmu_disconnect bypasses the new callback_mutex.
--

commit 29c05632bf3bdcdb9087fb8230fb421ba8176ce5
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

sh: maple: introduce callback_mutex in maple_device

This commit introduces callback_mutex in struct maple_device to synchronize
callback registration and invocation. It is intended to prevent 
use-after-free bugs if a device is removed while a callback is in flight.

> diff --git a/drivers/sh/maple/maple.c b/drivers/sh/maple/maple.c
> index c0715e3ace6f1..7c82b7a8a2809 100644
> --- a/drivers/sh/maple/maple.c
> +++ b/drivers/sh/maple/maple.c
> @@ -121,6 +121,7 @@ void maple_getcond_callback(struct maple_device *dev,
>  			void (*callback) (struct mapleq *mq),
>  			unsigned long interval, unsigned long function)
>  {
> +	guard(mutex)(&dev->callback_mutex);

[Severity: Critical]
Does this introduce a self-deadlock when a callback registers a new callback?

When enumerating a vmu-flash device, maple_dma_handler() acquires 
mdev->callback_mutex and executes the callback vmu_queryblocks(). 
Inside vmu_queryblocks(), the driver calls maple_getcond_callback() to
configure the next phase. 

Since maple_getcond_callback() unconditionally attempts to acquire the
same non-recursive mdev->callback_mutex here, won't this immediately
deadlock the maple bus workqueue?

>  	dev->callback = callback;
>  	dev->interval = interval;
>  	dev->function = cpu_to_be32(function);

[ ... ]

> @@ -230,11 +231,13 @@ static struct maple_device *maple_alloc_dev(int port, int unit)
>  	mdev->dev.bus = &maple_bus_type;
>  	mdev->dev.parent = &maple_bus;
>  	init_waitqueue_head(&mdev->maple_wait);
> +	mutex_init(&mdev->callback_mutex);
>  	return mdev;
>  }
>  
>  static void maple_free_dev(struct maple_device *mdev)
>  {
> +	mutex_destroy(&mdev->callback_mutex);

[Severity: Medium]
Is a corresponding mutex_destroy() needed in the standard device release 
path?

While mutex_destroy() is added to maple_free_dev() here, if a maple device's 
reference count drops to zero after device_unregister(), the release 
path maple_release_device() is executed. 

Will this leave stale lockdep/RT-mutex structures behind if debugging 
configs are enabled, since maple_release_device() does not destroy the 
mutex before calling kfree(mdev)?

>  	kmem_cache_free(maple_queue_cache, mdev->mq->recvbuf);
>  	kfree(mdev->mq);
>  	kfree(mdev);

[ ... ]

> @@ -655,8 +658,10 @@ static void maple_dma_handler(struct work_struct *work)
>  				break;
>  
>  			case MAPLE_RESPONSE_DATATRF:
> -				if (mdev->callback)
> -					mdev->callback(mq);
> +				scoped_guard(mutex, &mdev->callback_mutex) {
> +					if (mdev->callback)
> +						mdev->callback(mq);
> +				}

[Severity: High]
This is a pre-existing issue, but does this actually prevent the 
use-after-free race condition for all drivers?

If a user unbinds a vmu-flash device while a maple data transfer is 
in-flight, vmu_disconnect() bypasses this new mutex entirely:

drivers/mtd/maps/vmu-flash.c:vmu_disconnect() {
    ...
    mdev->callback = NULL;
    card = maple_get_drvdata(mdev);
    ...
    kfree(card);
}

Since vmu_disconnect() modifies the callback and frees the private data 
without acquiring callback_mutex, can maple_dma_handler() still 
concurrently dereference the freed card data here?

>  				atomic_set(&mdev->busy, 0);
>  				wake_up(&mdev->maple_wait);
>  				break;

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