Re: [PATCH v8 6/7] bus: mhi: Capture DDR training data via command mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] mhi: sleep in atomic context via mutex_lock in dl_xfer_cb
- [High] mhi: module unload panic via leaked controller devres
- [High] mhi: DMA buffer corruption from concurrent tx[0] reuse
- [High] mhi: UAF due to incorrect teardown order in sahara_mhi_remove
- [Medium] mhi: unbounded memory allocation due to unsigned tautology
- [Medium] mhi: transfer stall for large or zero DDR training payloads
- [Medium] mhi: TOCTOU race leading to dropped training packets
- [Medium] mhi: protocol desynchronization if training buffer allocation fails
--

commit 1abdc448b94ff31d485a817b37e870ca14bbbfd6
Author: Kishore Batta <kishore.batta@oss.qualcomm.com>

    bus: mhi: Capture DDR training data via command mode
    
    During early boot, devices may perform DDR training and produce training
    data that can be reused on subsequent boots to reduce initialization
    time. The Sahara protocol provides a command mode flow to transfer this
    training data to the host, but the driver currently does not handle
    command mode and drops the training payload.

> diff --git a/drivers/bus/mhi/host/clients/sahara/sahara.c b/drivers/bus/mhi/host/clients/sahara/sahara.c
> index afb72910fd67b..e7a6289b11a72 100644
> --- a/drivers/bus/mhi/host/clients/sahara/sahara.c
> +++ b/drivers/bus/mhi/host/clients/sahara/sahara.c

[ ... ]

> @@ -335,6 +426,153 @@ static void sahara_hello(struct sahara_context *context)
> +static void sahara_command_execute_resp(struct sahara_context *context)
> +{
> +	struct device *dev = &context->mhi_dev->mhi_cntrl->mhi_dev->dev;
> +	struct sahara_cntrl_training_data *ct;
> +	u32 client_cmd, resp_len;
> +	int ret;
> +	u64 remaining;
> +	u32 i;
> +
> +	if (le32_to_cpu(context->rx->length) != SAHARA_COMMAND_EXEC_RESP_LENGTH ||
> +	    le32_to_cpu(context->rx->command_execute_resp.response_length) < 0) {

[Severity: Medium]
Since le32_to_cpu() returns a 32-bit unsigned integer, is the < 0 check
mathematically impossible? This would allow a malfunctioning device to provide
an exceptionally large response_length that bypasses the check and causes a
massive allocation in kzalloc().

> +		dev_err(&context->mhi_dev->dev,
> +			"Malformed command execute resp packet - length %d\n",
> +			le32_to_cpu(context->rx->length));
> +		return;
> +	}
> +
> +	client_cmd = le32_to_cpu(context->rx->command_execute_resp.client_command);
> +	resp_len = le32_to_cpu(context->rx->command_execute_resp.response_length);
> +
> +	sahara_command_execute_data(context, client_cmd);
> +
> +	if (client_cmd == SAHARA_EXEC_CMD_GET_COMMAND_ID_LIST) {
> +		sahara_command_execute(context, SAHARA_EXEC_CMD_GET_TRAINING_DATA);

[Severity: High]
Does calling sahara_command_execute_data() followed immediately by
sahara_command_execute() queue the same context->tx[0] buffer twice?

Since DMA buffers are submitted by reference, modifying context->tx[0] before
the hardware has consumed the first packet could cause data corruption.

> +		return;
> +	}
> +
> +	if (client_cmd != SAHARA_EXEC_CMD_GET_TRAINING_DATA)
> +		return;
> +
> +	ct = sahara_cntrl_training_get(dev);
> +	if (!ct) {
> +		context->is_cmd_mode = false;
> +		sahara_switch_mode_to_img_tx(context);
> +		return;
> +	}
> +
> +	mutex_lock(&ct->lock);
> +	kfree(ct->data);
> +	ct->data = kzalloc(resp_len, GFP_KERNEL);
> +	ct->size = resp_len;
> +	ct->copied = 0;
> +	ct->receiving = true;
> +	mutex_unlock(&ct->lock);
> +
> +	if (!ct->data) {
> +		context->is_cmd_mode = false;
> +		sahara_switch_mode_to_img_tx(context);
> +		return;
> +	}

[Severity: Medium]
If kzalloc() fails, we return and switch back to IMAGE_TX_PENDING without
setting context->receiving_training_data = true.

However, sahara_command_execute_data() was already sent earlier, instructing
the device to transmit the payload. If the device starts streaming data, won't
fw_work misinterpret the raw payload as Sahara protocol messages?

> +
> +	context->training_size = resp_len;
> +	context->training_rcvd = 0;
> +	context->receiving_training_data = true;
> +
> +	remaining = resp_len;
> +	for (i = 0; i < SAHARA_NUM_CMD_BUF && remaining; i++) {
> +		size_t pkt = min_t(size_t, remaining, SAHARA_PACKET_MAX_SIZE);
> +
> +		ret = mhi_queue_buf(context->mhi_dev, DMA_FROM_DEVICE,
> +				    context->cmd_buff[i], pkt,
> +				    (remaining <= pkt) ? MHI_EOT : MHI_CHAIN);
> +		if (ret)
> +			break;
> +
> +		remaining -= pkt;
> +	}
> +
> +	context->training_nbuf = i;
> +}

