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