Re: [PATCH 19/37] scsi: mpt3sas: Reject implausibly small MaxReplyDescriptorPostQueueDepth

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: mpt3sas: scsiio_depth underflow in _base_allocate_memory_pools()
--

commit 3982113aa57710616a39847fb5922b95a8b805cf
Author: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>

scsi: mpt3sas: Reject implausibly small MaxReplyDescriptorPostQueueDepth

This commit adds a minimum bound check on MaxReplyDescriptorPostQueueDepth in
_base_allocate_memory_pools() to prevent an unsigned underflow when computing
ioc->hba_queue_depth.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 4e2cee2e13fc2..12295ab544ad5 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -6452,6 +6452,13 @@ _base_allocate_memory_pools(struct MPT3SAS_ADAPTER *ioc)
>  				(ioc->reply_post_queue_depth % 16);
>  	}
>  
> +	if (facts->MaxReplyDescriptorPostQueueDepth <
> +	    MPT3SAS_MIN_REPLY_POST_QUEUE_DEPTH) {
> +		ioc_err(ioc, "MaxReplyDescriptorPostQueueDepth is too small (%u)\n",
> +			facts->MaxReplyDescriptorPostQueueDepth);
> +		return -EINVAL;
> +	}
> +
>  	if (ioc->reply_post_queue_depth >
>  	    facts->MaxReplyDescriptorPostQueueDepth) {
>  		ioc->reply_post_queue_depth =
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h
> index 47255bf9cdda1..192b8e8a5b4d4 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.h
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
> @@ -145,6 +145,15 @@
>  #define MPT_MAX_HBA_NUM_PHYS		32
>  
>  #define INTERNAL_CMDS_COUNT		10	/* reserved cmds */
> +
> +/*
> + * Minimum reply post queue depth for hba_queue_depth to stay non-negative.
> + * Values 66-79 floor to 64 after 16-alignment, making ((64-64)/2)-1 = -1
> + * which wraps to 65535 as u16. The smallest value whose aligned floor exceeds
> + * 64 is 80 (80 % 16 == 0, giving ((80-64)/2)-1 == 7).
> + */
> +#define MPT3SAS_MIN_REPLY_POST_QUEUE_DEPTH	80

[Severity: High]
Does a minimum of 80 for MPT3SAS_MIN_REPLY_POST_QUEUE_DEPTH still leave
ioc->scsiio_depth and ioc->shost->can_queue vulnerable to underflow in
_base_allocate_memory_pools()?

Earlier in _base_allocate_memory_pools(), ioc->internal_depth and
ioc->hi_priority_depth are initialized as:

	ioc->internal_depth = min_t(int, (facts->HighPriorityCredit + (5)),
		(facts->RequestCredit / 4));
	if (ioc->internal_depth < INTERNAL_CMDS_COUNT) {
		...
		ioc->internal_depth = 10;
	}

	ioc->hi_priority_depth = ioc->internal_depth - (5);

So ioc->internal_depth is at least 10 and ioc->hi_priority_depth is at least
5 (and both can be larger when facts->HighPriorityCredit > 5).

When facts->MaxReplyDescriptorPostQueueDepth is between 80 and 95, it passes
the MPT3SAS_MIN_REPLY_POST_QUEUE_DEPTH check and clamps ioc->hba_queue_depth
to 7:

	if (ioc->reply_post_queue_depth >
	    facts->MaxReplyDescriptorPostQueueDepth) {
		ioc->reply_post_queue_depth =
				facts->MaxReplyDescriptorPostQueueDepth -
		    (facts->MaxReplyDescriptorPostQueueDepth % 16);
		ioc->hba_queue_depth =
				((ioc->reply_post_queue_depth - 64) / 2) - 1;
		ioc->reply_free_queue_depth = ioc->hba_queue_depth + 64;
	}

Later in _base_allocate_memory_pools(), ioc->hi_priority_depth and
ioc->internal_depth are subtracted from ioc->hba_queue_depth:

	ioc->scsiio_depth = ioc->hba_queue_depth -
	    ioc->hi_priority_depth - ioc->internal_depth;

	/* set the scsi host can_queue depth
	 * with some internal commands that could be outstanding
	 */
	ioc->shost->can_queue = ioc->scsiio_depth - INTERNAL_SCSIIO_CMDS_COUNT;

With ioc->hba_queue_depth == 7, subtracting ioc->hi_priority_depth (>= 5) and
ioc->internal_depth (>= 10) underflows the u16 ioc->scsiio_depth to at most
65528, setting ioc->shost->can_queue to 65525 and sizing the request and
lookup allocations for ~65k entries. Similarly, if
facts->MaxReplyDescriptorPostQueueDepth is in 96..111, ioc->hba_queue_depth
becomes 15, which yields ioc->scsiio_depth == 0 and a negative
ioc->shost->can_queue of -3 (or underflows ioc->scsiio_depth if
ioc->internal_depth > 10).

Should _base_allocate_memory_pools() also verify that the resulting
ioc->hba_queue_depth is greater than ioc->hi_priority_depth +
ioc->internal_depth + INTERNAL_SCSIIO_CMDS_COUNT?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1793925772-1303-1-git-send-email-shivasharan.srikanteshwara@broadcom.com?part=19