[Severity: Medium]
Is there a race condition where mhi_queue_buf() allows the hardware to start
processing buffers immediately, before context->training_nbuf is updated?

If a buffer completes quickly on another CPU, sahara_mhi_dl_xfer_cb() might
check "i < context->training_nbuf" while training_nbuf is still 0, failing to
recognize the buffer as training data.

[ ... ]

> @@ -884,6 +1139,7 @@ static void sahara_mhi_remove(struct mhi_device *mhi_dev)
>  
>  	cancel_work_sync(&context->fw_work);
>  	cancel_work_sync(&context->dump_work);
> +	cancel_work_sync(&context->cmd_work);
>  	vfree(context->mem_dump);
>  	sahara_release_image(context);
>  	mhi_unprepare_from_transfer(mhi_dev);

[Severity: High]
Does cancelling the workqueues before calling mhi_unprepare_from_transfer()
leave a window where incoming completions can re-arm the workqueue?

sahara_mhi_dl_xfer_cb() could run in this window and 
schedule_work(&context->cmd_work). When remove finishes, context is freed, 
which could result in a Use-After-Free when the workqueue executes.

[Severity: High]
Does this function also need to clean up the devres allocated in
sahara_cntrl_training_get()?

The devres is attached to the parent MHI controller. If the Sahara driver
module is unloaded without explicitly detaching it, the release callback
pointer will become stale. When the MHI controller is subsequently removed,
the devres core will invoke the unmapped callback, causing a panic.

[ ... ]

> @@ -900,15 +1156,71 @@ static void sahara_mhi_ul_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result
>  static void sahara_mhi_dl_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result *mhi_result)
>  {
>  	struct sahara_context *context = dev_get_drvdata(&mhi_dev->dev);
> +	struct sahara_cntrl_training_data *ct;
> +	struct device *dev;
> +	size_t copy;
> +	int ret;
> +	u32 i;
> +
> +	if (mhi_result->transaction_status)
> +		return;
> +
> +	/*
> +	 * Raw training payload completions arrive for cmd_buff[] buffers.
> +	 * Do not schedule cmd_work for those.
> +	 */
> +	if (context->is_cmd_mode && context->receiving_training_data &&
> +	    mhi_result->buf_addr != context->rx) {
> +		dev = &context->mhi_dev->mhi_cntrl->mhi_dev->dev;
> +		ct = sahara_cntrl_training_get(dev);
> +		if (!ct)
> +			return;
>  
> -	if (!mhi_result->transaction_status) {
> -		context->rx_size = mhi_result->bytes_xferd;
> -		if (context->is_mem_dump_mode)
> -			schedule_work(&context->dump_work);
> -		else
> -			schedule_work(&context->fw_work);
> +		for (i = 0; i < context->training_nbuf; i++) {
> +			if (mhi_result->buf_addr == context->cmd_buff[i]) {
> +				mutex_lock(&ct->lock);

[Severity: High]
sahara_mhi_dl_xfer_cb() is an MHI transfer completion callback which executes
in atomic context (softirq). Will acquiring a sleepable mutex here trigger a
'scheduling while atomic' BUG?

> +				copy = min_t(size_t, mhi_result->bytes_xferd,
> +					     ct->size - ct->copied);
> +				memcpy((u8 *)ct->data + ct->copied,
> +				       mhi_result->buf_addr, copy);
> +				ct->copied += copy;
> +				mutex_unlock(&ct->lock);
> +
> +				context->training_rcvd += copy;
> +
> +				if (context->training_rcvd >= context->training_size) {
> +					mutex_lock(&ct->lock);
> +					ct->receiving = false;
> +					mutex_unlock(&ct->lock);
> +
> +					context->receiving_training_data = false;
> +					context->is_cmd_mode = false;
> +
> +					sahara_switch_mode_to_img_tx(context);
> +					ret = mhi_queue_buf(context->mhi_dev,
> +							    DMA_FROM_DEVICE,
> +							    context->rx,
> +							    SAHARA_PACKET_MAX_SIZE,
> +							    MHI_EOT);
> +					if (ret)
> +						dev_err(&context->mhi_dev->dev,
> +							"Unable to requeue rx buf %d\n", ret);
> +				}
> +				return;
> +			}
> +		}
> +		return;
>  	}

[Severity: Medium]
If the device payload is larger than the capacity of the initially queued
SAHARA_NUM_CMD_BUF buffers, the remainder is not queued. Since the buffer is
not requeued here after its contents are copied, won't the transfer stall
permanently waiting for more buffers?

Similarly, if the response_length is 0, zero buffers are queued in
sahara_command_execute_resp(). Will this also stall the state machine?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-sahara_protocol_new_v2-v8-0-e12939d229a1@oss.qualcomm.com?part=6