scsi: mpi3mr: Fix out-of-bounds accesses and reference leaks

drivers/scsi/mpi3mr/mpi3mr_app.c static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job) · line 2390

[Severity: High]
This is a pre-existing issue, but does this lack validation of job->request_len when parsing the BSG request and buffer entries?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
...

	bsg_req = job->request;
	karg = (struct mpi3mr_bsg_mptcmd *)&bsg_req->cmd.mptcmd;
...
	bufcnt = karg->buf_entry_list.num_of_entries;
...
	buf_entries = karg->buf_entry_list.buf_entry;

...
}

And similarly in mpi3mr_bsg_process_drv_cmds():

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_drv_cmds() {
...

	bsg_req = job->request;
	drvrcmd = &bsg_req->cmd.drvrcmd;
	mrioc = mpi3mr_bsg_verify_adapter(drvrcmd->mrioc_id);

...
}

If a user intentionally specifies a request_len smaller than the size of struct mpi3mr_bsg_packet, could this result in an out-of-bounds slab memory read since the driver casts and accesses the buffer without checking the length?

drivers/scsi/mpi3mr/mpi3mr_app.c static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job) · line 2544

[Severity: High]
This is a pre-existing issue, but does this pointer arithmetic bypass bounds checks on 32-bit architectures?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
...

		case MPI3MR_BSG_BUFTYPE_MPI_REPLY:
			sgl_iter = sgl_din_iter;
			sgl_din_iter += buf_entries->buf_len;

...
}

If a user submits a massive buf_len (e.g., 0xFFFFFFFF), could the addition to sgl_din_iter (which is a u8 *) cause an integer wrap-around on 32-bit systems?

Since a wrapped pointer is mathematically smaller, would it bypass the safety check:

	if (sgl_din_iter > (din_buf + job->reply_payload.payload_len))

And allow an out-of-bounds heap write when the driver copies the reply:

	memcpy(drv_buf_iter->bsg_buf, bsg_reply_buf, tmplen);

[Severity: High]
This is a pre-existing issue, but does this code leak DMA coherent memory on command posting failures or timeouts?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_build_nvme_prp() {
...

	mrioc->prp_list_virt = dma_alloc_coherent(&mrioc->pdev->dev,
	    dev_pgsz, &mrioc->prp_list_dma, GFP_KERNEL);

...
}

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
...

		if (rval) {
			mrioc->bsg_cmds.is_waiting = 0;
			dprint_bsg_err(mrioc,
			    "%s: posting bsg request is failed\n", __func__);
			rval = -EAGAIN;
			if (block_io && stgt_priv)
				atomic_dec(&stgt_priv->block_io);
			goto out_unlock;
		}

...
}

If mpi3mr_admin_request_post() fails or if the command times out, the code jumps to out_unlock. Does out_unlock fail to free mrioc->prp_list_virt, leading to a permanent leak of DMA memory?

[Severity: High]
This is a pre-existing issue, but is there a potential Use-After-Free of stgt_priv if the SCSI target is removed concurrently?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
...

		if (tgtdev) {
			if (tgtdev->starget && tgtdev->starget->hostdata) {
				stgt_priv = (struct mpi3mr_stgt_priv_data *)
				    tgtdev->starget->hostdata;
				atomic_inc(&stgt_priv->block_io);
			}
			mpi3mr_tgtdev_put(tgtdev);
		}
...
		if (block_io && stgt_priv)
			atomic_dec(&stgt_priv->block_io);

...
}

The reference to the target device is dropped via mpi3mr_tgtdev_put(tgtdev) before the long wait. If the physical SCSI device is hot-unplugged during the wait for command completion, could the SCSI midlayer free the starget and stgt_priv before atomic_dec() is called?

[Severity: High]
This is a pre-existing issue, but is there a data race on mrioc->bsg_cmds state between the timeout handler and the interrupt handler?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
...

		if (!(mrioc->bsg_cmds.state & MPI3MR_CMD_COMPLETE)) {
			mrioc->bsg_cmds.is_waiting = 0;
			rval = -EAGAIN;

...
}

And in the interrupt handler:

drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_process_admin_reply_desc() {
...

	cmdptr->state |= MPI3MR_CMD_COMPLETE;
...
	cmdptr->state &= ~MPI3MR_CMD_PENDING;
...
	if (cmdptr->is_waiting) {
		cmdptr->is_waiting = 0;
		complete(&cmdptr->done);
	}

...
}

If a BSG command times out, the timeout handler locklessly sets is_waiting = 0 and aborts, allowing another thread to acquire the lock and submit a new command using the same structure.

If a delayed hardware interrupt for the original command fires while the new command is pending, could the non-atomic modifications to cmdptr->state in mpi3mr_process_admin_reply_desc corrupt the state of the new command and prematurely complete it?

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

drivers/scsi/mpi3mr/mpi3mr_app.c static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job) · line 2548

[Severity: High]
This is a pre-existing issue, but since this patch fixes bounds checks in this function, is the initial loop bounded correctly against the user allocation?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
    ...
    bufcnt = karg->buf_entry_list.num_of_entries;
    ...
    buf_entries = karg->buf_entry_list.buf_entry;
    ...
    for (count = 0; count < bufcnt; count++, buf_entries++, drv_buf_iter++) {
    ...
}

A user-space process submitting an MPI command via mpi3mr_bsg_request could provide a small request_len but a large, unchecked karg->buf_entry_list.num_of_entries.

Does this code overflow buf_entries[] by reading from arbitrary adjacent heap memory?

[Severity: High]
This isn't a bug introduced by this patch, but does this code cause corruption on big-endian systems?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
    ...
    dev_handle = tm_req->dev_handle;
    ...
}

And later in the same function:

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
    ...
    status_desc->ioc_status = mrioc->bsg_cmds.ioc_status;
    status_desc->ioc_log_info = mrioc->bsg_cmds.ioc_loginfo;
    ...
}

When an admin issues a SCSI Task Management BSG command, tm_req->dev_handle (__le16) is directly assigned to dev_handle (u16) without le16_to_cpu(), and native values are assigned to __le16/__le32 fields without cpu_to_le16/32().

Can this fail the mpi3mr_get_tgtdev_by_handle() lookup and corrupt the response sent to user-space, bypassing the block_io increment and enabling unsafe concurrent I/O during target reset?

drivers/scsi/mpi3mr/mpi3mr_app.c static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job) · line 2786

[Severity: High]
This isn't a bug introduced by this patch, but does dropping the target device reference earlier leave stgt_priv unprotected?

Earlier in this function, the tgtdev reference is dropped without pinning the SCSI target:

	if (tgtdev->starget && tgtdev->starget->hostdata) {
		stgt_priv = (struct mpi3mr_stgt_priv_data *)
		    tgtdev->starget->hostdata;
		atomic_inc(&stgt_priv->block_io);
	}
	mpi3mr_tgtdev_put(tgtdev);

If a concurrent device removal occurs, the SCSI midlayer could destroy the target and free its hostdata via mpi3mr_target_destroy() while the driver is processing the request or sleeping in wait_for_completion_timeout().

Could this lead to a use-after-free when atomic_dec(&stgt_priv->block_io) is called on this error path and on the completion path?

drivers/scsi/mpi3mr/mpi3mr_app.c static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job) · line 2788

[Severity: High]
This is also a pre-existing issue, but does this function properly validate the number of buffer entries against the allocated request payload size?

Earlier in mpi3mr_bsg_process_mpt_cmds():

	bufcnt = karg->buf_entry_list.num_of_entries;
	...
	for (count = 0; count < bufcnt; count++, buf_entries++, drv_buf_iter++) {
		switch (buf_entries->buf_type) {

If a userspace application supplies a small request_len but a large num_of_entries, could the loop read past the end of the job->request payload?

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3

drivers/scsi/mpi3mr/mpi3mr_app.c static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job) · line 2747

[Severity: Medium]
This isn't a bug introduced by this patch, but is dev_handle missing an endianness conversion here?

It appears dev_handle is read natively from the request structure:

mpi3mr_bsg_process_mpt_cmds() {
    ...
    dev_handle = tm_req->dev_handle;
    ...
}

Since dev_handle is defined as little-endian (__le16) in struct mpi3_scsi_task_mgmt_request, could Big-Endian systems read a byte-swapped value, causing mpi3mr_get_tgtdev_by_handle() to look up the wrong device?

drivers/scsi/mpi3mr/mpi3mr_app.c static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job) · line 2752

