Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] workqueue: missing cancel_work_sync for read_data_work causes use-after-free - [High] mhi: teardown race in sahara_mhi_remove allows requeuing canceled work - [High] mhi: data race on read_data_offset allows out-of-bounds firmware memory read - [Low] qaic: redundant strcmp in qaic_mhi_register_controller() for sahara_fw --
commit 6aa29eb13055431d194b1b8244436f789375e69f Author: Kishore Batta <[email protected]> bus: mhi: Centralize Sahara firmware image table selection at probe time This patch refactors the Sahara firmware image table selection into a single, explicit probe time mechanism using a controller provided firmware mapping table. This centralizes device specific decisions and simplifies the Sahara probe logic. > diff --git a/drivers/accel/qaic/mhi_controller.c > b/drivers/accel/qaic/mhi_controller.c > index 40e6d262ef217..dd320739c07ad 100644 > --- a/drivers/accel/qaic/mhi_controller.c > +++ b/drivers/accel/qaic/mhi_controller.c [ ... ] > @@ -871,6 +927,11 @@ struct mhi_controller > *qaic_mhi_register_controller(struct pci_dev *pci_dev, voi > mhi_cntrl->name = "AIC100"; > } > > + if (mhi_cntrl->name && !strcmp(mhi_cntrl->name, "AIC100")) > + mhi_cntrl->sahara_fw = &aic100_sahara_fw; > + else if (mhi_cntrl->name && !strcmp(mhi_cntrl->name, "AIC200")) > + mhi_cntrl->sahara_fw = &aic200_sahara_fw; > + [Severity: Low] This isn't a bug, but since mhi_cntrl->name was just assigned as a literal string ("AIC100" or "AIC200") right above this, is it necessary to use strcmp() to check it again? Could the sahara_fw assignment be done directly in the same if/else block that sets the name? > diff --git a/drivers/bus/mhi/host/clients/sahara/sahara.c > b/drivers/bus/mhi/host/clients/sahara/sahara.c > index dc64beae06ada..74bf57f02873e 100644 > --- a/drivers/bus/mhi/host/clients/sahara/sahara.c > +++ b/drivers/bus/mhi/host/clients/sahara/sahara.c [ ... ] > @@ -808,14 +768,14 @@ static int sahara_mhi_probe(struct mhi_device *mhi_dev, > const struct mhi_device_ > - if (!strcmp(mhi_dev->mhi_cntrl->name, "AIC200")) { > - context->image_table = aic200_image_table; > - context->table_size = ARRAY_SIZE(aic200_image_table); > - } else { > - context->image_table = aic100_image_table; > - context->table_size = ARRAY_SIZE(aic100_image_table); > - context->non_streaming = true; > - } > + sahara_fw = mhi_dev->mhi_cntrl->sahara_fw; > + if (!sahara_fw || !sahara_fw->image_table || !sahara_fw->table_size) > + return -ENODEV; > + > + context->image_table = sahara_fw->image_table; > + context->table_size = sahara_fw->table_size; > + context->non_streaming = sahara_fw->non_streaming; > + context->fw_folder = sahara_fw->fw_folder; [Severity: High] Since this configures devices for streaming mode which relies on read_data_work being scheduled, could the teardown path leave this work running? Looking at sahara_mhi_remove(), it cancels fw_work and dump_work but skips read_data_work: sahara_mhi_remove() { ... cancel_work_sync(&context->fw_work); cancel_work_sync(&context->dump_work); vfree(context->mem_dump); ... } If the device is unbound while read_data_work is pending or executing, will the work execute after the devm-allocated context is freed by devres, leading to a use-after-free when sahara_read_data_processing() runs? [Severity: High] Also in sahara_mhi_remove(), is there a race with the MHI channels that could requeue canceled work? sahara_mhi_remove() { ... cancel_work_sync(&context->fw_work); cancel_work_sync(&context->dump_work); ... mhi_unprepare_from_transfer(mhi_dev); } Since cancel_work_sync() happens before mhi_unprepare_from_transfer() stops the channels, if an MHI packet arrives during or immediately after the cancellations, won't sahara_mhi_dl_xfer_cb() blindly call schedule_work(&context->fw_work) again? This newly queued work would then execute on the freed context memory after remove() completes. [Severity: High] Does streaming mode have a data race on read_data_offset and read_data_length? In sahara_read_data(), these fields are updated without locks: sahara_read_data() { ... context->read_data_offset = data_offset; context->read_data_length = data_len; } Back in sahara_processing(), the driver re-queues the single context->rx buffer, allowing the device to send another command. If a malicious device sends a second command while a large streaming transfer is ongoing, fw_work will update these fields while read_data_work is actively running on another CPU. Can read_data_helper() then read a mixed state (like the large length from the first command and a huge offset from the second command) causing an out-of-bounds read from kernel memory via memcpy()? read_data_helper() { ... pkt_data_len = min(context->read_data_length, SAHARA_PACKET_MAX_SIZE); memcpy(context->tx[buf_index], &context->firmware->data[context->read_data_offset], pkt_data_len); } -- Sashiko AI review · https://sashiko.dev/#/patchset/20261012-sahara_protocol_new_v2-v8-0-e12939d22...@oss.qualcomm.com?part=3
