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

[sashiko-bot@kernel.org]

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