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

[Florian Fuchs <fuchsfl@gmail.com>]

Hi Dmitry,

On 03 Jul 22:57, Dmitry Torokhov wrote:
> The Maple bus core invokes client callbacks asynchronously from a
> workqueue (maple_dma_handler). If a device is removed (or closed) while
> a callback is in flight, it can lead to UAF bugs if the driver's private
> data is freed.
> 
> Introduce callback_mutex in struct maple_device to synchronize
> callback registration/modification and callback invocation.
> 
> Assisted-by: Antigravity:gemini-3.5-flash
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/sh/maple/maple.c | 9 +++++++--
>  include/linux/maple.h    | 2 ++
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/sh/maple/maple.c b/drivers/sh/maple/maple.c
> index c0715e3ace6f..7c82b7a8a280 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);
>  	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);
>  	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);
> +				}
>  				atomic_set(&mdev->busy, 0);
>  				wake_up(&mdev->maple_wait);
>  				break;
> diff --git a/include/linux/maple.h b/include/linux/maple.h
> index 641cf3330409..48ed7558b8ab 100644
> --- a/include/linux/maple.h
> +++ b/include/linux/maple.h
> @@ -3,6 +3,7 @@
>  #define __LINUX_MAPLE_H
>  
>  #include <linux/device.h>
> +#include <linux/mutex.h>
>  #include <mach/maple.h>
>  
>  /* Maple Bus command and response codes */
> @@ -75,6 +76,7 @@ struct maple_device {
>  	char product_licence[64];
>  	atomic_t busy;
>  	wait_queue_head_t maple_wait;
> +	struct mutex callback_mutex;
>  	struct device dev;
>  };
>  
> 
> -- 
> 2.55.0.rc0.799.gd6f94ed593-goog
> 

Thank you so much for your efforts. I can confirm sashikos finding in my
test (that I saw later), that this patch hangs the bus if a VMU memcard
is inserted. So that the peripherals don't work anymore after insertion
of the memcard. With a last log like:
Dreamcast_visual_memory 3:01.E: VMU at (3, 1) is busy

Regards
Florian