Re: [PATCH v8 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 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 <kishore.batta@oss.qualcomm.com>
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-e12939d229a1@oss.qualcomm.com?part=3