Re: [PATCH v5 2/2] dmaengine: dw-edma: Add changes to support Channel Separation

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Untrusted channel separation size from VSEC can cause out-of-bounds MMIO accesses beyond the mapped PCI BAR.

Pre-existing issues:
- [High] Allocated PCI IRQ vectors are leaked on multiple error paths in dw_edma_pcie_probe() due to missing cleanup.
--

--- Patch [2]: [PATCH v5 2/2] dmaengine: dw-edma: Add changes to support Channel Separation ---
commit a5388dc7284225085ff4d5827150b9b1002f1690
Author: Devendra K Verma <[email protected]>

dmaengine: dw-edma: Add changes to support Channel Separation

This patch implements configurable DMA channel register space separation,
ranging from 256B to 32KB, for HDMA. It updates offset calculations using
ch_space_sz based on the VSEC capability for CPM6 and removes unused V0
structs.

> diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
> index f287879b2cb9e..2eb8293b87b4c 100644
> --- a/drivers/dma/dw-edma/dw-edma-pcie.c
> +++ b/drivers/dma/dw-edma/dw-edma-pcie.c

[ ... ]

> @@ -436,6 +439,7 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
>  	chip->nr_irqs = nr_irqs;
>  	chip->ops = &dw_edma_pcie_plat_ops;
>  	chip->cfg_non_ll = non_ll;
> +	chip->ch_space_sz = vsec_data->ch_space_sz;

[Severity: High]
This is a pre-existing issue, but does dw_edma_pcie_probe() leak IRQ resources
on error paths?

The function successfully allocates IRQ vectors using pci_alloc_irq_vectors()
earlier in the probe. However, if any subsequent initialization step fails,
such as pcim_iomap_table() returning NULL:

	chip->reg_base = pcim_iomap_table(pdev)[vsec_data->rg.bar];
	if (!chip->reg_base)
		return -ENOMEM;

the function returns directly without calling pci_free_irq_vectors(). Could
this lead to exhausting system IRQ capacity after repeated probe failures?

>  
>  	chip->ll_wr_cnt = vsec_data->wr_ch_cnt;
>  	chip->ll_rd_cnt = vsec_data->rd_ch_cnt;

[ ... ]

> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> index 632abb8b481cf..f8b2383b294ac 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c

[ ... ]

>  static inline struct dw_hdma_v0_ch_regs __iomem *
>  __dw_ch_regs(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch)
>  {
> -	if (dir == EDMA_DIR_WRITE)
> -		return &(__dw_regs(dw)->ch[ch].wr);
> -	else
> -		return &(__dw_regs(dw)->ch[ch].rd);
> +	u32 ch_base;
> +
> +	/*
> +	 * For Write, the channel register index starts at
> +	 * wr_base(ch_idx) = (2 * ch_idx) * ch_space_sz
> +	 *
> +	 * For Read channel,
> +	 * rd_base(ch_idx) = (2 * ch_idx + 1) * ch_space_sz
> +	 */
> +	ch_base = 2 * ch;
> +	if (dir == EDMA_DIR_READ)
> +		ch_base += 1;
> +
> +	return dw->chip->reg_base + (ch_base * dw->chip->ch_space_sz);

[Severity: High]
Can an untrusted channel separation size from VSEC cause out-of-bounds MMIO
accesses beyond the mapped PCI BAR?

If a maliciously crafted PCIe endpoint device reports a small BAR size but
provides a large channel separation value via the VSEC capability,
dw_edma_pcie_get_xilinx_dma_data() will extract this untrusted value to set
ch_space_sz.

Since the calculated offset (ch_base * dw->chip->ch_space_sz) is not explicitly
bounds-checked against the actual mapped size of the BAR (such as by verifying
against pci_resource_len()), could this allow the subsequent SET_CH_32() macro
to dereference memory outside the mapped bounds and potentially cause a host
kernel panic?

>  }
>  
>  #define SET_CH_32(dw, dir, ch, name, value) \

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2