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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pci: vntb endpoint triggers out-of-bounds MSI/MSI-X vectors
--

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

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

ntb_hw_epf tries MSI-X first and falls back to MSI. It reports the
result in COMMAND_CONFIGURE_DOORBELL. pci-epf-vntb ignores MSIX_ENABLE,
configures only MSI, and always raises peer doorbells with PCI_IRQ_MSI.

> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index 07ba338732d4e..c30a73c537a0b 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c

[ ... ]

> @@ -308,6 +311,15 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
>  
>  	switch (command) {
>  	case COMMAND_CONFIGURE_DOORBELL:
> +		epc_features = pci_epc_get_features(ntb->epf->epc,
> +						    ntb->epf->func_no,
> +						    ntb->epf->vfunc_no);
> +		if (((argument & MSIX_ENABLE) && !epc_features->msix_capable) ||
> +		    (!(argument & MSIX_ENABLE) && !epc_features->msi_capable)) {
> +			ctrl->command_status = COMMAND_STATUS_ERROR;
> +			break;
> +		}
> +		ntb->peer_msix = argument & MSIX_ENABLE;

[Severity: High]
Does epf_ntb_cmd_handler in drivers/pci/endpoint/functions/pci-epf-vntb.c
only extract MSIX_ENABLE from argument while failing to extract and apply
the host's allocated doorbell count (argument & DB_COUNT_MASK)? If so,
could this leave the endpoint's doorbell count unbound?

>  		ctrl->command_status = COMMAND_STATUS_OK;
>  		break;
>  	case COMMAND_TEARDOWN_DOORBELL:

[ ... ]

> @@ -532,16 +553,24 @@ 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: High]
Is MSI allocation hardcoded to 16 vectors here in
epf_ntb_configure_interrupt, even though ntb->db_count can be up to 32?
Could this mismatch cause out-of-bounds MSI raises when MSI fallback is
used?

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

[ ... ]

> @@ -1553,16 +1584,16 @@ static void vntb_epf_peer_db_work(struct work_struct *work)
>  
>  		while (db_bits) {
>  			/*
> -			 * pci_epc_raise_irq() for MSI expects a 1-based
> -			 * interrupt number. The first usable doorbell starts
> -			 * at EPF_IRQ_DB_START in the legacy slot layout.
> +			 * pci_epc_raise_irq() expects a 1-based interrupt
> +			 * number for MSI and MSI-X. The first usable doorbell
> +			 * starts at EPF_IRQ_DB_START in the legacy slot layout.
>  			 *
>  			 * Legacy mapping (kept for compatibility):
>  			 *
> -			 *   MSI #1 : link event (reserved)
> -			 *   MSI #2 : unused (historical offset)
> -			 *   MSI #3 : doorbell bit 0 (DB#0)
> -			 *   MSI #4 : doorbell bit 1 (DB#1)
> +			 *   IRQ #1 : link event (reserved)
> +			 *   IRQ #2 : unused (historical offset)
> +			 *   IRQ #3 : doorbell bit 0 (DB#0)
> +			 *   IRQ #4 : doorbell bit 1 (DB#1)
>  			 *   ...
>  			 *
>  			 * Do not change this mapping to avoid breaking
> @@ -1573,7 +1604,7 @@ static void vntb_epf_peer_db_work(struct work_struct *work)
>  			db_bits &= ~BIT_ULL(db_bit);
>  
>  			ret = pci_epc_raise_irq(epf->epc, func_no, vfunc_no,
> -						PCI_IRQ_MSI, interrupt_num);
> +						irq_type, interrupt_num);

[Severity: High]
Since interrupt_num is calculated based on the un-negotiated db_count,
could this loop in vntb_epf_peer_db_work raise IRQs exceeding the host's
actual allocated vectors?

If this happens when the endpoint's local doorbell is triggered, could it
trigger an out-of-bounds access in the controller driver's MSI/MSI-X
table, leading to a wild DMA write?

>  			if (ret)
>  				dev_err(&epf->dev,
>  					"Failed to raise IRQ for interrupt_num %u: %d\n",

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