Re: [PATCH 01/37] scsi: mpt3sas: Validate firmware-reported IOCRequestFrameSize

[sashiko-bot@kernel.org]

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

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

scsi: mpt3sas: Validate firmware-reported IOCRequestFrameSize

This patch validates the IOCFacts-reported request frame size in
_base_allocate_memory_pools() and rejects values smaller than
sizeof(Mpi25SCSIIORequest_t) to prevent adjacent memory corruption.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 1af25a22611a1..9bf92c4a14b7a 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -6373,6 +6373,12 @@ _base_allocate_memory_pools(struct MPT3SAS_ADAPTER *ioc)
>  
>  	/* request frame size */
>  	ioc->request_sz = facts->IOCRequestFrameSize * 4;
> +	if (ioc->request_sz < sizeof(Mpi25SCSIIORequest_t)) {
> +		ioc_err(ioc,
> +			"IOCFacts reported invalid RequestFrameSize (%u bytes)\n",
> +			ioc->request_sz);
> +		return -EINVAL;
> +	}

[Severity: High]
Is checking against sizeof(Mpi25SCSIIORequest_t) sufficient to prevent
out-of-bounds writes into adjacent request frames?

In drivers/scsi/mpt3sas/mpi/mpi2_init.h, Mpi25SCSIIORequest_t has a 96-byte
header followed by a single 16-byte SGE union (MPI25_SGE_IO_UNION), so
sizeof(Mpi25SCSIIORequest_t) is 112 bytes rather than 128 bytes. If IOCFacts
reports IOCRequestFrameSize between 28 and 31 dwords (or a value >= 0x4000
where the u16 multiplication facts->IOCRequestFrameSize * 4 wraps modulo
65536 to >= 112), ioc->request_sz is 112..124 bytes and passes this check.

Just below in _base_allocate_memory_pools(), the driver reserves one SGE
slot in the main frame for a chain element:

drivers/scsi/mpt3sas/mpt3sas_base.c:_base_allocate_memory_pools() {
	...
	max_sge_elements = ioc->request_sz - ((sizeof(Mpi2SCSIIORequest_t) -
	    sizeof(Mpi2SGEIOUnion_t)) + sge_size);
	ioc->max_sges_in_main_message = max_sge_elements/sge_size;
	...
}

When ioc->request_sz is 112..124 bytes and sge_size is 16, max_sge_elements
is less than sge_size, leaving ioc->max_sges_in_main_message == 0.

Furthermore, several paths unconditionally write up to 128 bytes (two SGE
slots after the 96-byte header, or sizeof(Mpi2DiagBufferPostRequest_t)) into
a request frame allocated from ioc->request:

1. base_make_prp_nvme() places the chain element in the second SGE entry at
bytes 112..127:

drivers/scsi/mpt3sas/mpt3sas_base.c:base_make_prp_nvme() {
	...
	main_chain_element = (pMpi25IeeeSgeChain64_t)&mpi_request->SGL;
	/*
	 * For NVMe the chain element needs to be the 2nd SG entry in the main
	 * message.
	 */
	main_chain_element = (Mpi25IeeeSgeChain64_t *)
		((u8 *)main_chain_element + sizeof(MPI25_IEEE_SGE_CHAIN64));
	...
}

2. _base_build_sg_ieee() and _base_build_sg() write two SGEs starting at
&mpi_request->SGL (up to byte 128 for 16-byte IEEE SGEs, or byte 120 for
12-byte 64-bit MPI SGEs) when both data_out_sz and data_in_sz are non-zero:

drivers/scsi/mpt3sas/mpt3sas_base.c:_base_build_sg_ieee() {
	...
	if (data_out_sz && data_in_sz) {
		/* WRITE sgel first */
		sgl_flags = MPI2_IEEE_SGE_FLAGS_SIMPLE_ELEMENT |
		    MPI2_IEEE_SGE_FLAGS_SYSTEM_ADDR;
		_base_add_sg_single_ieee(psge, sgl_flags, 0, data_out_sz,
		    data_out_dma);

		/* incr sgel */
		psge += ioc->sge_size_ieee;

		/* READ sgel last */
		sgl_flags |= MPI25_IEEE_SGE_FLAGS_END_OF_LIST;
		_base_add_sg_single_ieee(psge, sgl_flags, 0, data_in_sz,
		    data_in_dma);
	...
}

3. _ctl_diag_register_2() and _ctl_diag_read_buffer() populate a
128-byte Mpi2DiagBufferPostRequest_t in a frame from
mpt3sas_base_get_msg_frame():

drivers/scsi/mpt3sas/mpt3sas_ctl.c:_ctl_diag_register_2() {
	...
	for (i = 0; i < MPT3_PRODUCT_SPECIFIC_DWORDS; i++)
		mpi_request->ProductSpecific[i] =
			cpu_to_le32(ioc->product_specific[buffer_type][i]);
	...
}

Should this validate that ioc->request_sz is at least 128 bytes (ensuring at
least two SGE slots in the main message frame and covering
Mpi2DiagBufferPostRequest_t) while also guarding against u16 overflow in
facts->IOCRequestFrameSize * 4?

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