Re: [PATCH v2] scsi: mpt3sas: fix nr_msix underflow in _base_assign_reply_queues()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Clamping `ioc->reply_queue_count` to `MaxMSIxVectors` without adjusting `ioc->iopoll_q_start_index` or `ioc->high_iops_queues` causes severe integer underflows when setting up block multi-queue mappings.
- [High] Missing bounds check on `cpu` in `_base_assign_reply_queues` fallback path allows out-of-bounds heap write.
- [Medium] Incorrect loop termination condition `cpu >= nr_cpus` breaks MSI-X vector assignment on sparse CPU masks.
--

commit ae1909467e242e2fb7afde51ded872b5492ad061
Author: Ivy Lopez <skunkolee@gmail.com>

scsi: mpt3sas: fix nr_msix underflow in _base_assign_reply_queues()

This commit fixes a bug in the fallback path of _base_assign_reply_queues()
where iopoll_q_count was incorrectly subtracted from nr_msix. It ensures
that the reserved queue count is checked against nr_msix before performing
the subtraction to prevent underflows that could break affinity grouping or
cause a divide-by-zero.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 1af25a22611a1..40888b07b5a26 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -3275,7 +3275,11 @@ _base_assign_reply_queues(struct MPT3SAS_ADAPTER *ioc)
>  
>  fall_back:
>  	cpu = cpumask_first(cpu_online_mask);
> -	nr_msix -= (ioc->high_iops_queues - iopoll_q_count);
> +	if (ioc->high_iops_queues + iopoll_q_count >= nr_msix) {
> +		ioc_warn(ioc, "high_iops_queues and iopoll_q_count exceed available MSI-X vectors\n");
> +		return;
> +	}
> +	nr_msix -= (ioc->high_iops_queues + iopoll_q_count);
>  	index = 0;
>  
>  	list_for_each_entry(reply_q, &ioc->reply_queue_list, list) {

[Severity: High]
This is a pre-existing issue, but does clamping ioc->reply_queue_count
to MaxMSIxVectors earlier in this function cause integer underflows in
the block layer mappings?

In _base_assign_reply_queues(), the driver initially clamps the count:

drivers/scsi/mpt3sas/mpt3sas_base.c:_base_assign_reply_queues() {
    ...
	nr_msix = ioc->reply_queue_count = min(ioc->reply_queue_count,
					       ioc->facts.MaxMSIxVectors);
    ...
}

But it doesn't adjust ioc->iopoll_q_start_index or ioc->high_iops_queues.
Later, this clamped value is subtracted in scsih_map_queues():

drivers/scsi/mpt3sas/mpt3sas_scsih.c:scsih_map_queues() {
    ...
	int iopoll_q_count = ioc->reply_queue_count - nr_msix_vectors;
    ...
}

And in scsih_init():

drivers/scsi/mpt3sas/mpt3sas_scsih.c:scsih_init() {
    ...
		shost->nr_hw_queues =
		    ioc->reply_queue_count - ioc->high_iops_queues;
    ...
		iopoll_q_count =
		    ioc->reply_queue_count - ioc->iopoll_q_start_index;
    ...
}

If the firmware advertises fewer MSI-X vectors than initially reserved,
could these subtractions evaluate to negative integers that wrap around
to massive unsigned values, causing out-of-bounds loop iterations and
memory allocation failures?

[Severity: Medium]
This is a pre-existing issue, but does the fallback loop handle sparse CPU
masks correctly?

drivers/scsi/mpt3sas/mpt3sas_base.c:_base_assign_reply_queues() {
    ...
	list_for_each_entry(reply_q, &ioc->reply_queue_list, list) {
    ...
		if (cpu >= nr_cpus)
			break;
    ...
		for (i = 0 ; i < group ; i++) {
			ioc->cpu_msix_table[cpu] = reply_q->msix_index;
			cpu = cpumask_next(cpu, cpu_online_mask);
		}
    ...
}

If a system has a sparse CPU topology (e.g., CPU IDs 0 and 4, meaning
nr_cpus is 2), wouldn't the check cpu >= nr_cpus improperly compare a
discrete CPU ID against the aggregate online CPU count and terminate the
loop prematurely?

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing bounds
check that could lead to an out-of-bounds heap write?

Earlier in _base_assign_reply_queues(), nr_cpus is sampled locklessly:

drivers/scsi/mpt3sas/mpt3sas_base.c:_base_assign_reply_queues() {
    ...
	nr_cpus = num_online_cpus();
    ...
}

Later, the inner fallback loop assigns array elements using the CPU ID:

drivers/scsi/mpt3sas/mpt3sas_base.c:_base_assign_reply_queues() {
    ...
		for (i = 0 ; i < group ; i++) {
			ioc->cpu_msix_table[cpu] = reply_q->msix_index;
			cpu = cpumask_next(cpu, cpu_online_mask);
		}
    ...
}

If a new CPU is hotplugged with a higher ID, or a concurrent hot-unplug
reduces the online count while this loop runs, could cpumask_next()
return a CPU ID that exceeds ioc->cpu_msix_table_sz?

Since this fallback loop lacks the bounds check present in the main
SMP affinity path (if (cpu >= ioc->cpu_msix_table_sz)), could this
write past the end of the dynamically allocated cpu_msix_table array?

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