Re: [PATCH 20/37] scsi: mpt3sas: Validate CurrentHostPageSize before using it as a shift amount

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: mpt3sas: signed shift overflow and bad bound in _base_get_ioc_facts()
--

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

scsi: mpt3sas: Validate CurrentHostPageSize before using it as a shift amount

This commit validates facts->CurrentHostPageSize read from the IOCFacts reply
in _base_get_ioc_facts() and falls back to MPT3SAS_HOST_PAGE_SIZE_4K if it is
outside the allowed range before computing ioc->page_size.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 12295ab544ad5..3622d8a42725b 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -7532,8 +7532,15 @@ _base_get_ioc_facts(struct MPT3SAS_ADAPTER *ioc)
>  	facts->CurrentHostPageSize = mpi_reply.CurrentHostPageSize;
>  
>  	/*
> -	 * Get the Page Size from IOC Facts. If it's 0, default to 4k.
> +	 * Get the Page Size from IOC Facts. If it's 0 or too large to be a
> +	 * valid shift amount, default to 4k.
>  	 */
> +	if ((facts->CurrentHostPageSize >= 32) ||
> +	    (facts->CurrentHostPageSize != 0 && facts->CurrentHostPageSize < 12)) {
> +		ioc_warn(ioc, "CurrentHostPageSize(%d) is invalid: Setting default host page size to 4k\n",
> +			 facts->CurrentHostPageSize);
> +		facts->CurrentHostPageSize = MPT3SAS_HOST_PAGE_SIZE_4K;
> +	}
>  	ioc->page_size = 1 << facts->CurrentHostPageSize;

[Severity: Medium]
Can facts->CurrentHostPageSize == 31 still trigger a signed 32-bit shift
overflow here, and can values in 13..31 break downstream page_size consumers?

Because the left operand of 1 << facts->CurrentHostPageSize is a signed 32-bit
int literal (rather than 1U or BIT()), a value of 31 passes the >= 32 check
and shifts 1 into the sign bit, which is undefined behavior in C and still
triggers UBSAN shift-out-of-bounds.

In addition, _base_send_ioc_init() always programs the controller for a 4 KiB
host page size:

drivers/scsi/mpt3sas/mpt3sas_base.c:_base_send_ioc_init() {
	...
	mpi_request.HostPageSize = MPT3SAS_HOST_PAGE_SIZE_4K;
	...
}

Allowing CurrentHostPageSize values up to 31 produces ioc->page_size values up
to 0x80000000U (2 GiB). During probe in _base_allocate_memory_pools(), this
can overflow 32-bit pool sizing arithmetic and cause
_base_allocate_pcie_sgl_pool() to fail in dma_pool_create() with -ENOMEM
instead of falling back to 4 KiB:

drivers/scsi/mpt3sas/mpt3sas_base.c:_base_allocate_memory_pools() {
	...
		sz = nvme_blocks_needed * ioc->page_size;
		rc = _base_allocate_pcie_sgl_pool(ioc, sz);
		if (rc == -ENOMEM)
			return -ENOMEM;
		else if (rc == -EAGAIN)
			goto try_32bit_dma;
		total_sz += sz * ioc->scsiio_depth;
	...
}

If a controller reset via
mpt3sas_base_hard_reset_handler()->_base_get_ioc_facts() updates
ioc->page_size to 0x80000000U after pools are already allocated,
base_make_prp_nvme() subtracts u32 nvme_pg_size (ioc->page_size) from signed
int data_len and sge_len:

drivers/scsi/mpt3sas/mpt3sas_base.c:base_make_prp_nvme() {
	...
		sge_addr += nvme_pg_size;
		sge_len -= nvme_pg_size;
		data_len -= nvme_pg_size;

		if (data_len <= 0)
			break;
	...
}

When nvme_pg_size is 0x80000000U, subtracting it wraps negative signed 32-bit
values back to positive, which can walk sg_next(sg_scmd) past the end of the
scatterlist and overrun the 4 KiB pcie_sgl DMA buffer.

Should this use 1U << facts->CurrentHostPageSize (or BIT()) and restrict
CurrentHostPageSize to MPT3SAS_HOST_PAGE_SIZE_4K (or a safe upper bound that
cannot overflow DMA pool and NVMe PRP calculations)?

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