Re: [PATCH v6 3/7] bus: mhi: Centralize Sahara firmware image table selection at probe time

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing cancellation of `read_data_work` during device removal leads to a Use-After-Free.
- [High] Lack of synchronization between concurrent device command processing and data streaming leads to Use-After-Free and TX buffer corruption.
--

commit 812426ed0db5b6e824926f5d8eecb24c749a578e
Author: Kishore Batta <kishore.batta@oss.qualcomm.com>

bus: mhi: Centralize Sahara firmware image table selection at probe time

This commit refactors the Sahara driver to select firmware image tables using
a controller provided mapping table at probe time. It centralizes device
specific conditionals that were previously scattered across the driver. This
prepares the driver for extending support to new devices.

[ ... ]

> diff --git a/drivers/bus/mhi/host/clients/sahara/sahara.c b/drivers/bus/mhi/host/clients/sahara/sahara.c
> index 858dc5bc39c1a..e339c67e236af 100644
> --- a/drivers/bus/mhi/host/clients/sahara/sahara.c
> +++ b/drivers/bus/mhi/host/clients/sahara/sahara.c
> @@ -179,48 +179,7 @@ struct sahara_context {
>  	u32				read_data_length;
>  	bool				is_mem_dump_mode;
>  	bool				non_streaming;
[ ... ]
> +	const char			*fw_folder;
>  };

[Severity: High]
This is a pre-existing issue, but can the device removal path cause a
use-after-free here?

In drivers/bus/mhi/host/clients/sahara/sahara.c:sahara_mhi_remove(),
the teardown cancels other work items:

    cancel_work_sync(&context->fw_work);
    cancel_work_sync(&context->dump_work);
    cancel_work_sync(&context->cmd_work);

However, it seems cancel_work_sync(&context->read_data_work) is omitted.
If a device in streaming mode is actively transferring firmware data via
read_data_work, and the driver is unbound, devres will free the device-managed
context structure. 

Will the pending or sleeping sahara_read_data_processing() work item then
continue to run and access the freed context and its firmware buffer?

[Severity: High]
This is a pre-existing issue, but is there a lack of synchronization between
concurrent device command processing and data streaming that could corrupt
the TX buffer?

If read_data_work is running to stream a firmware image, and a faulty or
compromised device sends an out-of-band message (like END_OF_IMAGE_CMD) on
the downlink channel, fw_work will run concurrently with read_data_work.

fw_work calls sahara_end_of_image(), which frees context->firmware and writes a
response to context->tx[0]:

    sahara_release_image(context);
    ...
    context->tx[0]->cmd = cpu_to_le32(SAHARA_DONE_CMD);

Concurrently, read_data_helper() executed from read_data_work accesses the
freed firmware and writes to the same TX buffer:

    memcpy(context->tx[buf_index],
           &context->firmware->data[context->read_data_offset],
           pkt_data_len);

Does this sequence result in a use-after-free and packet corruption before
queuing it to MHI?

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