[Severity: High]
This is a pre-existing issue, but does caching stgt_priv here create a use-after-free risk later in the function?

When a user-space process issues a blocking command, the thread will wait for completion:

mpi3mr_bsg_process_mpt_cmds() {
    ...
    wait_for_completion_timeout(&mrioc->bsg_cmds.done, ...);
    if (block_io && stgt_priv)
        atomic_dec(&stgt_priv->block_io);
    ...
}

If the target device and its hostdata are concurrently destroyed while the thread is blocked, tgt_dev->starget is cleared:

mpi3mr_target_destroy() {
    ...
    tgt_dev->starget = NULL;
    ...
}

If this race occurs, wouldn't the BSG thread wake up and dereference the freed stgt_priv pointer when calling atomic_dec()?

drivers/scsi/mpi3mr/mpi3mr_app.c static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job) · line 2755

[Severity: High]
This is also a pre-existing issue, but is there a data race in the BSG timeout handling that can cause a use-after-free on the sense buffer?

In the timeout path, the user thread locklessly clears the pointer and frees the memory:

mpi3mr_bsg_process_mpt_cmds() {
    ...
    mrioc->bsg_cmds.sensebuf = NULL;
    mrioc->bsg_cmds.state = MPI3MR_CMD_NOTUSED;
    ...
    kfree(sense_buff_k);
    ...
}

If the admin reply ISR executes concurrently and checks the cmdptr->sensebuf condition before it is cleared:

mpi3mr_fw.c:mpi3mr_process_admin_reply_desc() {
    if (sense_buf && cmdptr->sensebuf) {
        cmdptr->is_sense = 1;
        memcpy(cmdptr->sensebuf, sense_buf, MPI3MR_SENSE_BUF_SZ);
    }
    ...
}

Could the ISR execute memcpy() into the freed sense_buff_k memory block?

