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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ntb: missing offset subtraction in mw_set_trans allows window to overlap DMA submap
- [High] ntb: incomplete validation of mw_offset allows OOB physical memory mapping
--

commit 51fd2384ff902b8fa804aedcaaf547e5f8303043
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. Decode the extension and register the advertised channels before
the NTB device, so clients of that NTB device can use the DMA
functionality in the same PCI function.

> diff --git a/drivers/ntb/hw/epf/ntb_hw_epf.c b/drivers/ntb/hw/epf/ntb_hw_epf.c
> index c47607d4f8a7f..7faa40d1ae00d 100644
> --- a/drivers/ntb/hw/epf/ntb_hw_epf.c
> +++ b/drivers/ntb/hw/epf/ntb_hw_epf.c
[ ... ]
> @@ -212,7 +483,7 @@ static int ntb_epf_mw_get_align(struct ntb_dev *ntb, int pidx, int idx,
>  		*size_align = 1;
>  
>  	if (size_max)
> -		*size_max = pci_resource_len(ndev->ntb.pdev, bar);
> +		*size_max = ntb_epf_mw_size(ndev, bar);

[Severity: High]
Does this code accurately report the maximum available size to clients?

Because the physical memory window starts at a hardware offset, is it
necessary to deduct the offset here to ensure clients are not misled
about the actual available space?

>  
>  	return 0;
>  }
[ ... ]
> @@ -486,7 +770,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, bar);
>  
>  	if (size > mw_size) {
>  		dev_err(dev, "Size:%pa is greater than the MW size %pa\n",

[Severity: High]
Does this validation successfully prevent the memory window from
overlapping the DMA submap?

Because the memory window starts at the hardware offset (read via
ntb_epf_mw_offset()), checking if size > mw_size without subtracting
the offset might allow a client to request a size up to mw_size. This
would result in the memory window ending at size + offset, which
exceeds the safe mw_size boundary.

Can this allow the host's memory window transfers to silently overwrite
the endpoint's new DMA control registers?

>  			&size, &mw_size);
>  		return -EINVAL;
>  	}
[ ... ]
> @@ -520,21 +804,20 @@ static int ntb_epf_peer_mw_get_addr(struct ntb_dev *ntb, int idx,
>  				    phys_addr_t *base, resource_size_t *size)
>  {
>  	struct ntb_epf_dev *ndev = ntb_ndev(ntb);
> -	u32 offset = 0;
> +	resource_size_t offset;
>  	int bar;
>  
> -	if (idx == 0)
> -		offset = readl(ndev->ctrl_reg + NTB_EPF_MW1_OFFSET);
> -
>  	bar = ntb_epf_mw_to_bar(ndev, idx);
>  	if (bar < 0)
>  		return bar;
>  
> +	offset = ntb_epf_mw_offset(ndev, idx);
> +
>  	if (base)
>  		*base = pci_resource_start(ndev->ntb.pdev, bar) + offset;
>  
>  	if (size)
> -		*size = pci_resource_len(ndev->ntb.pdev, bar) - offset;
> +		*size = ntb_epf_mw_size(ndev, bar) - offset;

[Severity: High]
Does this subtraction risk an integer underflow if a malicious endpoint
provides an offset larger than the window size?

If offset > mw_size, this subtraction wraps. On 32-bit systems, a
massive offset could result in a small valid size while shifting the
base physical address backwards. This might allow an untrusted endpoint
to trick the host into mapping arbitrary physical memory before the BAR
via memremap().

Is the offset thoroughly validated for all BARs in ntb_epf_init_dev()
before being used here?

>  
>  	return 0;
>  }
[ ... ]
> @@ -630,12 +913,30 @@ static int ntb_epf_init_dev(struct ntb_epf_dev *ndev)
>  {
>  	struct device *dev = ndev->dev;
>  	int ret;
> +	int i;
>  
>  	ndev->mw_count = readl(ndev->ctrl_reg + NTB_EPF_MW_COUNT);
>  	if (ndev->mw_count > NTB_EPF_MAX_MW_COUNT) {
>  		dev_err(dev, "Unsupported MW count: %u\n", ndev->mw_count);
>  		return -EINVAL;
>  	}
> +	ret = ntb_epf_dma_parse(ndev);
> +	if (ret) {
> +		dev_err(dev, "Invalid endpoint DMA layout\n");
> +		return ret;
> +	}
> +	if (ndev->dma.submap.size) {
> +		for (i = 0; i < ndev->mw_count; i++) {
> +			int bar = ntb_epf_mw_to_bar(ndev, i);
> +
> +			if (bar == ndev->dma.submap.bar &&
> +			    ntb_epf_mw_size(ndev, bar) <=
> +			    ntb_epf_mw_offset(ndev, i)) {
> +				dev_err(dev, "Invalid DMA/MW boundary\n");
> +				return -EINVAL;
> +			}
> +		}
> +	}

[Severity: High]
Does this validation loop completely protect against an overly large
offset?

It currently only checks if the memory window offset exceeds the window
size when the DMA submap is present, and only for the specific DMA
submap BAR. If the DMA submap is absent, or for other BARs, the
untrusted offset seems to remain unvalidated.

Could this incomplete validation lead to the integer underflow and
out-of-bounds physical memory mapping in ntb_epf_peer_mw_get_addr()?

>  
>  	/* One Link interrupt and rest doorbell interrupt */
>  	ret = ntb_epf_init_isr(ndev, NTB_EPF_MIN_DB_COUNT + 1,

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