Re: [PATCH v6 5/7] bus: mhi: Load DDR training data using device serial number

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] On 32-bit systems, `dump_length` is truncated during `vzalloc`, leading to a massive heap buffer overflow when parsing memory dumps.
- [High] Lack of synchronization between `fw_work` and `read_data_work` allows a malicious or misbehaving device to trigger a Use-After-Free of firmware data.
- [High] Missing `cancel_work_sync` for `read_data_work` during driver removal leads to a Use-After-Free.
--

commit 089e41c25a9507e0cb3ce33fb65d0e86f16f5c04
Author: Kishore Batta <kishore.batta@oss.qualcomm.com>

bus: mhi: Load DDR training data using device serial number

This commit extends the firmware loading logic to first attempt loading a 
per-device DDR training image derived from the device's serial number. It
falls back to the default image if the per-device one is missing.

> diff --git a/drivers/bus/mhi/host/clients/sahara/sahara.c b/drivers/bus/mhi/host/clients/sahara/sahara.c
> --- a/drivers/bus/mhi/host/clients/sahara/sahara.c
> +++ b/drivers/bus/mhi/host/clients/sahara/sahara.c
> @@ -226,6 +227,27 @@ static int sahara_find_image(struct sahara_context *context, u32 image_id)
>  		return 0;
>  	}
>  
> +	/* DDR training special case: Try per-serial number file first */
> +	if (image_id == SAHARA_DDR_TRAINING_IMG_ID && context->fw_folder) {
> +		u32 serial_num = context->mhi_dev->mhi_cntrl->serial_number;
> +
> +		fw_path = kasprintf(GFP_KERNEL,
> +				    "qcom/%s/mdmddr_0x%x.mbn",
> +				    context->fw_folder, serial_num);
[ ... ]

While reviewing this driver, I noticed a few pre-existing issues in the same
file that might need attention.

[Severity: High]
This is a pre-existing issue, but is there a lack of synchronization between
fw_work and read_data_work that could allow a use-after-free of the firmware
data?

If a compromised device sends an end of image packet while streaming data,
fw_work executes sahara_end_of_image() and calls sahara_release_image(),
which frees the firmware:

drivers/bus/mhi/host/clients/sahara/sahara.c:sahara_release_image() {
    ...
    release_firmware(context->firmware);
    ...
}

Concurrently, read_data_work streams data using read_data_helper(), which
copies from the freed context->firmware->data:

drivers/bus/mhi/host/clients/sahara/sahara.c:read_data_helper() {
    ...
    memcpy(context->tx[buf_index],
           &context->firmware->data[context->read_data_offset],
           pkt_data_len);
    ...
}

Could this lead to memory corruption if a malicious device sends out-of-order
protocol packets?


[Severity: High]
This is a pre-existing issue, but does sahara_mhi_remove() fail to cancel
read_data_work before the context is freed?

drivers/bus/mhi/host/clients/sahara/sahara.c:sahara_mhi_remove() {
    ...
    sahara_release_image(context);
    mhi_unprepare_from_transfer(mhi_dev);
    ...
}

Since read_data_work is not canceled with cancel_work_sync(), can it continue
to execute concurrently after the device is removed?

The context structure is allocated with devm_kzalloc and will be freed when
the remove function returns, potentially resulting in a use-after-free if the
driver is unbound or the device is removed while streaming data.


[Severity: Critical]
This is a pre-existing issue, but on 32-bit systems, does this code truncate
dump_length during allocation?

In sahara_parse_dump_table(), dump_length is a 64-bit value representing the
total size of memory dumps:

drivers/bus/mhi/host/clients/sahara/sahara.c:sahara_parse_dump_table() {
    ...
    context->mem_dump_sz = dump_length;
    context->mem_dump = vzalloc(dump_length);
    ...
}

Because vzalloc() takes a 32-bit unsigned long on 32-bit systems, a length
greater than 4GB will be silently truncated.

However, the driver then proceeds to stream the full 64-bit length of data
from the device, copying it into the under-allocated buffer:

drivers/bus/mhi/host/clients/sahara/sahara.c:sahara_parse_dump_image() {
    ...
    memcpy(context->mem_dump_freespace, context->rx, context->rx_size);
    ...
}

Could a compromised MHI peripheral providing a crafted dump table trigger a
heap buffer overflow?

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