Re: [PATCH 04/11] PCI: endpoint: pci-epf-vntb: Implement MW group translation callbacks

On Mon, Aug 03, 2026 at 06:25:28PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] DMA Use-After-Free due to `vntb_epf_mw_clear_trans_group()` and `vntb_epf_mw_clear_trans()` returning success (0) without actually disabling the inbound hardware translations.

To be clear, vntb_epf_mw_clear_trans_group() is intentionally left as a no-op,
matching the existing the existing vntb_epf_mw_clear_trans() behavior. This
series does not try to fix that pre-existing limitation.
See the "About existing inbound MW clear behavior" note in the cover letter.

Koichiro

> - [High] The `vntb_epf_mw_set_trans()` function incorrectly hardcodes `func_no` and `vfunc_no` to 0 when invoking `pci_epc_set_bar()`, whereas other EPF APIs (including the newly added group API) correctly use the endpoint's configured function numbers.
> - [High] The `vntb_pci_driver` structure is missing a `.remove` callback, causing NTB device leaks and use-after-free conditions when the endpoint function is unbound.
> --
> 
> --- Patch [4]: [PATCH 04/11] PCI: endpoint: pci-epf-vntb: Implement MW group translation callbacks ---
> commit 419429d9534b2c856c11cb4a4a4107e84a87fe8e
> Author: Koichiro Den <[email protected]>
> 
> PCI: endpoint: pci-epf-vntb: Implement MW group translation callbacks
> 
> All RC-to-EP MWs in a packed group share one inbound BAR. Setting each
> MW separately would keep replacing that BAR and could expose a partial
> group. Add group callbacks that program the shared BAR once for the
> complete backing range.
> 
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > index f22459e5a8c69..0c72b907c2896 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > @@ -1622,6 +1622,31 @@ static int vntb_epf_mw_clear_trans(struct ntb_dev *ntb, int pidx, int idx)
> >  	return 0;
> >  }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but while looking at this context,
> I noticed that the existing vntb_epf_mw_set_trans() hardcodes the function
> numbers to 0:
> 
> drivers/pci/endpoint/functions/pci-epf-vntb.c:vntb_epf_mw_set_trans() {
>     ...
> 	ret = pci_epc_set_bar(ntb->epf->epc, 0, 0, epf_bar);
>     ...
> }
> 
> Does this incorrectly configure the BAR for function 0 when the virtual NTB
> device is bound to a non-zero endpoint function or virtual function?
> 
> If the device is configured via configfs with a non-zero function, could this
> lead to silent failures in memory window translation or corrupt the ATU
> configuration of function 0?
> 
> > +
> > +static int
> > +vntb_epf_mw_set_trans_group(struct ntb_dev *ndev, int pidx, int widx,
> > +			    dma_addr_t addr, resource_size_t size)
> > +{
> [ ... ]
> > +	return pci_epc_set_bar(ntb->epf->epc, ntb->epf->func_no,
> > +			       ntb->epf->vfunc_no, epf_bar);
> > +}
> > +
> > +static int vntb_epf_mw_clear_trans_group(struct ntb_dev *ndev, int pidx,
> > +					 int widx)
> > +{
> > +	return 0;
> > +}
> 
> [Severity: Critical]
> This is a pre-existing issue perpetuated here by the new group API, but does
> returning 0 without actually disabling the inbound hardware translations create a
> DMA use-after-free window?
> 
> When an NTB client unmaps a memory window group and frees the backing memory,
> the hardware BAR mapping will remain active.
> 
> If the local host frees this memory, won't the endpoint hardware continue
> translating incoming PCIe accesses to the freed physical address, allowing a
> remote peer to access arbitrary new allocations on the endpoint?
> 
> [Severity: High]
> This is another pre-existing issue, but I noticed the vntb_pci_driver structure
> is missing a .remove callback:
> 
> drivers/pci/endpoint/functions/pci-epf-vntb.c:
> static struct pci_driver vntb_pci_driver = {
> 	.name           = "pci-vntb",
> 	.id_table       = pci_vntb_table,
> 	.probe          = pci_vntb_probe,
> };
> 
> Without a .remove callback, ntb_unregister_device() is never called when the
> EPF device is unbound via configfs.
> 
> Does this cause NTB device leaks and driver state use-after-free?
> 
> If the endpoint function is unbound, won't the NTB clients remain active and
> bound to a device whose backing EPF structures, DMA masks, and hardware
> mappings have been destroyed?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4