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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly introduced `ch_sep_sz` field in `struct dw_edma_chip` is uninitialized (defaults to 0) for platform drivers outside of `dw-edma-pcie` that use HDMA natively, causing all channel register accesses to map to identical offset 0.
- [High] The driver trusts the device-provided VSEC configuration to calculate DMA channel register offsets but never verifies if these offsets remain within the mapped PCI BAR region.

Pre-existing issues:
- [High] Pre-existing double free of IRQ vectors in `dw_edma_pcie_remove()` due to devres interference.
--

--- Patch [3]: [PATCH v1 3/3] dmaengine: dw-edma: Add changes to support Channel Separation ---
commit 2ad7c860cd4c4a1bef961b814db689f718e1407c
Author: Devendra K Verma <devverma@amd.com>

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

This patch modifies the dw-edma driver to support configurable channel
separation sizes. It updates the register offset calculations to use a
dynamic size provided by the hardware IP instead of assuming a default
256-byte separation.

> diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
> index 6295d01ba2f71..ca2a4be2f3a50 100644
> --- a/drivers/dma/dw-edma/dw-edma-pcie.c
> +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
[ ... ]
> @@ -443,6 +446,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_sep_sz = vsec_data->ch_sep_sz;
>  
>  	chip->ll_wr_cnt = vsec_data->wr_ch_cnt;
>  	chip->ll_rd_cnt = vsec_data->rd_ch_cnt;

[Severity: High]
This is a pre-existing issue, but does dw_edma_pcie_remove() explicitly call
pci_free_irq_vectors(pdev)?

Since pcim_enable_device(pdev) is called during dw_edma_pcie_probe(), it
automatically manages subsequent IRQ vectors via devres. When the driver is
unbound, the devres cleanup routine will attempt to free the already-freed
vectors, which could cause a double-free kernel panic on the device unbind
or module unload path.

> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> index 632abb8b481cf..3f27a93c4d8c8 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
[ ... ]
> @@ -23,18 +23,23 @@ enum dw_hdma_control {
>  	DW_HDMA_V0_LLE					= BIT(9),
>  };
>  
> -static inline struct dw_hdma_v0_regs __iomem *__dw_regs(struct dw_edma *dw)
> -{
> -	return dw->chip->reg_base;
> -}
> -
>  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_sep_sz
> +	 *
> +	 * For Read channel,
> +	 * rd_base(ch_idx) = (2 * ch_idx + 1) * ch_sep_sz
> +	 */
> +	ch_base = 2 * ch;
> +	if (dir == EDMA_DIR_READ)
> +		ch_base += 1;
> +
> +	return dw->chip->reg_base + (ch_base * dw->chip->ch_sep_sz);

[Severity: High]
Does this code verify that the computed channel register offset remains within
the mapped PCI BAR region?

If a malicious or misconfigured PCIe endpoint provides a large channel
separation size, could this calculation result in out-of-bounds MMIO accesses?

[Severity: High]
If platform drivers outside of dw-edma-pcie use HDMA natively and allocate
struct dw_edma_chip via devm_kzalloc without explicitly initializing
ch_sep_sz, does ch_sep_sz default to 0?

Could this cause all channel register accesses to map to offset 0, leading to
data corruption by incorrectly targeting Write Channel 0's registers?

>  }
>  
> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-debugfs.c b/drivers/dma/dw-edma/dw-hdma-v0-debugfs.c
> index dcdc57fe976c1..33128685bbd90 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-debugfs.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-debugfs.c
[ ... ]
> @@ -13,22 +13,17 @@
>  #include "dw-hdma-v0-regs.h"
>  #include "dw-edma-core.h"
>  
> -#define REGS_ADDR(dw, name)						       \
> -	({								       \
> -		struct dw_hdma_v0_regs __iomem *__regs = (dw)->chip->reg_base; \
> -									       \
> -		(void __iomem *)&__regs->name;				       \
> -	})
> -
>  #define REGS_CH_ADDR(dw, name, _dir, _ch)				       \
>  	({								       \
> -		struct dw_hdma_v0_ch_regs __iomem *__ch_regs;		       \
> +		struct dw_hdma_v0_ch_regs __iomem *__ch_regs;                  \
> +		off_t __off = (dw)->chip->ch_sep_sz;			       \
>  									       \
> -		if (_dir == EDMA_DIR_READ)				       \
> -			__ch_regs = REGS_ADDR(dw, ch[_ch].rd);		       \
> +		if ((_dir) == EDMA_DIR_READ)				       \
> +			__off *= (2 * (_ch) + 1);			       \
>  		else							       \
> -			__ch_regs = REGS_ADDR(dw, ch[_ch].wr);		       \
> +			__off *= (2 * (_ch));				       \
>  									       \
> +		__ch_regs = ((dw)->chip->reg_base + __off);		       \

[Severity: High]
Could this also lead to out-of-bounds MMIO accesses during debugfs reads?

If the untrusted channel separation size is used to compute the offset, and
userspace triggers a debugfs read, it seems this could be exploited if not
validated against the BAR size.

>  		(void __iomem *)&__ch_regs->name;			       \
>  	})
>  
> diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h
> index 1fafd5b0e3153..a6c5728f03d38 100644
> --- a/include/linux/dma/edma.h
> +++ b/include/linux/dma/edma.h
> @@ -104,6 +104,7 @@ struct dw_edma_chip {
>  
>  	struct dw_edma		*dw;
>  	bool			cfg_non_ll;
> +	u32			ch_sep_sz;

[Severity: High]
Does this new field need a fallback default for non-PCIe drivers?

Other core drivers that instantiate struct dw_edma_chip for HDMA might not
explicitly set this, leaving it uninitialized.

>  };
>  
>  /* Export to the platform drivers */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722111640.1146212-1-devverma@amd.com?part=3