drivers/scsi/mpi3mr/mpi3mr_app.c void mpi3mr_app_save_logdata_th(struct mpi3mr_ioc *mrioc, char *event_data, · line 2959

[Severity: High]
This is a pre-existing issue, but might this dereference a ZERO_SIZE_PTR and cause a kernel panic?

If the device firmware sets reply_frame_size to 5 (or less), mrioc->reply_sz becomes 20 (or less). In mpi3mr_enable_logdata():

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_enable_logdata() {
    ...
    mrioc->logdata_entry_sz =
        (mrioc->reply_sz - (sizeof(struct mpi3_event_notification_reply) - 4))
        + MPI3MR_BSG_LOGDATA_ENTRY_HEADER_SZ;
    ...
}

Since the sizeof expression evaluates as a size_t, 20 - 24 underflows. The result plus 4 wraps around to exactly 0 when assigned to the u16 logdata_entry_sz.

This causes kcalloc to return ZERO_SIZE_PTR (0x10). The check
if (!mrioc->logdata_buf) earlier in this function fails to catch this, leading to a panic when writing to the unmapped address 0x10.

Additionally, could this cause a division by zero in mpi3mr_get_logdata() when calculating num_entries?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_get_logdata() {
    ...
    num_entries = job->request_payload.payload_len / entry_sz;
    ...
}

drivers/scsi/mpi3mr/mpi3mr_app.c void mpi3mr_app_save_logdata_th(struct mpi3mr_ioc *mrioc, char *event_data, · line 2966

[Severity: High]
This isn't a bug introduced by this patch, but will this memcpy trigger a FORTIFY_SOURCE panic under -fstrict-flex-arrays=3?

The log data payload is copied into entry->data, which is defined as __u8 data[1]; in the UAPI header struct mpi3mr_logdata_entry.

When compiled with -fstrict-flex-arrays=3, the compiler treats data strictly as a 1-byte array. The fortified memcpy() will evaluate __member_size() as 1, and since log events typically have a size > 1, this will deterministically trigger a kernel panic upon receiving a log data event due to the detected buffer overflow.

drivers/scsi/mpi3mr/mpi3mr_app.c void mpi3mr_app_save_logdata_th(struct mpi3mr_ioc *mrioc, char *event_data, · line 2968

[Severity: High]
This isn't a bug introduced by this patch, but could the size calculation in mpi3mr_get_logdata() overflow and lead to silent data truncation?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_get_logdata() {
    u16 num_entries, sz, entry_sz = mrioc->logdata_entry_sz;
    ...
    sz = num_entries * entry_sz;
    ...
}

num_entries can be up to 400 (MPI3MR_BSG_LOGDATA_MAX_ENTRIES), and entry_sz is typically 236 bytes. The product 400 * 236 equals 94400, which exceeds the maximum value of a 16-bit unsigned integer (65535).

Might this silently truncate/wrap around to 28864, causing sg_copy_from_buffer() to return incomplete or corrupted log data back to user space without generating an error?

[Severity: High]
This is a pre-existing issue, but is there an unintentional fall-through from MPI3_EVENT_DEVICE_INFO_CHANGED to MPI3_EVENT_LOG_DATA?

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_os_handle_events() {
    ...
    case MPI3_EVENT_DEVICE_INFO_CHANGED:
    case MPI3_EVENT_LOG_DATA:

        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);
    ...
}

The switch case for MPI3_EVENT_DEVICE_INFO_CHANGED lacks the required process_evt_bh = 1; assignment and a break; statement.

Will this cause device information changes to be permanently ignored (since they are never queued to the fwevt_worker_thread), while erroneously passing the device page0 data payload to mpi3mr_app_save_logdata_th(), polluting the log data ring buffer?

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5

drivers/scsi/mpi3mr/mpi3mr_fw.c void *mpi3mr_get_reply_virt_addr(struct mpi3mr_ioc *mrioc, · line 159

While reviewing this patch, I noticed some pre-existing issues in the driver:

drivers/scsi/mpi3mr/mpi3mr_os.c static void mpi3mr_dev_rmhs_complete_iou(struct mpi3mr_ioc *mrioc, · line 2408

[Severity: High]
This is a pre-existing issue, but does a dispatch failure permanently strand the delayed device removal queue here?

If mpi3mr_dev_rmhs_send_tm() is called to process a delayed TM request, but mpi3mr_admin_request_post() fails during dispatch:

    retval = mpi3mr_admin_request_post(mrioc, &tm_req, sizeof(tm_req), 1);
    if (retval) {
        ...
        goto out_failed;
    }
    ...
out_failed:
    drv_cmd->state = MPI3MR_CMD_NOTUSED;
    clear_bit(cmd_idx, mrioc->devrem_bitmap);

The command is marked as unused and dropped. Because mpi3mr_dev_rmhs_complete_iou() only dequeues one element from delayed_rmhs_list and does not loop if the dispatch fails, the active command slot becomes idle.

Does this break the chain of asynchronous completions, leaving any remaining elements in the delayed list stranded indefinitely?

drivers/scsi/mpi3mr/mpi3mr_os.c static void mpi3mr_dev_rmhs_send_tm(struct mpi3mr_ioc *mrioc, u16 handle, · line 2533

[Severity: High]
This is a pre-existing issue, but does a memory allocation failure silently drop device removal requests here?

Later in mpi3mr_dev_rmhs_send_tm(), if no slots are available in devrem_bitmap, the code falls back to adding the request to the delayed list:

    if (cmd_idx >= MPI3MR_NUM_DEVRMCMD) {
        delayed_dev_rmhs = kzalloc_obj(*delayed_dev_rmhs, GFP_ATOMIC);
        if (!delayed_dev_rmhs)
            return;

If the atomic allocation fails, the function returns silently without propagating an error. Can this result in the controller waiting indefinitely for a handshake if the removal event is lost?

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8

drivers/scsi/mpi3mr/mpi3mr_os.c static void mpi3mr_dev_rmhs_send_tm(struct mpi3mr_ioc *mrioc, u16 handle, · line 2544

[Severity: Medium]
This isn't a bug introduced by this patch, but further down in mpi3mr_dev_rmhs_send_tm(), if the pre-allocated command slots are exhausted, the code falls back to allocating a delayed_dev_rmhs node:

    delayed_dev_rmhs = kzalloc_obj(*delayed_dev_rmhs, GFP_ATOMIC);
    if (!delayed_dev_rmhs)
        return;

If this GFP_ATOMIC allocation fails, the function returns without scheduling a retry or rolling back the device state (MPI3MR_DEV_REMOVE_HS_STARTED).

Can this silent failure leave the hardware state machine permanently wedged, waiting for the host to send this TM request?

[Severity: Medium]
This is a pre-existing issue, but looking at mpi3mr_target_alloc() in the same file, there appears to be a reference count leak on the error paths.

If __mpi3mr_get_tgtdev_by_perst_id() or __mpi3mr_get_tgtdev_by_addr_and_rphy() successfully finds a target device, but the device doesn't satisfy the conditional checks for PCIe devices:

    if (tgt_dev && !tgt_dev->is_hidden && tgt_dev->non_stl) {
        ...
    } else
        retval = -ENXIO;

Or for SAS/SATA devices:

    if (tgt_dev && !tgt_dev->is_hidden && !tgt_dev->non_stl &&
        (tgt_dev->dev_type == MPI3_DEVICE_DEVFORM_SAS_SATA)) {
        ...
    } else
        retval = -ENXIO;

In both cases, the code returns -ENXIO without dropping the reference count via mpi3mr_tgtdev_put(tgt_dev), and without assigning it to scsi_tgt_priv_data->tgt_dev for later cleanup.

Would this leak the mpi3mr_tgt_dev memory structures during device scanning if hidden or non-standard devices are present?

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9

drivers/scsi/mpi3mr/mpi3mr_os.c static void mpi3mr_sastopochg_evt_th(struct mpi3mr_ioc *mrioc, · line 2848

[Severity: High]
This is a pre-existing issue, but returning early here aborts top-half processing without clearing the caller's process_evt_bh flag.

In drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_os_handle_events(), process_evt_bh is set to 1 before calling mpi3mr_sastopochg_evt_th(). If we return early, the event is unconditionally scheduled for bottom-half processing.

Since the bottom-half blindly loops using the attacker-controlled num_entries on the fwevt buffer:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_sastopochg_evt_bh() {
    ...
    for (i = 0; i < event_data->num_entries; i++) {
    ...
}

Could this leave the bottom-half exposed to a heap out-of-bounds read when processing a malformed event?

drivers/scsi/mpi3mr/mpi3mr_os.c static void mpi3mr_sastopochg_evt_th(struct mpi3mr_ioc *mrioc, · line 2852

[Severity: High]
Since phy_entry is annotated with __counted_by(num_entries), won't the compiler-inserted bounds check for topo_evt->phy_entry[i] directly re-fetch topo_evt->num_entries from DMA memory?

If the firmware alters topo_evt->num_entries to be less than the loop index i during the loop in mpi3mr_sastopochg_evt_th(), wouldn't this bypass the local num_entries cache and trigger an array bounds sanitizer violation panic?

[Severity: High]
This isn't a bug introduced by this patch, but further down in mpi3mr_sastopochg_evt_th() there is a lockless access to tgtdev->starget that can race with device removal:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_sastopochg_evt_th() {
    ...
    if (tgtdev && tgtdev->starget && tgtdev->starget->hostdata)
        scsi_tgt_priv_data = (struct mpi3mr_stgt_priv_data *)
            tgtdev->starget->hostdata;
    ...
    case MPI3_EVENT_SAS_TOPO_PHY_RC_TARG_NOT_RESPONDING:
        if (scsi_tgt_priv_data) {
            scsi_tgt_priv_data->dev_removed = 1;
    ...
}

Is it possible that if this executes concurrently with mpi3mr_target_destroy(), tgtdev->starget could be cleared under lock and the scsi_target freed by the SCSI midlayer, causing a Use-After-Free when writing to scsi_tgt_priv_data->dev_removed?

drivers/scsi/mpi3mr/mpi3mr_os.c void mpi3mr_os_handle_events(struct mpi3mr_ioc *mrioc, · line 3180

[Severity: High]
This is a pre-existing issue, but is it intentional that MPI3_EVENT_DEVICE_INFO_CHANGED falls through to MPI3_EVENT_LOG_DATA?

When this happens, the payload is passed to mpi3mr_app_save_logdata_th() and process_evt_bh is not set to 1. This appears to cause the bottom-half event to never be scheduled, and might overwrite firmware log buffers with unrelated device information page data.

drivers/scsi/mpi3mr/mpi3mr_os.c void mpi3mr_os_handle_events(struct mpi3mr_ioc *mrioc, · line 3190

[Severity: High]
This is a pre-existing issue, but while this fixes the buffer copy size here, does it leave out-of-bounds reads unpatched in the top-half topology event handlers?

In mpi3mr_pcietopochg_evt_th(), the bounds checking still uses the unclamped firmware-provided length directly:

avail_len = event_reply->event_data_length * 4;
...
max_entries = (avail_len - ...) / sizeof(...);

If the event_data_length provided by firmware is larger than the physical DMA reply frame size, could this lead to reading out of bounds directly from the DMA mapping during the top-half processing loop?

[ ... ]

drivers/scsi/mpi3mr/mpi3mr_os.c void mpi3mr_os_handle_events(struct mpi3mr_ioc *mrioc, · line 3226

[Severity: High]
Since this patch restricts the allocated size of the heap buffer fwevt by clamping it to avail_reply_room, could this cause an out-of-bounds read in the bottom-half handler?

For instance, in mpi3mr_sastopochg_evt_bh(), the loop iterates up to the unclamped num_entries from the payload. If the allocation was truncated, iterating up to the original unvalidated num_entries could read past the end of the newly clamped fwevt heap allocation.

[Severity: High]
This is a pre-existing issue, but could truncating the event data size cause out-of-bounds slab reads later if the buffer size is exceptionally small?

If the clamped sz ends up being extremely small or zero, the bottom-half handlers like mpi3mr_fwevt_bh() blindly cast fwevt->event_data to fixed-size structures such as struct mpi3_device_page0 and access fields like persistent_id. Should there be a minimum size check before accessing these fields in the bottom half?

drivers/scsi/mpi3mr/mpi3mr_transport.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but looking at mpi3mr_sastopochg_evt_bh() in drivers/scsi/mpi3mr/mpi3mr_os.c, when processing events with a target not responding reason code:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_sastopochg_evt_bh() {
    ...
            mpi3mr_tgtdev_put(tgtdev);
            break;
    ...
        if (tgtdev)
            mpi3mr_tgtdev_put(tgtdev);
}

Does this result in a double decrement of the tgtdev reference count, causing premature freeing of the target device structure? Similar logic appears to exist in mpi3mr_pcietopochg_evt_bh() and mpi3mr_devinfochg_evt_bh().

[Severity: High]
This is a pre-existing issue, but looking at how the phy mask is updated in mpi3mr_sas_port_add() in drivers/scsi/mpi3mr/mpi3mr_transport.c:

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_sas_port_add() {
    ...
            mr_sas_port->phy_mask |= (1 << i);
    ...
}

Since 1 is a signed 32-bit integer, evaluating it up to num_phys (which can be 32 or greater on enterprise HBAs) could produce a negative value that sign-extends to 0xFFFFFFFF80000000ULL when implicitly cast to the u64 phy_mask.

Can this cause out-of-bounds array accesses on the phy arrays?

Another instance with the identical signed shift occurs in mpi3mr_refresh_sas_ports():

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_refresh_sas_ports() {
    ...
        h_port[port_idx].phy_mask = (1 << i);
    ...
}

[Severity: High]
This is a pre-existing issue, but looking at how phys_to_be_added is processed in mpi3mr_update_mr_sas_port():

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_update_mr_sas_port() {
    ...
    for_each_set_bit(i, (ulong *) &phys_to_be_added, BITS_PER_TYPE(u64)) {
    ...
}

The variable phys_to_be_added is allocated as a 64-bit u64. On 32-bit big-endian platforms, unsigned long is 4 bytes.

Is it possible that casting the u64 bitmask to (ulong *) for for_each_set_bit() causes it to process the most significant 32 bits first, treating bits 32-63 as bits 0-31 and vice versa, leading to incorrect port mappings?

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12

drivers/scsi/mpi3mr/mpi3mr_app.c static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc, · line 1494

[Severity: Medium]
This is a pre-existing issue, but is there a Time-of-Check to Time-of-Use (TOCTOU) race condition here?

Right above this block, the code evaluates list_empty(&mrioc->tgtdev_list) without holding a lock:

	if ((job->request_payload.payload_len <= sizeof(u64)) ||
		list_empty(&mrioc->tgtdev_list)) {
		sg_copy_from_buffer(job->request_payload.sg_list,
				    job->request_payload.sg_cnt,
				    &num_devices, sizeof(num_devices));
		return 0;
	}

If a target device is removed (e.g., hot unplug) right after the tgtdev_lock is released (where num_devices was counted) but before this list_empty() check, could we return a non-zero device count to user space without actually populating the device mapping data array?

This might lead the application to read uninitialized memory from its own buffer as valid device information.

drivers/scsi/mpi3mr/mpi3mr_app.c static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc, · line 1500

[Severity: Medium]
This isn't a bug introduced by this patch, but while reviewing the integer overflow fix here, I noticed a similar pre-existing issue in mpi3mr_get_logdata().

In mpi3mr_get_logdata(), the sz variable is declared as u16 and can overflow when multiplied:

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_get_logdata() {
    ...
	u16 sz, num_entries, entry_sz;
    ...
	sz = num_entries * entry_sz;
    ...
}

If the firmware advertises a reply_frame_size that makes entry_sz > 163 bytes (e.g., a standard 256-byte frame), and we request MPI3MR_BSG_LOGDATA_MAX_ENTRIES (400), could num_entries * entry_sz overflow the 16-bit sz variable?

This could silently truncate the amount of log data copied to user space, causing the management tool to read uninitialized memory for the remaining entries.

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13

drivers/scsi/mpi3mr/mpi3mr_os.c static void mpi3mr_pcietopochg_evt_th(struct mpi3mr_ioc *mrioc, · line 2769

[Severity: High]
This is a pre-existing issue, but the patch provides an incomplete fix. Does this code blindly trust event_data_length provided by the hardware without clamping it to the physical size of the DMA reply buffer?

If a malicious or buggy hardware controller sends a PCIe topology change event with a large event_data_length (e.g., 255) and a large num_entries, avail_len could become up to 1020, exceeding the actual physical buffer size (mrioc->reply_sz, typically 128 bytes).

Since avail_len is not clamped, max_entries would become artificially large, bypassing the loop bounds check. Could this result in an out-of-bounds read of kernel memory adjacent to the DMA reply buffer?

drivers/scsi/mpi3mr/mpi3mr_os.c static void mpi3mr_pcietopochg_evt_th(struct mpi3mr_ioc *mrioc, · line 2782

[Severity: High]
This isn't a bug introduced by this patch, but the patch's attempt to fix the bounds check might be bypassed in the bottom half. If the top-half handler returns early here, what happens to process_evt_bh which was set to 1 prior to this call by mpi3mr_os_handle_events()?

If process_evt_bh is not cleared, wouldn't the bottom-half handler mpi3mr_pcietopochg_evt_bh() still be scheduled and iterate out-of-bounds over the same malformed event data?

mpi3mr_pcietopochg_evt_bh():

	for (i = 0; i < event_data->num_entries; i++) {

drivers/scsi/mpi3mr/mpi3mr_os.c static void mpi3mr_pcietopochg_evt_th(struct mpi3mr_ioc *mrioc, · line 2786

[Severity: High]
This isn't a bug introduced by this patch, but the patch's attempt to fix the TOCTOU vulnerability might be nullified. Even though num_entries is cached in a local variable, doesn't accessing topo_evt->port_entry[i] trigger a compiler-inserted bounds check because of the __counted_by(num_entries) attribute in the structure definition?

Under CONFIG_UBSAN_BOUNDS, this would force the compiler to re-read topo_evt->num_entries directly from DMA memory to perform the bounds check.

If a malicious device modifies this value post-validation, could it trigger a bounds check failure and panic the kernel?

drivers/scsi/mpi3mr/mpi3mr_os.c

[Severity: High]
This is a pre-existing issue that was not addressed, but is it safe to access tgtdev->starget and starget->hostdata locklessly here?

If mpi3mr_target_destroy() executes concurrently, it can take the lock, set tgtdev->starget = NULL, release the lock, and call kfree(starget->hostdata).

Could the top-half handler race with this destruction and lead to a Use-After-Free when dereferencing scsi_tgt_priv_data immediately afterwards?

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14

drivers/scsi/mpi3mr/mpi3mr_os.c static void mpi3mr_fwevt_add_to_list(struct mpi3mr_ioc *mrioc, · line 243

[Severity: High]
This isn't a bug introduced by this patch, but looking at how firmware events are managed and cleaned up, is there a use-after-free risk during cancellation?

When mpi3mr_cleanup_fwevt_list() flushes pending events, it removes the event from the list and immediately drops its list reference:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_dequeue_fwevt() {
    ...
    list_del_init(&fwevt->list);
    /*
     * Put fwevt reference count after
     * removing it from fwevt_list
     */
    mpi3mr_fwevt_put(fwevt);
    ...
}

It then calls mpi3mr_cancel_work(fwevt), which invokes cancel_work_sync(&fwevt->work). If the work is already executing but hasn't yet removed itself from the list in mpi3mr_fwevt_bh(), cancel_work_sync() will block waiting for the worker thread.

When the worker thread completes, it drops all remaining references and frees the event memory:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_fwevt_worker() {
    ...
    mpi3mr_fwevt_bh(fwevt->mrioc, fwevt);
    /*
     * Put fwevt reference count after
     * dequeuing it from worker queue
     */
    mpi3mr_fwevt_put(fwevt);
}

Does this leave cancel_work_sync() operating on a freed work_struct?

[Severity: High]
This is a pre-existing issue, but there appears to be a data race when accessing mrioc->current_event in the cleanup path.

In mpi3mr_cleanup_fwevt_list(), after draining the list, the current_event pointer is accessed without holding a lock or taking a reference:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_cleanup_fwevt_list() {
    ...
    if (mrioc->current_event) {
        fwevt = mrioc->current_event;
        ...
        if (current_work() == &fwevt->work || fwevt->pending_at_sml) {
    ...
}

Concurrently, if the worker thread is finishing execution in mpi3mr_fwevt_bh(), it drops the reference and sets the pointer to NULL:

    mpi3mr_fwevt_put(fwevt);
    mrioc->current_event = NULL;

If the cleanup thread reads mrioc->current_event before it is set to NULL, and the worker thread completes and drops its final reference (freeing the event), could the cleanup thread dereference freed memory when evaluating fwevt->pending_at_sml or calling mpi3mr_cancel_work(fwevt)?

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=16

drivers/scsi/mpi3mr/mpi3mr_os.c static void mpi3mr_fwevt_bh(struct mpi3mr_ioc *mrioc, · line 2131

[Severity: Critical]
This is a pre-existing issue, but does assigning mrioc->current_event here lead to a use-after-free during driver teardown or reset?

If mpi3mr_cleanup_fwevt_list() is executing concurrently, it performs a lockless read of mrioc->current_event:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_cleanup_fwevt_list() {
        ...
        fwevt = mrioc->current_event;
        ...
}

At the end of mpi3mr_fwevt_bh(), the worker thread drops the reference and NULLs the pointer:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_fwevt_bh() {
        ...
out:
        mpi3mr_fwevt_put(fwevt);
        mrioc->current_event = NULL;
}

If mpi3mr_cleanup_fwevt_list() captures the pointer before the worker thread NULLs it, but accesses it after mpi3mr_fwevt_put() frees the memory, could this corrupt memory when interacting with the freed event's work struct?

drivers/scsi/mpi3mr/mpi3mr_os.c static void mpi3mr_fwevt_bh(struct mpi3mr_ioc *mrioc, · line 2133

[Severity: High]
This isn't a bug introduced by this patch, but does this lockless check fully resolve the race condition described in the commit message?

Since this check is evaluated early in the handler, could an in-flight worker pass this check, get preempted, and then attempt to access hardware registers concurrently with mpi3mr_error_detected() unmapping and NULLing sysif_regs?

For example, if the worker proceeds to mpi3mr_admin_request_post():

drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_admin_request_post() {
        ...
        writel(mrioc->admin_req_pi, &mrioc->sysif_regs->admin_request_queue_pi);
        ...
}

Would this still result in a NULL pointer dereference or unmapped memory access if the PCI error recovery path clears mrioc->sysif_regs after the boolean check in mpi3mr_fwevt_bh() has already passed?