Re: [PATCH 26/37] scsi: mpt3sas: Enforce minimum reply frame size and validate chain segment size

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: mpt3sas: minimum reply_sz check in _base_allocate_memory_pools() too small
- [Medium] scsi: mpt3sas: chain_sz bound in _base_allocate_memory_pools() truncates u8
--

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

scsi: mpt3sas: Enforce minimum reply frame size and validate chain segment size

This commit adds validation in _base_allocate_memory_pools() to enforce a
minimum reply frame size based on MPI2DefaultReply_t and to bounds-check
IOCMaxChainSegmentSize when calculating chain_segment_sz.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 3622d8a42725b..1af9dc241666c 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -6395,21 +6395,46 @@ _base_allocate_memory_pools(struct MPT3SAS_ADAPTER *ioc)
>  	/* reply frame size */
>  	ioc->reply_sz = facts->ReplyFrameSize * 4;
>  
> +	/* Enforce minimum reply frame size */
> +	if (ioc->reply_sz < sizeof(MPI2DefaultReply_t)) {
> +		ioc_err(ioc,
> +			"Firmware reported an invalid ReplyFrameSize (%d bytes). Cannot safely attach.\n",
> +			ioc->reply_sz);
> +		return -EINVAL;
> +	}

[Severity: High]
Is sizeof(MPI2DefaultReply_t) (20 bytes) large enough to prevent out-of-bounds
reads and writes on the reply buffers when ReplyFrameSize is between 5 and 12
dwords (20 to 48 bytes)?

In mpt3sas_base_attach(), the internal command reply buffers and DMA reply
frames are allocated using ioc->reply_sz:

drivers/scsi/mpt3sas/mpt3sas_base.c:mpt3sas_base_attach() {
	...
	ioc->base_cmds.reply = kzalloc(ioc->reply_sz, GFP_KERNEL);
	ioc->port_enable_cmds.reply = kzalloc(ioc->reply_sz, GFP_KERNEL);
	ioc->transport_cmds.reply = kzalloc(ioc->reply_sz, GFP_KERNEL);
	ioc->scsih_cmds.reply = kzalloc(ioc->reply_sz, GFP_KERNEL);
	ioc->tm_cmds.reply = kzalloc(ioc->reply_sz, GFP_KERNEL);
	ioc->config_cmds.reply = kzalloc(ioc->reply_sz, GFP_KERNEL);
	ioc->ctl_cmds.reply = kzalloc(ioc->reply_sz, GFP_KERNEL);
	...
}

However, command submission and completion paths unconditionally zero, copy,
or dereference larger reply structures from these buffers without checking
ioc->reply_sz. For example, during mpt3sas_base_attach() ->
_base_make_ioc_operational(), _base_display_fwpkg_version() copies 24 bytes
(sizeof(Mpi2FWUploadReply_t)) from ioc->base_cmds.reply:

drivers/scsi/mpt3sas/mpt3sas_base.c:_base_display_fwpkg_version() {
	...
			memcpy(&mpi_reply, ioc->base_cmds.reply,
					sizeof(Mpi2FWUploadReply_t));
	...
}

And _base_static_config_pages() -> _config_request() writes 24 bytes
(sizeof(Mpi2ConfigReply_t)) into ioc->config_cmds.reply:

drivers/scsi/mpt3sas/mpt3sas_config.c:_config_request() {
	...
	memset(ioc->config_cmds.reply, 0, sizeof(Mpi2ConfigReply_t));
	...
}

Other paths access even larger reply structures, such as
mpt3sas_scsih_issue_tm() writing sizeof(Mpi2SCSITaskManagementReply_t) (28
bytes) to ioc->tm_cmds.reply, _transport_smp_handler() copying
sizeof(Mpi2SmpPassthroughReply_t) (28 bytes) from ioc->transport_cmds.reply,
mpt3sas_base_scsi_enclosure_processor() copying sizeof(Mpi2SepReply_t) (32
bytes) from ioc->base_cmds.reply, and _scsi_send_scsi_io(), _scsih_io_done(),
and mpt3sas_ctl_done() casting reply buffers to Mpi2SCSIIOReply_t (52 bytes):

drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsi_send_scsi_io() {
	...
		mpi_reply = ioc->scsih_cmds.reply;
		transfer_packet->sense_length =
		   min_t(u32, le32_to_cpu(mpi_reply->SenseCount),
			 sizeof(transfer_packet->sense));
	...
}

Should the minimum ioc->reply_sz check require at least enough space for the
largest reply structure accessed by the driver, such as Mpi2SCSIIOReply_t?

> +
> +	/* calculate the max scatter element size */
> +	sge_size = max_t(u16, ioc->sge_size, ioc->sge_size_ieee);
> +
>  	/* chain segment size */
>  	if (ioc->hba_mpi_version_belonged != MPI2_VERSION) {
> -		if (facts->IOCMaxChainSegmentSize)
> -			ioc->chain_segment_sz =
> -					facts->IOCMaxChainSegmentSize *
> +		if (facts->IOCMaxChainSegmentSize) {
> +			/* compute in 32 bits so the product cannot wrap the u16 field */
> +			u32 chain_sz = (u32)facts->IOCMaxChainSegmentSize *
>  					MAX_CHAIN_ELEMT_SZ;
> -		else
> -		/* set to 128 bytes size if IOCMaxChainSegmentSize is zero */
> +
> +			/* reject values that wrap or leave < 2 SGEs per chain */
> +			if (chain_sz < 2 * sge_size || chain_sz > 0xFFFF) {

[Severity: Medium]
Can allowing chain_sz up to 0xFFFF truncate chain_offset when building
multi-chain IEEE scatter-gather lists in _base_build_sg_scmd_ieee()?

If facts->IOCMaxChainSegmentSize is between 257 and 4095, chain_sz is between
4112 and 65520 bytes, which passes this check and sets
ioc->max_sges_in_chain_message to facts->IOCMaxChainSegmentSize - 1 (256 to
4094).

When _base_build_sg_scmd_ieee() constructs an SGL that spans multiple chain
segments (sges_left > ioc->max_sges_in_chain_message), it sets chain_offset to
ioc->max_sges_in_chain_message and passes it to _base_add_sg_single_ieee():

drivers/scsi/mpt3sas/mpt3sas_base.c:_base_build_sg_scmd_ieee() {
	...
		sges_in_segment = (sges_left <=
		    ioc->max_sges_in_chain_message) ? sges_left :
		    ioc->max_sges_in_chain_message;
		chain_offset = (sges_left == sges_in_segment) ?
		    0 : sges_in_segment;
		chain_length = sges_in_segment * ioc->sge_size_ieee;
		if (chain_offset)
			chain_length += ioc->sge_size_ieee;
		_base_add_sg_single_ieee(sg_local, chain_sgl_flags,
		    chain_offset, chain_length, chain_dma);
	...
}

In _base_add_sg_single_ieee(), chain_offset is a u8 parameter written into
Mpi25IeeeSgeChain64_t::NextChainOffset (an 8-bit field in 16-byte SGE units,
with a maximum value of 255):

drivers/scsi/mpt3sas/mpt3sas_base.c:_base_add_sg_single_ieee() {
	Mpi25IeeeSgeChain64_t *sgel = paddr;

	sgel->Flags = flags;
	sgel->NextChainOffset = chain_offset;
	...
}

Should IOCMaxChainSegmentSize (or ioc->max_sges_in_chain_message) be bounded
so that chain_offset fits in the u8 NextChainOffset field?

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