Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [High] The function returns -ETIMEDOUT and frees DMA descriptors on timeout without safely aborting the active DMA transfer, leading to hardware use-after-free, memory corruption, and spurious completion races. - [High] The newly added -ETIMEDOUT return value is ignored by the caller, causing failed DMA transfers to be reported as successfully completed to userspace. - [High] Interrupt enablement state is modified without holding the required subsystem spinlock, causing a data race with other subdevices. - [High] The exported generic DMA copy function unconditionally writes to descr[-1], resulting in a buffer underflow. -- commit 9ab471ce4cf985645becf738f0f42716441159d1 Author: Pengpeng Hou <[email protected]> fbdev: via: return an error when DMA copy times out viafb_dma_copy_out_sg() logs a VIA DMA timeout when the DONE bit is not set after the completion wait and grace delay, but still returns success to the caller. Preserve the existing cleanup sequence and return -ETIMEDOUT when the DMA engine did not report completion. > diff --git a/drivers/video/fbdev/via/via-core.c > b/drivers/video/fbdev/via/via-core.c > index a8d4a3e2c65ed..1632a669941ab 100644 > --- a/drivers/video/fbdev/via/via-core.c > +++ b/drivers/video/fbdev/via/via-core.c > @@ -234,6 +234,7 @@ int viafb_dma_copy_out_sg(unsigned int offset, struct > scatterlist *sg, int nsg) > dma_addr_t descr_handle; > unsigned long flags; > int i; > + int ret = 0; > struct scatterlist *sgentry; > dma_addr_t nextdesc; [Severity: High] This is a pre-existing issue, but does viafb_dma_copy_out_sg() unconditionally write to descr[-1] here? If viafb_dma_copy_out_sg() is called with nsg == 0, the for_each_sg loop is bypassed and the descr pointer remains at the start of the allocation. The function then does: descr[-1].next_desc_low = VIAFB_DMA_FINAL_SEGMENT|VIAFB_DMA_MAGIC; Because dma_alloc_coherent() can successfully allocate memory and not return NULL for size 0, does this proceed and corrupt the memory immediately preceding the allocation? > > @@ -290,8 +291,10 @@ int viafb_dma_copy_out_sg(unsigned int offset, struct > scatterlist *sg, int nsg) > */ > wait_for_completion_timeout(&viafb_dma_completion, 1); > msleep(1); > - if ((viafb_mmio_read(VDMA_CSR0)&VDMA_C_DONE) == 0) > + if ((viafb_mmio_read(VDMA_CSR0) & VDMA_C_DONE) == 0) { > printk(KERN_ERR "VIA DMA timeout!\n"); > + ret = -ETIMEDOUT; > + } > /* > * Clean up and we're done. > */ > @@ -301,7 +304,7 @@ int viafb_dma_copy_out_sg(unsigned int offset, struct > scatterlist *sg, int nsg) > dma_free_coherent(&global_dev.pdev->dev, > nsg*sizeof(struct viafb_vx855_dma_descr), descrpages, > descr_handle); [Severity: High] This isn't a bug introduced by this patch, but if wait_for_completion_timeout() times out, is the DMA hardware still actively reading from memory? The error path proceeds to dma_free_coherent() to unmap and free the descriptor pages without explicitly aborting the DMA transfer (e.g. using VDMA_C_ABORT). Could the running hardware continue reading from the freed memory, leading to physical memory corruption and falsely complete a subsequent DMA transfer via the global viafb_dma_completion variable? > - return 0; > + return ret; [Severity: High] This isn't a bug introduced by this patch, but is this newly added -ETIMEDOUT return value ignored by the caller in drivers/media/platform/via/via-camera.c? Looking at viacam_irq(): viafb_dma_copy_out_sg(cam->cb_offsets[bufn], sgt->sgl, sgt->nents); vb->vbuf.sequence = cam->sequence++; ... vb2_buffer_done(&vb->vbuf, VB2_BUF_STATE_DONE); When the DMA times out, viacam_irq() silently ignores the newly added error code and unconditionally passes the buffer to userspace. Will userspace erroneously receive incomplete or corrupted video frames, defeating the purpose of this patch? > } > EXPORT_SYMBOL_GPL(viafb_dma_copy_out_sg); [Severity: High] This is a pre-existing issue, but do viafb_request_dma() and viafb_release_dma() in drivers/video/fbdev/via/via-core.c modify the interrupt enablement state without holding global_dev.reg_lock? Looking at viafb_request_dma(): viafb_irq_enable(VDE_I_DMA0TDEN); and viafb_release_dma(): viafb_irq_disable(VDE_I_DMA0TDEN); Does this create a data race with other subdevices that correctly hold the lock, potentially corrupting the interrupt enablement register and permanently disabling other subdevices' interrupts? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
