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 <[email protected]> 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-3a78362c4...@oss.qualcomm.com?part=5
