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 <[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 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-dc9f24357...@oss.qualcomm.com?part=2
