Re: [PATCH v9 2/7] bus: mhi: Move Sahara protocol driver under MHI host client drivers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] qaic: leak of timesync and ras drivers on init failure
- [High] mhi: sahara: UAF due to missing read_data_work cancellation
- [Medium] bus: mhi: mhi_sahara module missing MODULE_DEVICE_TABLE for autoloading
- [Medium] kconfig: MHI_SAHARA missing select WANT_DEV_COREDUMP silently drops dumps
--

commit 1096a5d73941553ee9b70087bffc4c550e7d93dd
Author: Kishore Batta <kishore.batta@oss.qualcomm.com>

bus: mhi: Move Sahara protocol driver under MHI host client drivers

This commit moves the Sahara protocol driver from the QAIC accelerator
subsystem to the generic MHI host client drivers. It creates a dedicated
Kconfig and Makefile hierarchy for Sahara. The driver is also converted
to use module_mhi_driver() instead of manual register and unregister hooks.

> diff --git a/drivers/bus/mhi/host/clients/sahara/Kconfig b/drivers/bus/mhi/host/clients/sahara/Kconfig
> new file mode 100644
> index 0000000000000..f1fc7013a2dee
> --- /dev/null
> +++ b/drivers/bus/mhi/host/clients/sahara/Kconfig
> @@ -0,0 +1,15 @@
> +config MHI_SAHARA
> +	tristate "Sahara protocol driver"
> +	depends on MHI_BUS

[Severity: Medium]
Will this silently drop memory dumps on systems without WANT_DEV_COREDUMP
enabled?

Since WANT_DEV_COREDUMP is a hidden symbol (no prompt), users cannot manually
enable it. Because the newly extracted MHI_SAHARA driver advertises memory
dump retrieval and relies on dev_coredumpv(), omitting a select
WANT_DEV_COREDUMP statement means the devcoredump fallback stub might just
free the buffer without warning, leading to a silent loss of functionality.

> diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
> index 0acf6cdd44f81..64e87dfaa5603 100644
> --- a/drivers/accel/qaic/qaic_drv.c
> +++ b/drivers/accel/qaic/qaic_drv.c
[ ... ]
> @@ -816,7 +809,6 @@ static int __init qaic_init(void)
>  
>  free_bootlog:
>  	qaic_bootlog_unregister();
> -free_mhi:
>  	mhi_driver_unregister(&qaic_mhi_driver);
>  free_pci:
>  	pci_unregister_driver(&qaic_pci_driver);

[Severity: High]
Does this error path leak the timesync and ras MHI drivers?

If qaic_ssr_register() fails, the code jumps to free_bootlog. This cleanup
path unregisters the bootlog, MHI, and PCI drivers, but skips calling
qaic_timesync_deinit() and qaic_ras_unregister(). 

Because mhi_driver_register() adds these driver structures to the driver
core and sysfs, failing module initialization without unregistering them
leaves dangling pointers to the unloaded module's memory, which could
trigger a use-after-free crash if the bus core subsequently accesses them.

> diff --git a/drivers/accel/qaic/sahara.c b/drivers/bus/mhi/host/clients/sahara/sahara.c
> similarity index 99%
> rename from drivers/accel/qaic/sahara.c
> rename to drivers/bus/mhi/host/clients/sahara/sahara.c
> index c7c0b3eb4b65f..75c0cf6010e15 100644
> --- a/drivers/accel/qaic/sahara.c
> +++ b/drivers/bus/mhi/host/clients/sahara/sahara.c
[ ... ]
> @@ -922,13 +922,7 @@ static struct mhi_driver sahara_mhi_driver = {
>  		.name = "sahara",
>  	},
>  };
> +module_mhi_driver(sahara_mhi_driver);

[Severity: Medium]
Will this driver fail to load automatically when the hardware is detected?

Because the driver is now extracted into an independent module, missing the
MODULE_DEVICE_TABLE macro for sahara_mhi_match_table means the match table
isn't exported to the module alias table. This prevents udev or modprobe
from automatically loading mhi_sahara when the QAIC_SAHARA channel is
detected.

[Severity: High]
Is there a use-after-free risk during device removal if read_data_work
remains pending?

When the device is removed or unbound via sysfs, sahara_mhi_remove() is
called. While fw_work and dump_work are cancelled, read_data_work is
completely omitted:

drivers/bus/mhi/host/clients/sahara/sahara.c:sahara_mhi_remove() {
	struct sahara_context *context = dev_get_drvdata(&mhi_dev->dev);

	cancel_work_sync(&context->fw_work);
	cancel_work_sync(&context->dump_work);
	vfree(context->mem_dump);
	sahara_release_image(context);
	mhi_unprepare_from_transfer(mhi_dev);
}

If read_data_work was scheduled and remains pending, it will execute after
sahara_mhi_remove() returns and access the context structure, which has
already been freed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-sahara_protocol_new_v2-v9-0-dc9f24357c4a@oss.qualcomm.com?part=2