Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [Medium] The firmware fallback mechanism inadvertently changes the behavior for existing AIC100 and AIC200 devices, bypassing their explicit allowlist and contradicting the commit message. - [Low] The `dev_err` statement in the fallback error path uses a signed format specifier `%d` to print an unsigned 32-bit integer. Pre-existing issues: - [High] A malicious or malfunctioning device can trigger a NULL pointer dereference or Use-After-Free by requesting an image transfer with `image_id` equal to `SAHARA_IMAGE_ID_NONE` (0xFFFFFFFF). - [High] Missing cancellation of `read_data_work` during driver removal leads to a Use-After-Free of `context` and `context->firmware`. - [High] Lockless concurrency between `fw_work` and `read_data_work` allows a device-triggered Use-After-Free during firmware streaming. -- commit e80f9e2e7047bfa82528957f04343840159ae175 Author: Kishore Batta <[email protected]> bus: mhi: Add QDU100 Sahara variant and firmware fallback Extend the probe time variant selection to match the SAHARA MHI channel for QDU100 devices. Add a firmware lookup fallback to provision images dynamically if they aren't explicitly listed in the image table. > diff --git a/drivers/bus/mhi/host/clients/sahara/sahara.c > b/drivers/bus/mhi/host/clients/sahara/sahara.c > index e339c67e236af..9adbd84859073 100644 > --- a/drivers/bus/mhi/host/clients/sahara/sahara.c > +++ b/drivers/bus/mhi/host/clients/sahara/sahara.c > @@ -189,6 +189,7 @@ static bool is_streaming(struct sahara_context *context) > > static int sahara_find_image(struct sahara_context *context, u32 image_id) > { > + char *fw_path; > int ret; > > if (image_id == context->active_image_id) > return 0; [Severity: High] This is a pre-existing issue, but can a malicious or malfunctioning device trigger a NULL pointer dereference or use-after-free here? If a device requests an image transfer with image_id equal to SAHARA_IMAGE_ID_NONE (0xFFFFFFFF), context->active_image_id is initialized and reset to this value. sahara_find_image() would return success (0) without acquiring firmware. Later, sahara_read_data() unconditionally dereferences context->firmware->size, which would be NULL or a dangling pointer. [ ... ] > @@ -201,8 +202,28 @@ static int sahara_find_image(struct sahara_context > *context, u32 image_id) > } > > if (image_id >= context->table_size || !context->image_table[image_id]) > { > - dev_err(&context->mhi_dev->dev, "request for unknown image: > %d\n", image_id); > - return -EINVAL; > + if (!context->fw_folder) { > + dev_err(&context->mhi_dev->dev, > + "Request for unknown image: %u (no fw > folder)\n", image_id); > + return -EINVAL; > + } [Severity: Medium] The commit message states this change does not affect behavior for existing AIC100 and AIC200 devices. However, legacy AIC devices populate fw_folder at probe time. Since this check relies on !context->fw_folder instead of matching against the SAHARA channel, will legacy AIC devices fall through to the VFS lookup via firmware_request_nowarn() instead of immediately returning -EINVAL? > + > + fw_path = kasprintf(GFP_KERNEL, "qcom/%s/%u", > + context->fw_folder, image_id); > + if (!fw_path) > + return -ENOMEM; > + > + ret = firmware_request_nowarn(&context->firmware, > + fw_path, > + &context->mhi_dev->dev); > + kfree(fw_path); > + if (ret) { > + dev_err(&context->mhi_dev->dev, > + "request for unknown image: %d\n", image_id); [Severity: Low] Is %d the correct format specifier here? image_id is a u32, so if the requested ID is greater than INT_MAX, it will be printed as a negative number. > + return -EINVAL; > + } > + context->active_image_id = image_id; > + return 0; > } [Severity: High] This is a pre-existing issue, but does device teardown systematically omit canceling read_data_work? When the driver is unbound, sahara_mhi_remove() calls cancel_work_sync() for fw_work and dump_work, but fails to cancel read_data_work. If read_data_work is pending or executing, it can run after remove() returns, leading to a use-after-free of the devres-managed context. [Severity: High] This is a pre-existing issue, but is there lockless concurrency between fw_work and read_data_work that allows a device-triggered use-after-free? During firmware streaming, sahara_read_data() schedules read_data_work. If a device sends a SAHARA_END_OF_IMAGE_CMD immediately after a read request, the command is processed by fw_work which calls sahara_release_image() to free context->firmware. Because these are independent work items without synchronization, could an untrusted MHI device deliberately race the EOI command against the executing streaming task? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260701-sahara_protocol_new_v2-v6-0-3a78362c4...@oss.qualcomm.com?part=4
