Re: [PATCH 1/3] PCI: endpoint: pci-epf-vntb: Pass PF/VF number when BAR programming

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

Pre-existing issues:
- [High] An off-by-one loop initialization error in `epf_ntb_init_epc_bar()=
` leaves memory window BARs beyond the first one unassigned (`NO_BAR` or `-=
1`). This results in negative array indexing and out-of-bounds memory acces=
ses when `epf_ntb_mw_bar_init()` and `vntb_epf_mw_set_trans()` index into `=
ntb->epf->bar[barno]`.
- [High] Missing NULL checks for `epc_features` returned by `pci_epc_get_fe=
atures()`, leading to a kernel panic if the underlying EPC driver does not =
implement the `.get_features` callback.
- [High] The virtual PCI driver `vntb_pci_driver` lacks a `.remove` callbac=
k, resulting in resource leaks of the virtual PCI bus and Use-After-Free of=
 the NTB device structures upon module unload or EPF unbind.
--

--- Patch [1]: [PATCH 1/3] PCI: endpoint: pci-epf-vntb: Pass PF/VF number w=
hen BAR programming ---
commit fc9ce298c179bf87e88da3f2964f2fbbd870806a
Author: Koichiro Den <[email protected]>
PCI: endpoint: pci-epf-vntb: Pass PF/VF number when BAR programming

This commit updates vntb_epf_mw_set_trans() to pass the endpoint function's
own func_no and vfunc_no instead of hardcoded zeros when programming the
memory-window BAR. This ensures the BAR is correctly configured when the
virtual NTB endpoint function is bound to a function other than PF0.

> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/=
endpoint/functions/pci-epf-vntb.c
> index c3caec927d748..fba65abfb6b21 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> @@ -1427,7 +1427,8 @@ static int vntb_epf_mw_set_trans(struct ntb_dev *nd=
ev, int pidx, int idx,
>  	epf_bar->barno =3D barno;
>  	epf_bar->size =3D size;

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for barno to
be -1 here, leading to an out-of-bounds write?

Looking at epf_ntb_init_epc_bar(), the loop iterating over optional memory
window BARs incorrectly starts from BAR_MW1 instead of BAR_MW2:

drivers/pci/endpoint/functions/pci-epf-vntb.c:epf_ntb_init_epc_bar() {
	...
	for (bar =3D BAR_MW1, i =3D 1; i < num_mws; bar++, i++) {
	...
}

If a user configures num_mws > 1, this leaves BAR_MW2 unassigned (-1). Late=
r,
when vntb_epf_mw_set_trans() is called for idx =3D 1, barno is evaluated as=
 -1.
Will epf_bar =3D &ntb->epf->bar[barno] then access out of bounds and corrupt
memory?

> -	ret =3D pci_epc_set_bar(ntb->epf->epc, 0, 0, epf_bar);
> +	ret =3D pci_epc_set_bar(ntb->epf->epc, ntb->epf->func_no,
> +			      ntb->epf->vfunc_no, epf_bar);
>  	if (ret) {
>  		dev_err(dev, "failure set mw trans\n");
>  		return ret;

Also, while reviewing this file, I noticed a few other pre-existing issues:

[Severity: High]
This is a pre-existing issue, but how is the case where epc_features returns
NULL handled?

drivers/pci/endpoint/functions/pci-epf-vntb.c:epf_ntb_configure_interrupt()=
 {
	...
	epc_features =3D pci_epc_get_features(ntb->epf->epc, ntb->epf->func_no, nt=
b->epf->vfunc_no);

	if (!(epc_features->msix_capable || epc_features->msi_capable)) {
	...
}

If the underlying EPC driver lacks the .get_features callback,
pci_epc_get_features() returns NULL, which would cause a NULL pointer
dereference here. A similar dereference happens in
epf_ntb_config_spad_bar_alloc().


[Severity: High]
This is a pre-existing issue, but does vntb_pci_driver intentionally omit a
.remove callback?

drivers/pci/endpoint/functions/pci-epf-vntb.c:
	static struct pci_driver vntb_pci_driver =3D {
		.name           =3D "pci-vntb",
		.id_table       =3D pci_vntb_table,
		.probe          =3D pci_vntb_probe,
	};

When epf_ntb_unbind() unregisters this driver:

drivers/pci/endpoint/functions/pci-epf-vntb.c:epf_ntb_unbind() {
	...
	pci_unregister_driver(&vntb_pci_driver);
}

Since there is no .remove callback, ntb_unregister_device() is never called=
 to
clean up the virtual PCI bus and its associated devices. Could this lead to=
 a
resource leak and dangling sysdata pointers upon module unload or EPF unbin=
d?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260728172306.2751=
[email protected]?part=3D1