Re: [PATCH 3/3] PCI: endpoint: pci-epf-vntb: Honor MSI-X selection

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The `epf_ntb_configure_mw` and `epf_ntb_teardown_mw` functions do not validate the `mw` index provided by the remote host over PCIe, allowing arbitrary physical memory mapping.
- [High] pci-epf-vntb ignores the negotiated db_count from the host during COMMAND_CONFIGURE_DOORBELL, potentially leading to IOMMU faults.
- [High] Dangling pointer / Use-After-Free during device unbind because BAR memory is freed before driver unregistration.
- [High] The configfs handler for `spad_count` lacks bounds checking, leading to an integer overflow during `spad_size` computation and subsequent out-of-bounds memory writes on the endpoint.
- [Medium] The EP driver configures the MSI capability with a hardcoded limit of 16 vectors, but `db_count` can be up to 32, leading to failed interrupt deliveries if `db_count` > 16 and MSI is used.
--

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

PCI: endpoint: pci-epf-vntb: Honor MSI-X selection

This commit updates the pci-epf-vntb driver to configure and respect MSI-X
when selected by the host, falling back to MSI otherwise. It ensures that
peer doorbells raise the correct interrupt type, avoiding writes to invalid
addresses that can trigger IOMMU faults.

Fixes: e35f56bb0330 ("PCI: endpoint: Support NTB transfer between RC and EP")

> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index c3caec927d748..d9622a5d47102 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
[ ... ]
> @@ -303,6 +305,7 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
>  
>  	switch (command) {
>  	case COMMAND_CONFIGURE_DOORBELL:
> +		WRITE_ONCE(ntb->peer_msix, argument & MSIX_ENABLE);

[Severity: High]
This is a pre-existing issue, but does this handler ignore the negotiated
db_count from the host?

If the host negotiates a smaller db_count and passes it in the lower 16 bits
of the argument, the endpoint ignores it and retains its original db_count.

Could the endpoint's NTB client then ring a doorbell index greater than the
negotiated count, triggering pci_epc_raise_irq() on an unprogrammed vector
and causing an IOMMU fault?

>  		ctrl->command_status = COMMAND_STATUS_OK;
>  		break;
>  	case COMMAND_TEARDOWN_DOORBELL:
[ ... ]
> @@ -502,6 +528,7 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
>  static int epf_ntb_configure_interrupt(struct epf_ntb *ntb)
>  {
[ ... ]
>  	if (epc_features->msi_capable) {
> -		ret = pci_epc_set_msi(ntb->epf->epc,
> -				      ntb->epf->func_no,
> -				      ntb->epf->vfunc_no,
> -				      16);
> +		ret = pci_epc_set_msi(epf->epc, epf->func_no, epf->vfunc_no, 16);

[Severity: Medium]
This isn't a bug introduced by this patch, but is there a conflict between
this hardcoded limit of 16 vectors and the db_count limit?

If the user configures db_count to a value between 17 and 32 via configfs,
and the endpoint relies on MSI interrupts because MSI-X is disabled, the EP
controller is only configured to support 16 MSI interrupts.

When vntb_epf_peer_db_work() later tries to raise an interrupt:

drivers/pci/endpoint/functions/pci-epf-vntb.c:vntb_epf_peer_db_work() {
    ...
	ret = pci_epc_raise_irq(epf->epc, func_no, vfunc_no,
				irq_type, interrupt_num);
    ...
}

Will pci_epc_raise_irq() fail for interrupt_num values greater than 16,
causing doorbell events to be dropped?

>  		if (ret) {
>  			dev_err(dev, "MSI configuration failed\n");
>  			return ret;
>  		}
>  	}


[Severity: Critical]
This is a pre-existing issue, but can a compromised remote host force an
out-of-bounds physical memory map?

The remote host can provide an arbitrary argument during a
COMMAND_CONFIGURE_MW command. This argument acts as the memory window index
(mw) and is used directly in epf_ntb_configure_mw() without being checked
against MAX_MW:

drivers/pci/endpoint/functions/pci-epf-vntb.c:epf_ntb_cmd_handler() {
    ...
	case COMMAND_CONFIGURE_MW:
		ret = epf_ntb_configure_mw(ntb, argument);
    ...
}

drivers/pci/endpoint/functions/pci-epf-vntb.c:epf_ntb_configure_mw() {
    ...
	phys_addr = ntb->vpci_mw_phy[mw];
    ...
}

Would this out-of-bounds read on the ntb->vpci_mw_phy array allow the remote
host to grant itself arbitrary read/write access to the endpoint's physical
memory?


[Severity: High]
This isn't a bug introduced by this patch, but is it safe to free the
configuration BAR memory before unregistering the driver?

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

The epf_ntb_config_spad_bar_free(ntb) call frees the backing memory for
ntb->reg. Since the NTB device is still fully registered and accessible to
clients until pci_unregister_driver() completes, could a concurrent client
operation like vntb_epf_spad_read() dereference the freed ntb->reg memory,
leading to a use-after-free?


[Severity: High]
This is a pre-existing issue, but could an unbounded write to spad_count
cause an integer overflow and subsequent out-of-bounds memory writes?

drivers/pci/endpoint/functions/pci-epf-vntb.c:epf_ntb_spad_count_store() {
    ...
	ntb->spad_count = val;
    ...
}

drivers/pci/endpoint/functions/pci-epf-vntb.c:vntb_epf_spad_write() {
    ...
	int off = ctrl->spad_offset, ct = ctrl->spad_count * sizeof(u32);
    ...
}

If a user writes a very large value (e.g., 0x20000000) to the spad_count
configfs file, the memory allocation calculation
spad_size = 2 * spad_count * sizeof(u32) will overflow, resulting in a small
allocation.

Later in vntb_epf_spad_write(), the calculation ctrl->spad_count * sizeof(u32)
can wrap to a negative number. Will this cause writel() to access memory well
outside the allocated boundaries?

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