Re: [PATCH v3 6/7] NTB: ntb_hw_epf: Discover vNTB-embedded DMA

[sashiko-bot@kernel.org]

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

New issues:
- [High] Probe fails entirely if the endpoint advertises an unsupported DMA type or if dw-edma host support is missing.
- [High] Strictly demanding `dma_irqs` extra MSI/MSI-X vectors breaks probe on IRQ-constrained systems.

Pre-existing issues:
- [Critical] Dynamic reading of the SPAD offset without validation allows a malicious endpoint to cause arbitrary host MMIO read/write.
- [High] Pre-existing race condition in `ntb_epf_mw_set_trans` where multiple concurrent calls can interleave writes to shared memory window configuration registers.
--

commit 9b8b5f4c03fc934132c68dec22efe8a4ca1332ef
Author: Koichiro Den <den@valinux.co.jp>

NTB: ntb_hw_epf: Discover vNTB-embedded DMA

A vNTB PCI function can now expose endpoint-local DMA channels to its
host. This patch decodes the extension and registers the advertised
channels before the NTB device.

> diff --git a/drivers/ntb/hw/epf/ntb_hw_epf.c b/drivers/ntb/hw/epf/ntb_hw_epf.c
> index c47607d4f8a7f..16cde97d4bde2 100644
> --- a/drivers/ntb/hw/epf/ntb_hw_epf.c
> +++ b/drivers/ntb/hw/epf/ntb_hw_epf.c

[ ... ]

> @@ -373,15 +681,19 @@ static int ntb_epf_init_isr(struct ntb_epf_dev *ndev, int msi_min, int msi_max)
>  {
>  	struct pci_dev *pdev = ndev->ntb.pdev;
>  	struct device *dev = ndev->dev;
> +	unsigned int dma_irqs = ndev->dma.nr_irqs;
> +	unsigned int ntb_irqs;
>  	u32 argument = MSIX_ENABLE;
>  	int irq;
>  	int ret;
>  	int i;
>  
> -	irq = pci_alloc_irq_vectors(pdev, msi_min, msi_max, PCI_IRQ_MSIX);
> +	irq = pci_alloc_irq_vectors(pdev, msi_min + dma_irqs,
> +				    msi_max + dma_irqs, PCI_IRQ_MSIX);

[Severity: High]
Will strictly demanding the extra dma_irqs break probe on IRQ-constrained
host systems?

If the host platform can only supply fewer vectors than the minimum
requested here (which can inflate up to 10 if the endpoint advertises 8
DW eDMA channels), pci_alloc_irq_vectors() will return a negative error
code.

Should the driver request only the minimum required for basic NTB
operation, and selectively allocate or disable DMA channels based on the
number of vectors actually granted?

[ ... ]

> @@ -486,7 +799,7 @@ static int ntb_epf_mw_set_trans(struct ntb_dev *ntb, int pidx, int idx,
>  	if (bar < 0)
>  		return bar;
>  
> -	mw_size = pci_resource_len(ntb->pdev, bar);
> +	mw_size = ntb_epf_mw_size(ndev, idx, bar);
>  
>  	if (size > mw_size) {
>  		dev_err(dev, "Size:%pa is greater than the MW size %pa\n",

[Severity: High]
This is a pre-existing issue, but can concurrent calls to
ntb_epf_mw_set_trans() corrupt the memory window configurations?

The function writes to a shared set of registers across memory windows
right after this block:

writel(lower_32_bits(addr), ndev->ctrl_reg + NTB_EPF_LOWER_ADDR);
writel(upper_32_bits(addr), ndev->ctrl_reg + NTB_EPF_UPPER_ADDR);
writel(lower_32_bits(size), ndev->ctrl_reg + NTB_EPF_LOWER_SIZE);
writel(upper_32_bits(size), ndev->ctrl_reg + NTB_EPF_UPPER_SIZE);

It then calls ntb_epf_send_command(), which is where ndev->cmd_lock is
finally acquired. If multiple threads configure different memory windows
concurrently, can their writes interleave and send a corrupted mixture of
addresses and sizes?

[ ... ]

> @@ -785,6 +1114,12 @@ static int ntb_epf_pci_probe(struct pci_dev *pdev,
>  		goto err_init_dev;
>  	}
>  
> +	ret = ntb_epf_dma_init(ndev);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize endpoint DMA\n");
> +		goto err_dma_init;
> +	}

[Severity: High]
If the endpoint advertises an unsupported DMA type, or if the host kernel
is compiled without CONFIG_DW_EDMA, ntb_epf_dma_init() will fail. Does
this mean the entire NTB device initialization will completely abort here?

Since the DMA extension is optional, would it be better to log a warning
and fall back to the existing non-DMA path instead of completely rejecting
the device?


While reviewing this file, I also noticed a pre-existing issue in
ntb_epf_spad_read() and ntb_epf_spad_write() which are not modified by
this patch:

[Severity: Critical]
This isn't a bug introduced by this patch, but is it safe to use the SPAD
offset directly from hardware for an IOMEM read or write without
validation?

In ntb_epf_spad_read(), the offset is read dynamically:

offset = readl(ndev->ctrl_reg + NTB_EPF_SPAD_OFFSET);
offset += (idx << 2);
return readl(ndev->ctrl_reg + offset);

Since the PCIe endpoint controls its configuration space, a malicious or
compromised endpoint could modify NTB_EPF_SPAD_OFFSET to an arbitrarily
large 32-bit value. Does this allow the endpoint to trigger an
unvalidated out-of-bounds MMIO read or write on the host?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831182657.329614-1-den@valinux.co.jp?part=6