Re: [PATCH v6 4/7] bus: mhi: Add QDU100 Sahara variant and firmware fallback

[sashiko-bot@kernel.org]

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 <kishore.batta@oss.qualcomm.com>

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-3a78362c4741@oss.qualcomm.com?part=4