Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
Pre-existing issues: - [High] Unvalidated hardware event 'chid' used as an array index. - [High] Unvalidated hardware event 'tre_index' leads to out-of-bounds memory read. - [High] The driver drops gchan->vc.lock while a DMA descriptor remains on the virt-dma issued list, leading to a race with channel termination and a use-after-free. - [High] Error path in gpi_resume() leaves the hardware IRQ enabled while the software state remains paused, leading to unbalanced IRQ enables upon retry. - [High] The driver does not validate the physical address (`ptr`) returned in a DMA completion event, allowing untrusted input to corrupt ring buffer pointers. - [High] Missing dma_rmb() before reading the DMA event ring buffer. - [High] Missing memory barrier before ringing the event doorbell in gpi_process_events(). - [High] Missing dma_wmb() before writing to the channel doorbell in gpi_issue_pending(). -- commit 1bf9fce9a4cdb676de685f3966a35631e5323808 Author: Praveen Talari <[email protected]> dmaengine: qcom: gpi: Convert dev_dbg() calls to tracepoints Replace the remaining dev_dbg() based debug logging in the GPI DMA driver with the qcom_gpi tracepoints, providing structured runtime visibility into GPI DMA behavior without requiring invasive debug patches. dev_err() calls are left untouched. > diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c > index b09354a73b46e..8e7d25a461475 100644 > --- a/drivers/dma/qcom/gpi.c > +++ b/drivers/dma/qcom/gpi.c > @@ -945,17 +940,10 @@ static void gpi_process_imed_data_event(struct gchan > *gchan, > struct gpi_tre *gpi_tre; > > spin_unlock_irqrestore(&gchan->vc.lock, flags); > - dev_dbg(gpii->gpi_dev->dev, "event without a pending > descriptor!\n"); > gpi_ere = (struct gpi_ere *)imed_event; > - dev_dbg(gpii->gpi_dev->dev, > - "Event: %08x %08x %08x %08x\n", > - gpi_ere->dword[0], gpi_ere->dword[1], > - gpi_ere->dword[2], gpi_ere->dword[3]); > gpi_tre = tre; > - dev_dbg(gpii->gpi_dev->dev, > - "Pending TRE: %08x %08x %08x %08x\n", > - gpi_tre->dword[0], gpi_tre->dword[1], > - gpi_tre->dword[2], gpi_tre->dword[3]); > + trace_gpi_ev_no_desc(gpii->gpi_dev->dev, imed_event->chid, > + gpi_ere->dword, gpi_tre->dword); > return; > } [Severity: High] This isn't a bug introduced by this patch, but does this code lead to an out-of-bounds memory read? drivers/dma/qcom/gpi.c:gpi_process_imed_data_event() { ... void *tre = ch_ring->base + (ch_ring->el_size * imed_event->tre_index); ... gpi_tre = tre; trace_gpi_ev_no_desc(gpii->gpi_dev->dev, imed_event->chid, gpi_ere->dword, gpi_tre->dword); } The tre_index is read from imed_event->tre_index without a bounds check against ch_ring->elements. If it exceeds the ring size, will the tracepoint dereference out-of-bounds memory? [ ... ] > @@ -1064,11 +1052,10 @@ static void gpi_process_xfer_compl_event(struct gchan > *gchan, > dev_err(gpii->gpi_dev->dev, "Error in Transaction\n"); > result.result = DMA_TRANS_ABORTED; > } else { > - dev_dbg(gpii->gpi_dev->dev, "Transaction Success\n"); > result.result = DMA_TRANS_NOERROR; > } > result.residue = gpi_desc->len - compl_event->length; > - dev_dbg(gpii->gpi_dev->dev, "Residue %d\n", result.residue); > + trace_gpi_xfer_result(gpii->gpi_dev->dev, chid, result.result, > result.residue); > > dma_cookie_complete(&vd->tx); > dmaengine_desc_get_callback_invoke(&vd->tx, &result); [Severity: High] This is a pre-existing issue, but does this driver validate the physical address returned in a DMA completion event? drivers/dma/qcom/gpi.c:gpi_process_xfer_compl_event() { ... void *ev_rp = to_virtual(ch_ring, compl_event->ptr); ... } The compl_event->ptr is a physical address provided by the hardware DMA event, and to_virtual() converts it without bounds checking. If the hardware yields an invalid ptr outside the physical bounds of the ring buffer, can this corrupt the ring pointer tracking by assigning a bogus virtual pointer to ch_ring->rp? [Severity: High] This isn't a bug introduced by this patch, but can this code race with channel termination and cause a use-after-free? drivers/dma/qcom/gpi.c:gpi_process_xfer_compl_event() { ... gpi_free_desc: spin_lock_irqsave(&gchan->vc.lock, flags); list_del(&vd->node); spin_unlock_irqrestore(&gchan->vc.lock, flags); kfree(gpi_desc); } The interrupt handler drops gchan->vc.lock earlier after getting the descriptor via vchan_next_desc(), without removing it from the desc_issued list. If a concurrent vchan_terminate_all() acquires the lock, removes, and frees the descriptor, the IRQ handler will then resume, modify the freed descriptor (dma_cookie_complete), and call list_del(&vd->node) on freed memory. This also applies to gpi_process_imed_data_event() where the same pattern exists. [ ... ] > @@ -1100,11 +1087,8 @@ static void gpi_process_events(struct gpii *gpii) > chid = gpi_event->xfer_compl_event.chid; > type = gpi_event->xfer_compl_event.type; > > - dev_dbg(gpii->gpi_dev->dev, > - "Event: CHID:%u, type:%x %08x %08x %08x %08x\n", > - chid, type, gpi_event->gpi_ere.dword[0], > - gpi_event->gpi_ere.dword[1], > gpi_event->gpi_ere.dword[2], > - gpi_event->gpi_ere.dword[3]); > + trace_gpi_process_event(gpii->gpi_dev->dev, chid, type, > + gpi_event->gpi_ere.dword); > > switch (type) { > case XFER_COMPLETE_EV_TYPE: > gchan = &gpii->gchan[chid]; [Severity: High] This isn't a bug introduced by this patch, but is a dma_rmb() missing before reading the DMA event ring buffer? drivers/dma/qcom/gpi.c:gpi_process_events() { cntxt_rp = gpi_read_reg(gpii, gpii->ev_ring_rp_lsb_reg); rp = to_virtual(ev_ring, cntxt_rp); do { while (rp != ev_ring->rp) { gpi_event = ev_ring->rp; ... } Because gpi_read_reg() wraps readl_relaxed(), it provides no ordering guarantees against normal memory reads. Can the CPU speculatively fetch from the DMA-mapped event ring before the register read completes and confirms there is new data? [Severity: High] This is a pre-existing issue, but does this code use an unvalidated hardware event chid as an array index? drivers/dma/qcom/gpi.c:gpi_process_events() { ... chid = gpi_event->xfer_compl_event.chid; type = gpi_event->xfer_compl_event.type; trace_gpi_process_event(gpii->gpi_dev->dev, chid, type, gpi_event->gpi_ere.dword); switch (type) { case XFER_COMPLETE_EV_TYPE: gchan = &gpii->gchan[chid]; ... } If the hardware is compromised or fails and sends a chid value greater than or equal to MAX_CHANNELS_PER_GPII, indexing gpii->gchan[chid] will read out of bounds. Can this corrupt adjacent memory or crash the kernel? [ ... ] > @@ -1121,11 +1104,8 @@ static void gpi_process_events(struct gpii *gpii) > > &gpi_event->immediate_data_event); > break; > case QUP_NOTIF_EV_TYPE: > - dev_dbg(gpii->gpi_dev->dev, > "QUP_NOTIF_EV_TYPE\n"); > break; > default: > - dev_dbg(gpii->gpi_dev->dev, > - "not supported event type:0x%x\n", > type); > } > gpi_ring_recycle_ev_element(ev_ring); > } [Severity: High] This is a pre-existing issue, but is a memory barrier missing before ringing the event doorbell? drivers/dma/qcom/gpi.c:gpi_process_events() { ... gpi_ring_recycle_ev_element(ev_ring); } gpi_write_ev_db(gpii, ev_ring, ev_ring->wp); ... } The gpi_write_ev_db() wraps writel_relaxed(). The smp_wmb() inside gpi_ring_recycle_ev_element() only orders normal memory writes, not prior memory reads against subsequent MMIO writes. Does this mean the hardware might overwrite the DMA event ring descriptor before the CPU actually finishes reading it? A full mb() or rmb() is typically required before the doorbell. [ ... ] > @@ -1578,7 +1554,7 @@ static int gpi_resume(struct dma_chan *chan) > > mutex_lock(&gpii->ctrl_lock); > if (gpii->pm_state == ACTIVE_STATE) { > - dev_dbg(gpii->gpi_dev->dev, "channel is already active\n"); > + trace_gpi_already_state(gpii->gpi_dev->dev, gpii->gpii_id, > gpii->pm_state); > mutex_unlock(&gpii->ctrl_lock); > return 0; > } [Severity: High] This isn't a bug introduced by this patch, but does this error path leave the hardware IRQ enabled while the software state remains paused? drivers/dma/qcom/gpi.c:gpi_resume() { ... enable_irq(gpii->irq); for (i = 0; i < MAX_CHANNELS_PER_GPII; i++) { ret = gpi_send_cmd(gpii, &gpii->gchan[i], GPI_CH_CMD_START); if (ret) { dev_err(gpii->gpi_dev->dev, "Error starting chan, ret:%d\n", ret); mutex_unlock(&gpii->ctrl_lock); return ret; } ... } If gpi_send_cmd() fails, it returns without disabling the IRQ, and pm_state remains PAUSE_STATE. On the next resume attempt, will it bypass the active state check and call enable_irq() a second time, resulting in an unbalanced IRQ enable and a kernel warning? [Severity: High] This is a pre-existing issue, but is a dma_wmb() missing before writing to the channel doorbell? drivers/dma/qcom/gpi.c:gpi_issue_pending() { for (i = 0; i < gpi_desc->num_tre; i++) { tre = &gpi_desc->tre[i]; gpi_queue_xfer(gpii, gchan, tre, &wp); } gpi_desc->db = ch_ring->wp; gpi_write_ch_db(gchan, &gchan->ch_ring, gpi_desc->db); } Descriptors are written to the DMA ring using memcpy() in gpi_queue_xfer(), and then the doorbell is rung via gpi_write_ch_db(), which wraps writel_relaxed(). Since writel_relaxed() does not order against prior normal memory writes, can the doorbell write reach the device before the memcpy() is visible in RAM? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260909-add-trace-support-gpio-v2-0-01f81739b...@oss.qualcomm.com?part=3
