Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Descriptor preparation functions return ERR_PTR instead of NULL and leak memory on failure. - [High] Out-of-bounds memory access in at_xdmac_tx_status() when the transfers list is empty. -- commit 3b17906d45663cb4c1aae18a74a2d9d31cfc3f4d Author: Frank Li <[email protected]> dmaengine: add (dmaengine|vchan)_chan_dev() helper This commit adds new helper functions (dmaengine|vchan)_chan_dev() to access the DMA channel device. It replaces private chan2dev() implementations in individual DMA engine drivers to provide a cleaner common API. > diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c > index 901971e8bae69..d40a8b40dddc3 100644 > --- a/drivers/dma/at_xdmac.c > +++ b/drivers/dma/at_xdmac.c [ ... ] > @@ -1667,7 +1662,7 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t > cookie, > > dma_set_residue(txstate, residue); > > - dev_dbg(chan2dev(chan), > + dev_dbg(dmaengine_chan_dev(chan), > "%s: desc=0x%p, tx_dma_desc.phys=%pad, tx_status=%d, > cookie=%d, residue=%d\n", > __func__, desc, &desc->tx_dma_desc.phys, ret, cookie, residue); [Severity: High] This is a pre-existing issue, but is there a potential out-of-bounds access in at_xdmac_tx_status() when the transfers list is empty? If dmaengine_tx_status() is called after at_xdmac_device_terminate_all() empties the atchan->xfers_list, dma_cookie_status() might still return DMA_IN_PROGRESS because the cookie is not marked as completed. Earlier in at_xdmac_tx_status(), the code blindly calls list_first_entry() on atchan->xfers_list without checking if it is empty, and then accesses desc->active_xfer. Since list_first_entry() does not verify emptiness, this interprets the list_head pointer as a structure, which could lead to an invalid memory access or a crash. [ ... ] > diff --git a/drivers/dma/loongson/loongson2-apb-cmc-dma.c > b/drivers/dma/loongson/loongson2-apb-cmc-dma.c > index 2f1c8c4fd0654..755363f4bfa2c 100644 > --- a/drivers/dma/loongson/loongson2-apb-cmc-dma.c > +++ b/drivers/dma/loongson/loongson2-apb-cmc-dma.c [ ... ] > @@ -409,7 +404,7 @@ loongson2_cmc_dma_prep_slave_sg(struct dma_chan *chan, > struct scatterlist *sgl, > > num_items = DIV_ROUND_UP(sg_dma_len(sg), buswidth); > if (num_items >= LOONSON2_CMCDMA_MAX_DATA_ITEMS) { > - dev_err(chan2dev(lchan), "Number of items not > supported\n"); > + dev_err(vchan_chan_dev(&lchan->vchan), "Number of items > not supported\n"); > kfree(desc); > return ERR_PTR(-EINVAL); > } [Severity: High] This isn't a bug introduced by this patch, but shouldn't the dmaengine API functions device_prep_slave_sg() and device_prep_dma_cyclic() return NULL on failure rather than ERR_PTR()? In loongson2_cmc_dma_prep_slave_sg(), if loongson2_cmc_dma_set_xfer_param() fails or the item count is unsupported, it returns ERR_PTR. Since callers typically check for !desc on failure, returning ERR_PTR means the error check passes, which could lead to a kernel panic when the invalid pointer is dereferenced. Additionally, earlier in the same loop, if set_xfer_param() fails, the previously allocated desc via kzalloc_flex() appears to be leaked before returning ERR_PTR(ret). > @@ -447,7 +442,7 @@ loongson2_cmc_dma_prep_dma_cyclic(struct dma_chan *chan, > dma_addr_t buf_addr, si > > num_items = DIV_ROUND_UP(period_len, buswidth); > if (num_items >= LOONSON2_CMCDMA_MAX_DATA_ITEMS) { > - dev_err(chan2dev(lchan), "Number of items not supported\n"); > + dev_err(vchan_chan_dev(&lchan->vchan), "Number of items not > supported\n"); > return ERR_PTR(-EINVAL); > } [Severity: High] This is also a pre-existing issue, but similar to the above, does this return value violate the API expectations? Returning ERR_PTR(-EINVAL) here and when loongson2_cmc_dma_set_xfer_param() fails will likely bypass caller checks for NULL, potentially leading to a crash on dereference. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
