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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Missing validation of MSI-X capability before honoring peer host's MSI-X configuration command.
- [High] TOCTOU race in doorbell processing leading to IOMMU faults on the Root Complex.

Pre-existing issues:
- [High] The driver allows raising MSI interrupt numbers that exceed the hardcoded requested amount.
- [High] Integer overflow in `spad_size` calculation leads to out-of-bounds kernel memory accesses.
--

commit 1b7eaae5c163696a2f15c6766f5254a75b259ab0
Author: Koichiro Den <den@valinux.co.jp>
Subject: 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.
When MSI-X is selected, the host does not program MSI, so raising it can
issue a write to an invalid address. This was observed with an IOMMU
enabled on the RC.

> 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: Critical]
Is it safe to unconditionally set ntb->peer_msix based on the host's request
without verifying if the endpoint actually supports MSI-X?

If the host requests MSI-X but the endpoint doesn't support it
(epc_features->msix_capable is false), could this cause vntb_epf_peer_db_work()
to call pci_epc_raise_irq() with PCI_IRQ_MSIX, potentially triggering a NULL
pointer dereference if the EPC driver hasn't initialized MSI-X structures?

>  		ctrl->command_status = COMMAND_STATUS_OK;
>  		break;
>  	case COMMAND_TEARDOWN_DOORBELL:
[ ... ]
> @@ -454,7 +457,7 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
>  	struct device *dev = &epf->dev;
>  	u32 spad_count;
>  	void *base;
> -	int i;
> +	int i, ret;
>  	const struct pci_epc_features *epc_features = pci_epc_get_features(epf->epc,
>  								epf->func_no,
>  								epf->vfunc_no);
> @@ -462,6 +465,29 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
>  	spad_count = ntb->spad_count;
>  
>  	ctrl_size = ALIGN(sizeof(struct epf_ntb_ctrl), sizeof(u32));
[ ... ]
>  	spad_size = 2 * spad_count * sizeof(u32);
>  
>  	base = pci_epf_alloc_space(epf, ctrl_size + spad_size,

[Severity: High]
This is a pre-existing issue, but does the spad_size calculation have an integer
overflow risk?

Since spad_count is populated from configfs without bounds checking, could a
large value cause the 64-bit result of 2 * spad_count * sizeof(u32) to be
truncated when assigned to the 32-bit spad_size variable?

This could lead to a small memory allocation from pci_epf_alloc_space(). Later,
when vntb_epf_spad_write() calculates offsets using the un-wrapped spad_count,
could this write out of bounds and corrupt kernel memory?

>  				   barno, epc_features, 0);
>  	if (!base) {
>  		dev_err(dev, "Config/Status/SPAD alloc region fail\n");
[ ... ]
> @@ -521,16 +548,22 @@ 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]
This is a pre-existing issue, but I noticed that 16 MSI interrupts are
statically requested regardless of ntb->db_count.

If a user configures db_count to a value greater than 16 (up to MAX_DB_COUNT),
could vntb_epf_peer_db_work() compute an interrupt_num that exceeds the 16
requested vectors? Could raising an out-of-bounds MSI interrupt cause spurious
interrupts or issues on the Root Complex?

>  		if (ret) {
>  			dev_err(dev, "MSI configuration failed\n");
>  			return ret;
>  		}
>  	}
[ ... ]
> @@ -1523,6 +1557,7 @@ static void vntb_epf_peer_db_work(struct work_struct *work)
>  
>  	func_no = epf->func_no;
>  	vfunc_no = epf->vfunc_no;
> +	irq_type = READ_ONCE(ntb->peer_msix) ? PCI_IRQ_MSIX : PCI_IRQ_MSI;
>  
>  	/*
>  	 * Drain doorbells from peer_db_pending in snapshots (atomic64_xchg()).
> @@ -1536,16 +1571,16 @@ static void vntb_epf_peer_db_work(struct work_struct *work)
>  
>  		while (db_bits) {
[ ... ]
>  			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 irq_type is read and cached outside the budget loop, what happens if the
host concurrently sends COMMAND_CONFIGURE_DOORBELL to switch from MSI to MSI-X
during a doorbell storm?

Could the workqueue loop process new doorbells using the stale, cached irq_type,
causing the endpoint to raise MSI instead of the expected MSI-X, which might
lead to an IOMMU fault on the Root Complex?

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

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