Re: [PATCH v6 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 5 potential issue(s) to consider:

New issues:
- [Medium] Extracting the Sahara protocol to `MHI_SAHARA` and making `DRM_ACCEL_QAIC` depend on it breaks existing kernel configurations.
- [Medium] The standalone `MHI_SAHARA` Kconfig entry fails to select `WANT_DEV_COREDUMP`, causing memory dumps to be silently discarded if QAIC is not enabled.

Pre-existing issues:
- [High] The `qaic_init()` error handling path fails to unregister the timesync and RAS MHI drivers if `qaic_ssr_register()` fails, causing a use-after-free panic.
- [High] The driver cleanup routine `sahara_mhi_remove()` fails to cancel `read_data_work`, which can lead to a use-after-free.
- [High] Teardown symmetry violation in `sahara_mhi_remove()` allows cancelled works to be rescheduled by incoming MHI traffic, leading to a use-after-free.
--

commit 45d20bba8e8207c4574856cd753b585b5512dea6
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 MHI bus host client drivers. It creates a dedicated
Kconfig/Makefile hierarchy for Sahara and converts it to use
module_mhi_driver().

> diff --git a/drivers/accel/qaic/Kconfig b/drivers/accel/qaic/Kconfig
> --- a/drivers/accel/qaic/Kconfig
> +++ b/drivers/accel/qaic/Kconfig
> @@ -8,6 +8,7 @@ config DRM_ACCEL_QAIC
>  	depends on DRM_ACCEL
>  	depends on PCI && HAS_IOMEM
>  	depends on MHI_BUS
> +	depends on MHI_SAHARA

[Severity: Medium]
Will this dependency silently disable the QAIC driver for existing users?

When users upgrade their kernels and run make olddefconfig, the new
MHI_SAHARA option will default to 'N'. Because DRM_ACCEL_QAIC now uses
"depends on MHI_SAHARA", it will be silently disabled and hidden from
menuconfig.

Should this use "select MHI_SAHARA" instead to preserve backward
compatibility?

>  	select CRC32
>  	select WANT_DEV_COREDUMP
>  	help

[ ... ]

> diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
> --- a/drivers/accel/qaic/qaic_drv.c
> +++ b/drivers/accel/qaic/qaic_drv.c
> @@ -818,7 +811,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]
This is a pre-existing issue, but does the qaic_init() error handling
path leak resources and cause a use-after-free?

If qaic_ssr_register() fails, the code jumps to free_bootlog, but it
misses unregistering the timesync and RAS MHI drivers:

drivers/accel/qaic/qaic_drv.c:qaic_init() {
    ...
    ret = qaic_timesync_init();
    ...
    ret = qaic_ras_register();
    ...
    ret = qaic_ssr_register();
    if (ret) {
        pr_debug("qaic: qaic_ssr_register failed %d\n", ret);
        goto free_bootlog;
    }
    ...
free_bootlog:
    qaic_bootlog_unregister();
    mhi_driver_unregister(&qaic_mhi_driver);
free_pci:
    ...
}

Since qaic_timesync_driver and qaic_ras_mhi_driver remain registered
with the MHI bus, will any subsequent MHI bus interaction dereference
these freed pointers and cause a kernel panic?

[ ... ]

> diff --git a/drivers/bus/mhi/host/clients/sahara/Kconfig b/drivers/bus/mhi/host/clients/sahara/Kconfig
> new file mode 100644
> --- /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]
Does this standalone Kconfig entry need to select WANT_DEV_COREDUMP?

The MHI_SAHARA driver uses dev_coredumpv() to generate memory dumps.
If it is built without QAIC or another driver that explicitly selects
WANT_DEV_COREDUMP, the kernel will use the fallback dummy stub for
dev_coredumpv(), silently freeing buffers and dropping the dumps.

Should MHI_SAHARA explicitly select WANT_DEV_COREDUMP here?

> +	help
> +	  Enable support for the Sahara protocol transported over the MHI bus.

[ ... ]

> 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
> --- a/drivers/accel/qaic/sahara.c
> +++ b/drivers/bus/mhi/host/clients/sahara/sahara.c
> @@ -923,13 +923,7 @@ static struct mhi_driver sahara_mhi_driver = {
>  		.name = "sahara",
>  	},
>  };
> +module_mhi_driver(sahara_mhi_driver);

[Severity: High]
This isn't a bug introduced by this patch, but does the driver cleanup
routine fail to cancel read_data_work?

The sahara_context struct is automatically freed after sahara_mhi_remove()
returns. The function cancels fw_work, dump_work, and cmd_work, but it
misses read_data_work:

drivers/bus/mhi/host/clients/sahara/sahara.c:sahara_mhi_remove() {
    ...
    cancel_work_sync(&context->fw_work);
    cancel_work_sync(&context->dump_work);
    cancel_work_sync(&context->cmd_work);
    vfree(context->mem_dump);
    sahara_release_image(context);
    ...
}

If read_data_work is pending or executing when the device is removed,
will it access the freed context struct and cause a use-after-free?


[Severity: High]
This is a pre-existing issue, but does the order of operations in
sahara_mhi_remove() allow cancelled works to be rescheduled?

The function calls cancel_work_sync() before calling
mhi_unprepare_from_transfer():

drivers/bus/mhi/host/clients/sahara/sahara.c:sahara_mhi_remove() {
    ...
    cancel_work_sync(&context->fw_work);
    cancel_work_sync(&context->dump_work);
    cancel_work_sync(&context->cmd_work);
    ...
    mhi_unprepare_from_transfer(mhi_dev);
}

Because the MHI channels remain active during the cancel_work_sync()
calls, can an incoming MHI packet immediately reschedule a work that
was just cancelled?

Should mhi_unprepare_from_transfer() be called first to silence
callbacks and prevent a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701-sahara_protocol_new_v2-v6-0-3a78362c4741@oss.qualcomm.com?part=2