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

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

New issues:
- [High] Out-of-Bounds MMIO Access via Untrusted `ch_space_sz`

Pre-existing issues:
- [High] Resource Leak (Missing IRQ Vector Cleanup)
- [High] Logic Error / Argument Swap in GET_CH_32
--

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

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

This commit adds configurable DMA channel register space separation 
ranging from 256B to 32KB. It updates functions to calculate base addresses 
based on the selected channel separation value available in the PCIe VSEC 
configuration space.

> diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
> index 5d7507a640565..81401620988c9 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;
>  
>  	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 this function leak IRQ vectors on error
paths?

If initialization fails after pci_alloc_irq_vectors() succeeds earlier in the 
function, it returns directly without cleaning up the IRQs:

dw_edma_pcie_probe()
{
    ...
    chip->reg_base = pcim_iomap_table(pdev)[vsec_data->rg.bar];
    if (!chip->reg_base)
        return -ENOMEM;
    ...
    err = dw_edma_probe(chip);
    if (err) {
        pci_err(pdev, "eDMA probe failed\n");
        return err;
    }
    ...
}

Should these error paths call pci_free_irq_vectors(pdev) to ensure we don't 
exhaust system IRQ capacity?

> 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
[ ... ]
> @@ -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_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]
Is it possible for this pointer arithmetic to access memory out of bounds?

The ch_space_sz value is derived dynamically from the untrusted PCIe VSEC 
configuration space (scaling up to 32KB per channel). If a malicious or buggy 
hardware endpoint presents a small BAR (e.g., 4KB) but provides a large VSEC 
channel separation value, multiplying ch_base by dw->chip->ch_space_sz could 
yield an offset well beyond the memory mapped in chip->reg_base.

Should ch_space_sz be validated against the mapped BAR size using 
pci_resource_len() before using it to index into the MMIO space?

[Severity: High]
This is a pre-existing issue, but does dw_hdma_v0_core_ch_status() pass 
arguments to the GET_CH_32 macro in the wrong order?

In dw_hdma_v0_core_ch_status(), the code does:

    tmp = FIELD_GET(HDMA_V0_CH_STATUS_MASK,
                    GET_CH_32(dw, chan->id, chan->dir, ch_stat));

The GET_CH_32 macro expects arguments in the order (dw, dir, ch, name), 
but chan->id and chan->dir appear to be swapped. Can this cause the driver 
to calculate an incorrect register base offset and read from the wrong 
hardware channel 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..3fa16e045a584 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-debugfs.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-debugfs.c
[ ... ]
>  #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_space_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);		       \
>  		(void __iomem *)&__ch_regs->name;			       \
>  	})

[Severity: High]
Can this result in an out-of-bounds MMIO read when userspace accesses debugfs 
attributes?

Similar to the calculation in __dw_ch_regs(), __off scales by the unvalidated 
ch_space_sz value. If the value isn't checked against the BAR size, could a 
userspace read/write to debugfs trigger an out-of-bounds access beyond the 
mapped memory space?

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