Re: [PATCH v3 5/7] PCI: endpoint: pci-epf-vntb: Allow DMA and MW to share a BAR

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Deactivating a shared DMA BAR unconditionally clears all submaps, breaking the active Memory Window (MW) mapping and causing severe memory corruption.
- [High] The `ntb->dma` structure is freed in `epf_ntb_unbind()` before the driver is unregistered, allowing concurrent NTB subsystem operations to use a freed pointer and its lock.

Pre-existing issues:
- [High] The virtual PCI driver `vntb_pci_driver` lacks a `.remove` callback, causing the NTB device and virtual PCI bus devices to leak and potentially use freed memory.
--

commit 4680cfd6b651b64bf9225e9233e1d66310c8b0da
Author: Koichiro Den <den@valinux.co.jp>

PCI: endpoint: pci-epf-vntb: Allow DMA and MW to share a BAR

This patch modifies the PCI endpoint vNTB function to allow a DMA BAR to
select an MW BAR and place DMA ranges after it. This enables configurations
that have no spare BAR for DMA resources to share one.

> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index 1842493c33b02..aa659ec62d931 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
[ ... ]
> @@ -621,17 +648,25 @@ static void epf_ntb_dma_release(struct epf_ntb *ntb, bool quiesce)
>  	ntb->dma = NULL;
>  }
>  
> -static int epf_ntb_dma_set_bar(struct epf_ntb *ntb, bool active)
> +static int epf_ntb_dma_set_bar_locked(struct epf_ntb *ntb, bool active,
> +				      const dma_addr_t *mw_addr)
>  {
>  	struct pci_epf_bar_submap *old_submap;
>  	struct epf_ntb_dma *dma = ntb->dma;
>  	struct pci_epf_bar *bar;
>  	unsigned int old_num_submap;
> +	dma_addr_t old_mw_addr;
>  	int restore, ret;
>  
> +	lockdep_assert_held(&dma->lock);
> +
>  	bar = &ntb->epf->bar[ntb->epf_ntb_bar[BAR_DMA]];
>  	old_submap = bar->submap;
>  	old_num_submap = bar->num_submap;
> +	if (mw_addr) {
> +		old_mw_addr = dma->submap[0].phys_addr;
> +		dma->submap[0].phys_addr = *mw_addr;
> +	}
> -	bar->submap = active ? dma->submap : NULL;
> -	bar->num_submap = active ? dma->num_submap : 0;
> +	bar->submap = active ? dma->submap : NULL;
> +	bar->num_submap = active ? dma->num_submap : 0;

[Severity: Critical]
If the host sends a COMMAND_TEARDOWN_DMA while a Memory Window is still
active, epf_ntb_dma_set_active() will call this function with active set to
false.

Does this unconditionally clear all submaps on the BAR, including submap[0]
which covers the MW prefix? 

Since pci_epf_assign_bar_space() used address 0 as the fallback base physical
address, clearing the submap would cause pci_epc_set_bar() to map the entire
BAR to CPU physical address 0 on the endpoint. Could this corrupt memory when
the host subsequently accesses the MW?

>  
>  	ret = pci_epc_set_bar(ntb->epf->epc, ntb->epf->func_no,
>  			      ntb->epf->vfunc_no, bar);
[ ... ]
> @@ -1974,6 +2019,16 @@ static int vntb_epf_mw_set_trans(struct ntb_dev *ndev, int pidx, int idx,
>  	dev = &ntb->ntb.dev;
>  	barno = ntb->epf_ntb_bar[BAR_MW1 + idx];
>  	epf_bar = &ntb->epf->bar[barno];
> +	if (epf_ntb_dma_shares_bar(ntb, barno)) {
> +		/* DMA submaps follow this MW, so its extent cannot be changed. */
> +		if (size != ntb->mws_size[idx])
> +			return -EINVAL;
> +
> +		guard(mutex)(&ntb->dma->lock);

[Severity: High]
Can this lock be accessed after ntb->dma has been freed?

In epf_ntb_unbind(), epf_ntb_dma_release() is called, which frees ntb->dma
via kfree(). However, pci_unregister_driver(&vntb_pci_driver) is called
after this.

If a host MW configuration command runs concurrently with unbind, could
vntb_epf_mw_set_trans() check epf_ntb_dma_shares_bar() and then attempt to
acquire this mutex on the freed ntb->dma pointer?

> +
> +		return epf_ntb_dma_set_bar_locked(ntb, true, &addr);
> +	}
> +
>  	epf_bar->phys_addr = addr;
>  	epf_bar->barno = barno;
>  	epf_bar->size = size;

[Severity: High]
This is a pre-existing issue, but I noticed that vntb_pci_driver lacks a
remove callback:

static struct pci_driver vntb_pci_driver = {
	.name           = "pci-vntb",
	.id_table       = pci_vntb_table,
	.probe          = pci_vntb_probe,
};

When epf_ntb_unbind() calls pci_unregister_driver(), it unbinds the driver
but ntb_unregister_device() is never called because there is no cleanup
function. 

Since the struct epf_ntb instance is allocated via devm_kzalloc() tied to
the EPF device, it gets freed when the EPF device is removed. Does this
cause the NTB device and virtual PCI bus devices to leak, leading to a
use-after-free if the NTB subsystem makes subsequent calls?

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