Re: [PATCH 14/26] mtd: maps: vmu-flash: remove unused mdev->driver assignment

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Dangling pointers for `fileerr_handler` and `can_unload` left on `maple_device` if `probe` fails.
- [Critical] Use-after-free of `card` structure due to dangling `mdev->callback` on probe error path.
- [High] Out-of-bounds read in `vmu_connect` when parsing device `function_data`.
- [High] NULL pointer dereference in `vmu_disconnect` if `vmu_queryblocks` fails.
--

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

mtd: maps: vmu-flash: remove unused mdev->driver assignment

This commit removes the unused assignment to mdev->driver and the now unused
mdrv variable in probe_maple_vmu. The driver field in struct maple_device is
no longer used since earlier cleanups.

> diff --git a/drivers/mtd/maps/vmu-flash.c b/drivers/mtd/maps/vmu-flash.c
> index 10244e6731d07..8f7028ac1b56a 100644
> --- a/drivers/mtd/maps/vmu-flash.c
> +++ b/drivers/mtd/maps/vmu-flash.c
> @@ -772,11 +772,9 @@ static void vmu_file_error(struct maple_device *mdev, void *recvbuf)
>  static int probe_maple_vmu(struct device *dev)
>  {
>  	struct maple_device *mdev = to_maple_dev(dev);
> -	struct maple_driver *mdrv = to_maple_driver(dev->driver);
>  
>  	mdev->can_unload = vmu_can_unload;
>  	mdev->fileerr_handler = vmu_file_error;
> -	mdev->driver = mdrv;
>  
>  	return vmu_connect(mdev);

[Severity: Critical]
This is a pre-existing issue, but if vmu_connect() fails below, are these
pointers properly cleared?

If vmu_connect() fails, the probe aborts and the device is left unbound, but
the function pointers to vmu_can_unload() and vmu_file_error() remain
assigned to the persistent maple_device structure. 

If the vmu-flash module is subsequently unloaded, could a later
MAPLE_RESPONSE_FILEERR event cause the maple bus core to dereference the
now-dangling mdev->fileerr_handler pointer?

>  }

While reviewing the context of this patch, I noticed a few other pre-existing
issues in the surrounding code:

> static int vmu_connect(struct maple_device *mdev)
> {
> 	unsigned long test_flash_data, basic_flash_data;
> 	int c, error;
> 	struct memcard *card;
> 	u32 partnum = 0;
> 
> 	test_flash_data = be32_to_cpu(mdev->devinfo.function);
> 	/* Need to count how many bits are set - to find out which
> 	 * function_data element has details of the memory card
> 	 */
> 	c = hweight_long(test_flash_data);
> 
> 	basic_flash_data = be32_to_cpu(mdev->devinfo.function_data[c - 1]);

[Severity: High]
This is a pre-existing issue, but does this array access risk an out-of-bounds
read?

Since test_flash_data is derived directly from the hardware packet, it could
have up to 32 bits set. If a malfunctioning device reports 0 bits or more than
3 bits set, does using c - 1 read past the bounds of the 3-element
function_data array?

[ ... ]

> 	/*
> 	* We want to trap meminfo not get cond
> 	* so set interval to zero, but rely on maple bus
> 	* driver to pass back the results of the meminfo
> 	*/
> 	maple_getcond_callback(mdev, vmu_queryblocks, 0,
> 		MAPLE_FUNC_MEMCARD);

[ ... ]

> 	error = maple_add_packet(mdev, MAPLE_FUNC_MEMCARD,
> 		MAPLE_COMMAND_GETMINFO, 2, &partnum);
> 	if (error) {
> 		dev_err(&mdev->dev, "Could not lock VMU at (%d, %d)"
> 			" error is 0x%X\n", mdev->port, mdev->unit, error);
> 		goto fail_mtd_info;
> 	}
> 	return 0;
> 
> fail_device_busy:
> 	kfree(card->mtd);
> fail_mtd_info:
> 	kfree(card->parts);
> fail_partitions:
> 	kfree(card);
> fail_nomem:
> 	return error;
> }

[Severity: Critical]
This is a pre-existing issue, but if maple_add_packet() fails, is the card
structure freed while the callback is still active?

In vmu_connect(), an asynchronous callback is registered via
maple_getcond_callback(). If a subsequent operation like maple_add_packet()
fails, the error path frees the card structure but does not clear
mdev->callback.

If a delayed or stray MAPLE_RESPONSE_DATATRF packet arrives later, could
maple_dma_handler() invoke the lingering mdev->callback, writing hardware
response data to the already freed card memory?

Regarding vmu_disconnect():

If vmu_queryblocks() fails, it branches to an error path that frees the
priv pointer and sets it to NULL:

> 	for (error = 0; error <= card->partition; error++) {
> 		kfree(((card->mtd)[error]).priv);
> 		((card->mtd)[error]).priv = NULL;
> 	}

Because the initial probe returned success, the device remains bound. When
it is eventually disconnected, vmu_disconnect() executes:

> 	for (x = 0; x < card->partitions; x++) {
> 		mpart = ((card->mtd)[x]).priv;
> 		mpart->mdev = NULL;

[Severity: High]
This is a pre-existing issue, but if vmu_queryblocks() encountered an
asynchronous failure earlier, will vmu_disconnect() blindly dereference a
NULL pointer here?

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