[PATCH] ntb: epf: fix PCI resource cleanup on init error path

[PATCH] ntb: epf: fix PCI resource cleanup on init error path

[Shashwat Agrawal]

After pci_request_regions() succeeds, failures from setting the DMA
mask or mapping BARs jumped to a path that only called
pci_disable_device(). PCI regions were left claimed, and any BARs
already mapped with pci_iomap() were not unmapped.

Release regions on those failures and unwind iomaps in reverse order.
When peer scratchpads share the config BAR, do not pci_iounmap() the
offset pointer, consistent with ntb_epf_deinit_pci().

Signed-off-by: Shashwat Agrawal <shashwatagrawal473@gmail.com>
---
 drivers/ntb/hw/epf/ntb_hw_epf.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/ntb/hw/epf/ntb_hw_epf.c b/drivers/ntb/hw/epf/ntb_hw_epf.c
index c47607d4f8a7..b42fb5f9fa75 100644
--- a/drivers/ntb/hw/epf/ntb_hw_epf.c
+++ b/drivers/ntb/hw/epf/ntb_hw_epf.c
@@ -683,7 +683,7 @@ static int ntb_epf_init_pci(struct ntb_epf_dev *ndev,
 		ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
 		if (ret) {
 			dev_err(dev, "Cannot set DMA mask\n");
-			goto err_pci_regions;
+			goto err_dma_mask;
 		}
 		dev_warn(&pdev->dev, "Cannot DMA highmem\n");
 	}
@@ -691,7 +691,7 @@ static int ntb_epf_init_pci(struct ntb_epf_dev *ndev,
 	ndev->ctrl_reg = pci_iomap(pdev, ndev->barno_map[BAR_CONFIG], 0);
 	if (!ndev->ctrl_reg) {
 		ret = -EIO;
-		goto err_pci_regions;
+		goto err_dma_mask;
 	}
 
 	if (ndev->barno_map[BAR_PEER_SPAD] != ndev->barno_map[BAR_CONFIG]) {
@@ -699,7 +699,7 @@ static int ntb_epf_init_pci(struct ntb_epf_dev *ndev,
 						ndev->barno_map[BAR_PEER_SPAD], 0);
 		if (!ndev->peer_spad_reg) {
 			ret = -EIO;
-			goto err_pci_regions;
+			goto err_ctrl_map;
 		}
 	} else {
 		spad_sz = 4 * readl(ndev->ctrl_reg + NTB_EPF_SPAD_COUNT);
@@ -710,14 +710,20 @@ static int ntb_epf_init_pci(struct ntb_epf_dev *ndev,
 	ndev->db_reg = pci_iomap(pdev, ndev->barno_map[BAR_DB], 0);
 	if (!ndev->db_reg) {
 		ret = -EIO;
-		goto err_pci_regions;
+		goto err_peer_map;
 	}
 
 	return 0;
 
+err_peer_map:
+	if (ndev->barno_map[BAR_PEER_SPAD] != ndev->barno_map[BAR_CONFIG])
+		pci_iounmap(pdev, ndev->peer_spad_reg);
+err_ctrl_map:
+	pci_iounmap(pdev, ndev->ctrl_reg);
+err_dma_mask:
+	pci_release_regions(pdev);
 err_pci_regions:
 	pci_disable_device(pdev);
-
 err_pci_enable:
 	pci_set_drvdata(pdev, NULL);
 
-- 
2.54.0

Re: [PATCH] ntb: epf: fix PCI resource cleanup on init error path

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver reads scratchpad counts and offsets directly from untrusted device MMIO memory without validating them against the mapped BAR size, allowing a malicious PCI device to cause out-of-bounds memory accesses.
- [High] Off-by-one error in `ntb_epf_mw_to_bar()` allows out-of-bounds array access.
--

commit b593a66e63f557a346bb27a3a7845733ec998c5b
Author: Shashwat Agrawal <shashwatagrawal473@gmail.com>

ntb: epf: fix PCI resource cleanup on init error path
    
After pci_request_regions() succeeds, failures from setting the DMA
mask or mapping BARs jumped to a path that only called
pci_disable_device(). This left PCI regions claimed and BARs already
mapped with pci_iomap() were not unmapped. This patch releases regions
on those failures and unwinds iomaps in reverse order.

> diff --git a/drivers/ntb/hw/epf/ntb_hw_epf.c b/drivers/ntb/hw/epf/ntb_hw_epf.c
> index c47607d4f8a7f..b42fb5f9fa75d 100644
> --- a/drivers/ntb/hw/epf/ntb_hw_epf.c
> +++ b/drivers/ntb/hw/epf/ntb_hw_epf.c
[ ... ]
> @@ -699,7 +699,7 @@ static int ntb_epf_init_pci(struct ntb_epf_dev *ndev,
>  						ndev->barno_map[BAR_PEER_SPAD], 0);
>  		if (!ndev->peer_spad_reg) {
>  			ret = -EIO;
> -			goto err_pci_regions;
> +			goto err_ctrl_map;
>  		}
>  	} else {
>  		spad_sz = 4 * readl(ndev->ctrl_reg + NTB_EPF_SPAD_COUNT);
>  		spad_off = readl(ndev->ctrl_reg + NTB_EPF_SPAD_OFFSET);
>  		ndev->peer_spad_reg = ndev->ctrl_reg + spad_off  + spad_sz;

[Severity: High]
This is a pre-existing issue, but does this code properly validate the
scratchpad count and offset before using them?

Looking at ntb_epf_init_pci(), the untrusted NTB_EPF_SPAD_COUNT and
NTB_EPF_SPAD_OFFSET values are read directly from device MMIO and added to
ndev->ctrl_reg without being validated against the mapped BAR size.

A maliciously large count or offset from the endpoint device could cause
spad_sz and peer_spad_reg to far exceed the PCI BAR size.

Later, ntb_epf_init_dev() trusts NTB_EPF_SPAD_COUNT as an upper bound, meaning
clients iterating over this count with ntb_epf_peer_spad_read/write() will
dereference an out-of-bounds pointer.

This allows a maliciously controlled remote endpoint to cause unmapped or
arbitrary physical memory access, leading to a kernel panic or memory
corruption.

>  	}
>  
>  	ndev->db_reg = pci_iomap(pdev, ndev->barno_map[BAR_DB], 0);

[ ... ]

[Severity: High]
This is a pre-existing issue, but does ntb_epf_mw_to_bar() have an off-by-one
error in its bounds checking?

Looking at ntb_epf_mw_to_bar():

	if (idx < 0 || idx > ndev->mw_count) {
		dev_err(dev, "Unsupported Memory Window index %d\n", idx);
		return -EINVAL;
	}

	return ndev->barno_map[BAR_MW1 + idx];

The bounds check incorrectly permits idx == ndev->mw_count.

If an NTB client driver requests an operation using this maximum index,
BAR_MW1 + idx will evaluate to a value exceeding the size of the
ndev->barno_map array.

Returning a garbage BAR number here and passing it to pci_resource_len() or
pci_resource_start() causes an out-of-bounds array access, potentially
returning arbitrary attacker-controlled offsets that lead to a kernel crash.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702070628.19840-1-shashwatagrawal473@gmail.com?part=1