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 <[email protected]> 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-3a78362c4...@oss.qualcomm.com?part=2
