Re: [PATCH v2] scsi: mpi3mr: Validate reply frame size bounds

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: mpi3mr: MPI3MR_MIN_REPLY_FRAME_SZ_DWORDS too small for device events
--

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

scsi: mpi3mr: Validate reply frame size bounds

This commit validates that the firmware-reported reply_frame_size in
mpi3mr_issue_iocfacts() falls within [16, U16_MAX / 4] dwords to prevent
integer overflow when computing facts.reply_sz and underflow when sizing
logdata entries.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr.h b/drivers/scsi/mpi3mr/mpi3mr.h
> index d6e16707fd97c..e78383df619c3 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr.h
> +++ b/drivers/scsi/mpi3mr/mpi3mr.h
> @@ -83,6 +83,7 @@ extern atomic64_t event_counter;
>  #define MPI3MR_ADMIN_REPLY_Q_SIZE	(8 * MPI3MR_PAGE_SIZE_4K)
>  #define MPI3MR_ADMIN_REQ_FRAME_SZ	128
>  #define MPI3MR_ADMIN_REPLY_FRAME_SZ	16
> +#define MPI3MR_MIN_REPLY_FRAME_SZ_DWORDS	16	/* 64 bytes */
>  
>  /* Operational queue management definitions */
>  #define MPI3MR_OP_REQ_Q_QD		512
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> index f0d3cd398dd00..60a92366e5904 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,23 @@ 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:
> +	 * - Must be at least 16 dwords (64 bytes) to accommodate fixed reply
> +	 *   structures and avoid underflow/divide-by-zero in logdata sizing.
> +	 * - Must not exceed U16_MAX / 4 to prevent integer overflow.
> +	 */
> +	if (reply_frame_size < MPI3MR_MIN_REPLY_FRAME_SZ_DWORDS ||
> +	    reply_frame_size > (U16_MAX / 4)) {
> +		ioc_err(mrioc, "Invalid reply_frame_size %u (must be %u..%u dwords)\n",
> +			reply_frame_size, MPI3MR_MIN_REPLY_FRAME_SZ_DWORDS,
> +			(unsigned int)(U16_MAX / 4));
> +		retval = -EINVAL;
> +		goto out_unlock;
> +	}

[Severity: High]
Is a minimum of 16 dwords (64 bytes) for MPI3MR_MIN_REPLY_FRAME_SZ_DWORDS
sufficient for event notification replies that carry struct
mpi3_device_page0?

Event notification replies (struct mpi3_event_notification_reply) are
delivered in the mrioc->reply_buf frames of size mrioc->reply_sz
(reply_frame_size * 4), where the event_data payload starts at byte offset
24 (6 dwords). For MPI3_EVENT_DEVICE_ADDED, the event_data payload is
struct mpi3_device_page0 (64 bytes / 16 dwords), which requires a reply
frame of at least 24 + 64 = 88 bytes (22 dwords).

When reply_frame_size is between 16 and 21 dwords (64 to 84 bytes),
mpi3mr_issue_iocfacts() accepts it, and an incoming MPI3_EVENT_DEVICE_ADDED
event in drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_os_handle_events() casts
event_reply->event_data to struct mpi3_device_page0:

	case MPI3_EVENT_DEVICE_ADDED:
	{
		struct mpi3_device_page0 *dev_pg0 =
		    (struct mpi3_device_page0 *)event_reply->event_data;
		if (mpi3mr_create_tgtdev(mrioc, dev_pg0))
			...

mpi3mr_create_tgtdev() -> mpi3mr_update_tgtdev() then reads dev_pg0->
device_specific fields up to offset 59 of dev_pg0 (offset 83 of
event_reply), reading past a 64..84 byte reply frame and past the end of
mrioc->reply_buf when using the last frame in the pool.

In addition, when mpi3mr_os_handle_events() allocates fwevt for the bottom
half, it clamps the event_data allocation size sz to avail_reply_room
(mrioc->reply_sz - 24, which is 40..48 bytes for 16..18 dwords):

	if (mrioc->reply_sz > offsetof(struct mpi3_event_notification_reply, event_data))
		avail_reply_room = mrioc->reply_sz -
		    offsetof(struct mpi3_event_notification_reply, event_data);
	else
		avail_reply_room = 0;
	sz = min_t(u16, event_reply->event_data_length * 4, avail_reply_room);
	fwevt = mpi3mr_alloc_fwevt(sz);

When drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_fwevt_bh() later processes
MPI3_EVENT_DEVICE_ADDED with persistent_id ==
MPI3_DEVICE0_PERSISTENTID_INVALID and sas_transport_enabled set for a
SAS/SATA device:

	dev_pg0 = (struct mpi3_device_page0 *)fwevt->event_data;
	perst_id = le16_to_cpu(dev_pg0->persistent_id);
	handle = le16_to_cpu(dev_pg0->dev_handle);
	if (perst_id != MPI3_DEVICE0_PERSISTENTID_INVALID)
		mpi3mr_report_tgtdev_to_host(mrioc, perst_id);
	else if (mrioc->sas_transport_enabled &&
	    (dev_pg0->device_form == MPI3_DEVICE_DEVFORM_SAS_SATA)) {
		sasinf = &dev_pg0->device_specific.sas_sata_format;
		dev_info = le16_to_cpu(sasinf->device_info);

can reading sasinf->device_info at offset 50..51 of fwevt->event_data read
out of bounds of the slab object when only 40..48 bytes were allocated?

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