Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
New issues: - [High] The MHI downlink completion callback acquires a mutex and makes a GFP_KERNEL allocation while in a tasklet context. - [High] The driver limits the number of queued RX buffers for the training payload but never requeues them, stalling the protocol and creating an incomplete MHI chain. - [High] Sequential MHI commands concurrently overwrite the shared `context->tx[0]` buffer before the asynchronous MHI DMA can process them. - [High] Attaching devres to the parent controller device without explicitly cleaning it up during module unload leads to a use-after-free kernel panic. - [High] A data race on `context->training_nbuf` causes the DL callback to miss training buffers, dropping data and re-processing stale control packets. - [High] Inadequate state tracking risks misinterpreting raw command list data as a protocol packet. - [High] Concurrent devres allocation can lead to multiple training data structures and memory leaks. - [Medium] The validation check for negative response length is tautologically false due to an unsigned integer comparison. - [Medium] A zero-length training data response causes the driver state machine to hang permanently. - [Medium] The sysfs node exposes uninitialized or zero-padded data to userspace during an active MHI transfer. Pre-existing issues: - [High] The driver cancels workqueues before stopping the MHI channel, creating a race condition that leads to a use-after-free. -- commit 3f86759313db40d3845a0cb024dac27dcf18f52b 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 b5ca6353540dc..07bc743aa061d 100644 > --- a/drivers/bus/mhi/host/clients/sahara/sahara.c > +++ b/drivers/bus/mhi/host/clients/sahara/sahara.c [ ... ] > @@ -181,6 +206,24 @@ struct sahara_context { > bool is_mem_dump_mode; > bool non_streaming; > const char *fw_folder; > + bool is_cmd_mode; > + bool receiving_training_data; > + size_t training_size; > + size_t training_rcvd; > + u32 training_nbuf; > + char *cmd_buff[SAHARA_NUM_CMD_BUF]; > +}; > + > +/* > + * Controller-scoped training data store (per MHI controller device). > + * Stored as devres resource on mhi_dev->mhi_cntrl->mhi_dev->dev. > + */ > +struct sahara_cntrl_training_data { > + struct mutex lock; /* Protects data, size, copied and receiving */ > + void *data; > + size_t size; > + size_t copied; > + bool receiving; > }; [Severity: Medium] Does the sysfs node expose uninitialized or zero-padded data to userspace during an active MHI transfer? The sysfs read function ddr_training_data_read() serves up to ct->size bytes of ct->data. Because ct->size is set to the full expected response length before the data is actually received, and the read function does not check if the transfer is complete (e.g., via ct->receiving) or limit the copy to ct->copied, a userspace process reading concurrently could receive uninitialized memory. > static bool is_streaming(struct sahara_context *context) [ ... ] > +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] Can concurrent calls to this function lead to multiple training data structures and memory leaks? Because devres_find() and devres_alloc() are used in a check-and-add pattern without locking, if this is called concurrently from the sysfs read callback and the MHI DL callback, both threads could see a NULL result, allocate their own structures, and add them. Would this result in multiple entries in the devres list and inconsistent states? > + if (!ct) > + return NULL; > + > + mutex_init(&ct->lock); > + ct->data = NULL; > + ct->size = 0; > + ct->copied = 0; > + ct->receiving = false; > + > + devres_add(dev, ct); > + return ct; > +} [ ... ] > +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 negative response length check tautologically false? Since le32_to_cpu() returns an unsigned 32-bit integer, this condition always evaluates to false. Could a malicious or malfunctioning device send a massive length, causing a large kzalloc() attempt to fail? > + 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); > + return; > + } [Severity: High] Will these sequential commands overwrite the shared buffer before transmission? sahara_command_execute_data() populates context->tx[0] and queues it via mhi_queue_buf() which schedules an asynchronous DMA transfer. Immediately afterward, sahara_command_execute() overwrites the exact same context->tx[0] buffer. Does this corrupt the outgoing protocol messages and queue the same virtual address into the MHI ring twice? [Severity: High] Is there a risk of misinterpreting raw command list data as a protocol packet? For SAHARA_EXEC_CMD_GET_COMMAND_ID_LIST, the device streams the command list raw data into context->rx. Since receiving_training_data is only set for ID 9 later in this function, does sahara_command_processing() process the raw command list data as protocol packets? > + > + if (client_cmd != SAHARA_EXEC_CMD_GET_TRAINING_DATA) > + 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] Does a zero-length response cause the driver state machine to hang? If resp_len is 0, the queuing loop is skipped entirely because remaining is 0. With receiving_training_data set to true, context->rx won't be re-queued in sahara_command_processing(). Does this mean no buffers are queued in hardware, preventing the driver from ever receiving an event to exit command mode? > + 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] Could a data race on context->training_nbuf cause the DL callback to miss training buffers? This loop queues cmd_buff receive buffers, but context->training_nbuf is updated only after the loop completes. If the device responds quickly, the DL callback for the first buffer might fire concurrently. Since training_nbuf would still be 0, the callback's loop would skip matching the training buffer and incorrectly fall through to the control packet path. [ ... ] > @@ -885,6 +1140,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] This is a pre-existing issue, but does the inverted cleanup order here create a race condition leading to a use-after-free? The driver synchronously cancels all works before calling mhi_unprepare_from_transfer(mhi_dev). Because the MHI channel is still active, an incoming DL completion can trigger the DL callback and reschedule a work item after the cancellation. When module removal completes, context is freed, and the scheduled work will access freed memory. [Severity: High] Does the module removal function exit without unregistering the persistent devres structures from the controller? The driver allocates a devres struct sahara_cntrl_training_data on the MHI controller device to outlive the Sahara channel. However, the release callbacks are defined in the sahara module. If sahara_mhi_remove() does not remove these controller-bound resources, will the kernel attempt to execute the devres callbacks located in freed module memory when the controller is later destroyed? > @@ -901,15 +1157,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; [Severity: High] Does this callback acquire a sleeping lock and perform blocking memory allocation while in an atomic context? sahara_mhi_dl_xfer_cb() is executed by the MHI core in an atomic context (typically a tasklet). However, it calls sahara_cntrl_training_get(dev) which allocates memory with GFP_KERNEL, and directly calls mutex_lock(&ct->lock) below. Will this trigger a 'BUG: scheduling while atomic' kernel panic during normal training data reception? > > - 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); > + 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) { [Severity: High] If the response requires more buffers than initially queued, does the transfer stall permanently? In sahara_command_execute_resp(), the driver queues up to SAHARA_NUM_CMD_BUF buffers. If the response size exceeds the total capacity, the loop finishes with the last buffer incorrectly flagged with MHI_CHAIN. When those buffers complete, this callback accumulates training_rcvd but fails to requeue additional buffers. Will training_rcvd never reach training_size, leaving the MHI channel hung waiting for the chain to complete? > + mutex_lock(&ct->lock); > + ct->receiving = false; > + mutex_unlock(&ct->lock); > + > + context->receiving_training_data = > false; > + context->is_cmd_mode = false; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260701-sahara_protocol_new_v2-v6-0-3a78362c4...@oss.qualcomm.com?part=6
