[PATCH v1] scsi: be2iscsi: Fix MSI-X IRQ unwind on request failure

[PATCH v1] scsi: be2iscsi: Fix MSI-X IRQ unwind on request failure

[Yuho Choi]

beiscsi_init_irqs() unwinds previously requested MSI-X IRQs if a later
request_irq() call fails. The unwind loop walks index j, but passed the
failing index i to pci_irq_vector(), so free_irq() did not use the same
IRQ and dev_id pair that was registered.

Use j for both the IRQ vector and be_eq entry when freeing previously
registered MSI-X IRQs.

Fixes: 831488669a33 ("scsi: be2iscsi: switch to pci_alloc_irq_vectors")
Signed-off-by: Yuho Choi <dbgh9129@gmail.com>
---
 drivers/scsi/be2iscsi/be_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index fd18d4d3d219..9833ab06b299 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -878,7 +878,7 @@ static int beiscsi_init_irqs(struct beiscsi_hba *phba)
 	return 0;
 free_msix_irqs:
 	for (j = i - 1; j >= 0; j--) {
-		free_irq(pci_irq_vector(pcidev, i), &phwi_context->be_eq[j]);
+		free_irq(pci_irq_vector(pcidev, j), &phwi_context->be_eq[j]);
 		kfree(phba->msi_name[j]);
 	}
 	return ret;
-- 
2.43.0

Re: [PATCH v1] scsi: be2iscsi: Fix MSI-X IRQ unwind on request failure

[sashiko-bot]

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

Pre-existing issues:
- [High] Erroneous error handling of pci_alloc_irq_vectors_affinity() in be2iscsi_enable_msix() restricts MSI-X to 1 CPU on success, and causes a buffer overflow on failure.
--

commit 770fa6df2516c59abfdd643e359fed37ffae37ec
Author: Yuho Choi <dbgh9129@gmail.com>

scsi: be2iscsi: Fix MSI-X IRQ unwind on request failure

This commit fixes the MSI-X IRQ unwind loop in beiscsi_init_irqs() when a
later request_irq() fails. It changes the vector index passed to
pci_irq_vector() from the failing index i to the current unwind index j to
correctly free the registered IRQs.

> diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
> index fd18d4d3d219b..9833ab06b299a 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -878,7 +878,7 @@ static int beiscsi_init_irqs(struct beiscsi_hba *phba)
>  	return 0;
>  free_msix_irqs:
>  	for (j = i - 1; j >= 0; j--) {
> -		free_irq(pci_irq_vector(pcidev, i), &phwi_context->be_eq[j]);
> +		free_irq(pci_irq_vector(pcidev, j), &phwi_context->be_eq[j]);

[Severity: High]
This is a pre-existing issue, but since this unwinding code depends on i
which iterates up to phba->num_cpus, can this loop result in a buffer
overflow?

Looking at be2iscsi_enable_msix(), it appears the error handling logic is
inverted when calling pci_alloc_irq_vectors_affinity():

drivers/scsi/be2iscsi/be_main.c:be2iscsi_enable_msix() {
    ...
		if (pci_alloc_irq_vectors_affinity(phba->pcidev, 2, nvec,
				PCI_IRQ_MSIX | PCI_IRQ_AFFINITY, &desc) < 0) {
			phba->num_cpus = nvec - 1;
			return;
		}
	}

	phba->num_cpus = 1;
    ...
}

On failure (< 0), it sets phba->num_cpus = nvec - 1, where nvec can be up
to 2048, which could lead to out-of-bounds array accesses in functions
iterating over num_cpus. For instance, in beiscsi_dev_probe():

drivers/scsi/be2iscsi/be_main.c:beiscsi_dev_probe() {
    ...
	for (i = 0; i < phba->num_cpus; i++) {
		pbe_eq = &phwi_context->be_eq[i];
		irq_poll_init(&pbe_eq->iopoll, be_iopoll_budget, be_iopoll);
	}
    ...
}

This loop writes into phwi_context->be_eq[i], which has a static size of
MAX_CPUS (64). Should be2iscsi_enable_msix() handle the return value
differently to prevent this overflow on failure, and to avoid hardcoding
phba->num_cpus = 1 on success?

>  		kfree(phba->msi_name[j]);
>  	}
>  	return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703053055.141337-1-dbgh9129@gmail.com?part=1