Re: [PATCH] scsi: mpi3mr: Fix integer overflow in reply frame size calculation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] scsi: mpi3mr: missing minimum reply_frame_size check in mpi3mr_issue_iocfacts()
--

commit b19e0f1b924cd80d0da53959c0e35b88d0537a9a
Author: Milan P. Gandhi <mgandhi@redhat.com>

scsi: mpi3mr: Fix integer overflow in reply frame size calculation

This commit validates reply_frame_size in mpi3mr_issue_iocfacts() to prevent
a u16 integer overflow when multiplying by 4 to compute mrioc->facts.reply_sz,
and rejects a zero reply_frame_size.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> index f0d3cd398dd00..2b72218e79fb3 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> @@ -3261,6 +3261,7 @@ static int mpi3mr_issue_iocfacts(struct mpi3mr_ioc *mrioc,
>  	struct mpi3_ioc_facts_data *facts_data)
>  {
>  	struct mpi3_ioc_facts_request iocfacts_req;
> +	u16 reply_frame_size;
>  	void *data = NULL;
>  	dma_addr_t data_dma;
>  	u32 data_len = sizeof(*facts_data);
> @@ -3318,6 +3319,19 @@ static int mpi3mr_issue_iocfacts(struct mpi3mr_ioc *mrioc,
>  		goto out_unlock;
>  	}
>  	memcpy(facts_data, (u8 *)data, data_len);
> +
> +	reply_frame_size = le16_to_cpu(facts_data->reply_frame_size);
> +	/*
> +	 * Validate reply_frame_size to prevent integer overflow when
> +	 * converting dwords to bytes (reply_frame_size * 4).
> +	 */
> +	if (!reply_frame_size || reply_frame_size > (U16_MAX / 4)) {
> +		ioc_err(mrioc, "Invalid reply_frame_size %u from firmware\n",
> +			reply_frame_size);
> +		retval = -EINVAL;
> +		goto out_unlock;
> +	}

[Severity: Critical]
Is rejecting only reply_frame_size == 0 at the lower bound sufficient when
validating firmware-provided facts data?

If buggy or malicious firmware reports reply_frame_size between 1 and 15
dwords (setting mrioc->facts.reply_sz and mrioc->reply_sz to 4..60 bytes),
several downstream consumers fail:

1. In drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_enable_logdata(),
   mrioc->logdata_entry_sz (u16) is computed as:

	mrioc->logdata_entry_sz =
	    (mrioc->reply_sz - (sizeof(struct mpi3_event_notification_reply) - 4))
	    + MPI3MR_BSG_LOGDATA_ENTRY_HEADER_SZ;
	mrioc->logdata_buf_idx = 0;
	mrioc->logdata_buf = kcalloc(MPI3MR_BSG_LOGDATA_MAX_ENTRIES,
	    mrioc->logdata_entry_sz, GFP_KERNEL);

   Since sizeof(struct mpi3_event_notification_reply) is 28 and
   MPI3MR_BSG_LOGDATA_ENTRY_HEADER_SZ is 4, this evaluates to
   mrioc->reply_sz - 20. When reply_frame_size == 5 (mrioc->reply_sz == 20),
   mrioc->logdata_entry_sz becomes 0 and kcalloc() returns ZERO_SIZE_PTR,
   which then causes a division by zero in mpi3mr_get_logdata():

	entry_sz = mrioc->logdata_entry_sz;
	num_entries = job->request_payload.payload_len / entry_sz;

   and a ZERO_SIZE_PTR dereference in mpi3mr_app_save_logdata_th(). When
   reply_frame_size is 1..4 (mrioc->reply_sz is 4..16),
   mrioc->logdata_entry_sz underflows u16 to 65520..65532.

2. In drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds(),
   bsg_reply_buf is allocated with sizeof(*bsg_reply_buf) + mrioc->reply_sz
   bytes. When MPI3MR_CMD_REPLY_VALID is not set, bsg_reply_buf->reply_buf
   is cast to struct mpi3_status_reply_descriptor * (8 bytes):

	status_desc = (struct mpi3_status_reply_descriptor *)
	    bsg_reply_buf->reply_buf;
	status_desc->ioc_status = mrioc->bsg_cmds.ioc_status;
	status_desc->ioc_log_info = mrioc->bsg_cmds.ioc_loginfo;

   When reply_frame_size == 1 (mrioc->reply_sz == 4), writing
   status_desc->ioc_log_info writes 4 bytes past the end of bsg_reply_buf.

3. In drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_alloc_reply_sense_bufs(),
   internal command reply buffers (such as mrioc->transport_cmds.reply,
   mrioc->pel_cmds.reply, and mrioc->host_tm_cmds.reply) are allocated with
   kzalloc(mrioc->reply_sz, GFP_KERNEL), and mrioc->reply_buf slots are
   sized to mrioc->reply_sz bytes. However, downstream completion paths read
   fixed-size reply structures up to 64 bytes without checking
   mrioc->reply_sz:

   - drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_post_transport_req()
     copies reply_sz (up to sizeof(struct mpi3_iounit_control_reply), 64
     bytes) from mrioc->transport_cmds.reply:

	if ((reply) && (mrioc->transport_cmds.state & MPI3MR_CMD_REPLY_VALID))
		memcpy((u8 *)reply, mrioc->transport_cmds.reply, reply_sz);

   - drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_process_op_reply_desc() reads
     struct mpi3_scsi_io_reply (56 bytes) from mrioc->reply_buf:

	sense_buf = mpi3mr_get_sensebuf_virt_addr(mrioc,
	    le64_to_cpu(scsi_reply->sense_data_buffer_address));

   - drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_process_admin_reply_desc(),
     PEL completion handlers, and task management handlers dereference
     struct mpi3_default_reply (16 bytes), struct mpi3_pel_reply (28 bytes),
     and struct mpi3_scsi_task_mgmt_reply (28 bytes).

Should mpi3mr_issue_iocfacts() enforce a minimum reply_frame_size of at least
16 dwords (64 bytes, covering struct mpi3_iounit_control_reply and all other
fixed reply structures) instead of only checking for zero?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103123316.223917-1-mgandhi@redhat.com?part=1