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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] mhi: Sleeping in atomic context due to mutex_lock() in MHI transfer callback
- [High] mhi: Data corruption due to concurrent modification of shared TX buffer
- [High] mhi: TOCTOU race condition dropping valid RX completions
- [High] bus: mhi: sleeping in atomic context during DL transfer callback
- [High] bus: mhi: TX buffer reuse race and raw payload misinterpretation in Sahara
- [High] mhi: UAF / crash due to parent devres callback on module unload
- [Medium] mhi: Permanent channel hang due to un-requeued RX buffers for large transfers
- [Medium] mhi: Dead code in bounds validation due to unsigned comparison
- [Medium] mhi: Permanent channel hang due to unhandled zero-length response
--

commit 80f9d9bf2b27a92703cf2016cc04d3f992e75657
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 62b9c72e82680..aa684355b7a31 100644
> --- a/drivers/bus/mhi/host/clients/sahara/sahara.c
> +++ b/drivers/bus/mhi/host/clients/sahara/sahara.c

[ ... ]

> +static struct sahara_cntrl_training_data *sahara_cntrl_training_get(struct device *dev)
> +{
> +	struct sahara_cntrl_training_data *ct;
> +
> +	ct = devres_find(dev, sahara_cntrl_training_release,
> +			 sahara_cntrl_training_match, NULL);
> +	if (ct)
> +		return ct;
> +
> +	ct = devres_alloc(sahara_cntrl_training_release, sizeof(*ct), GFP_KERNEL);

[Severity: High]
Does allocating devres memory with GFP_KERNEL here cause a sleeping in atomic
context bug?

This function is called from sahara_mhi_dl_xfer_cb(), which is executed from
the MHI event tasklet under a spinlock, creating an atomic context.

[Severity: High]
Can attaching this devres to the parent MHI controller device cause a
use-after-free on module unload?

Since the sahara module does not detach this devres on removal to persist the
data, when the parent controller device is later unbound, devres_release_all()
will call the sahara_cntrl_training_release pointer which resides in the
unloaded module's unmapped memory.

[ ... ]

> +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]
Is this bounds validation effective?

Since le32_to_cpu() returns an unsigned u32, checking if it is < 0 is
tautologically false and bypasses the validation.

> +		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]
Can this concurrent reuse of the TX buffer cause data corruption or protocol
failure?

The driver queues context->tx[0] via sahara_command_execute_data() and
immediately overwrites the same buffer in sahara_command_execute() without
waiting for the MHI UL completion.

Additionally, pipelining these commands causes the ID list raw payload to
stream back to the host while receiving_training_data is false. Will
sahara_command_processing() then incorrectly interpret this raw data as a
Sahara command header?

> +		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;
> +	}
> +
> +	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++) {

[Severity: Medium]
If the device reports a response_length of 0, will this loop execute zero
times, queueing no RX buffers and hanging the channel since no completions
will arrive to trigger the state machine?

> +		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: High]
Does updating context->training_nbuf after queuing the buffers create a race
condition?

Since MHI queue operations are asynchronous, a completion could arrive
on another CPU and run sahara_mhi_dl_xfer_cb() before the loop finishes. If
training_nbuf is still 0, the callback might silently drop the valid
completion and stall the state machine.

[ ... ]

> @@ -899,15 +1155,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]
Does acquiring a sleeping mutex here cause a scheduling while atomic bug?

The MHI downlink transfer callback is invoked from a tasklet context
(mhi_ev_task) holding a spinlock, which creates an atomic execution context.

> +				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;

[Severity: Medium]
If the payload reported by the device is larger than the initially queued
buffer capacity, does returning here starve the MHI channel?

Since additional RX buffers are not queued via mhi_queue_buf() for the
remaining data, the Sahara state machine could permanently hang waiting for
data that cannot arrive.

> +			}
> +		}
> +		return;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-sahara_protocol_new_v2-v9-0-dc9f24357c4a@oss.qualcomm.com?part=6