[PATCH 00/37] scsi: mpt3sas: Validate firmware/user input and fix error-path bugs

[PATCH 00/37] scsi: mpt3sas: Validate firmware/user input and fix error-path bugs

[Shivasharan S]

This series hardens the mpt3sas driver against malformed firmware
data and invalid ioctl/passthrough input, and fixes other locking,
reference-count, memory-leak and use-after-free bugs in the driver.

Shivasharan S (37):
  scsi: mpt3sas: Validate firmware-reported IOCRequestFrameSize
  scsi: mpt3sas: Validate RAID configuration page length
  scsi: mpt3sas: Pin Scsi_Host while an ioctl is using its ioc
  scsi: mpt3sas: Fix locking and bounds checks on host_trace_buffer
    sysfs files
  scsi: mpt3sas: Fix unlocked use of raid_device in
    enable_sdev_max_qd_store
  scsi: mpt3sas: Fix invalid kfree() of fw_event_work event_data
  scsi: mpt3sas: Validate SMP request/response length in bsg handler
  scsi: mpt3sas: Validate firmware/user supplied device handles against
    MaxDevHandle
  scsi: mpt3sas: Clamp topology/IR event entry counts to EventDataLength
  scsi: mpt3sas: NULL ioc->reply_post after freeing it
  scsi: mpt3sas: Fix port_table memory leak in _scsih_sas_port_refresh
  scsi: mpt3sas: Fix leak and dangling pointer for io_uring_poll_queues
  scsi: mpt3sas: Validate max_data_size in MPT3EVENTREPORT ioctl
  scsi: mpt3sas: Fix buffer overflow in _scsih_scsi_ioc_info
  scsi: mpt3sas: Fix Scsi_Host leak on invalid/tampered device probe
  scsi: mpt3sas: Fix page fault from stale ioc->chip after PCI error
    recovery
  scsi: mpt3sas: Clamp ValidEntries in driver trigger page update
    helpers
  scsi: mpt3sas: Clamp firmware reply copies to the allocated reply
    buffer
  scsi: mpt3sas: Reject implausibly small
    MaxReplyDescriptorPostQueueDepth
  scsi: mpt3sas: Validate CurrentHostPageSize before using it as a shift
    amount
  scsi: mpt3sas: Clamp sense_length before copying into
    transfer_packet->sense
  scsi: mpt3sas: Clamp num_luns in
    _scsih_wait_for_target_to_become_ready
  scsi: mpt3sas: Validate phy_number in mpt3sas_transport_update_links
  scsi: mpt3sas: Require CAP_SYS_RAWIO for mpt2ctl/mpt3ctl ioctls
  scsi: mpt3sas: Fix SGE offset boundary check in _ctl_do_mpt_command
  scsi: mpt3sas: Enforce minimum reply frame size and validate chain
    segment size
  scsi: mpt3sas: Replace panic() with error return in hard_reset_handler
  scsi: mpt3sas: Replace panic() calls with error returns in
    _config_request
  scsi: mpt3sas: Bound event-log copy size against reply frame size
  scsi: mpt3sas: Add _ctl_diag_buffer_reset() helper to clear diag
    buffer state
  scsi: mpt3sas: Restrict ioc_dump debugfs and replace raw ioc dump with
    text
  scsi: mpt3sas: Fix out-of-bounds bit operation in
    _scsih_check_topo_delete_events
  scsi: mpt3sas: Bound SMP passthrough buffer reads to actual buffer
    size
  scsi: mpt3sas: Release sas_device reference on error in
    mpt3sas_transport_port_add
  scsi: mpt3sas: NULL the event_log pointer after freeing it
  scsi: mpt3sas: Bound NVMe passthrough transfer size against PRP buffer
    capacity
  scsi: mpt3sas: Update driver version to 59.100.00.00

 drivers/scsi/mpt3sas/mpt3sas_base.c         |  91 ++++++++--
 drivers/scsi/mpt3sas/mpt3sas_base.h         |  13 +-
 drivers/scsi/mpt3sas/mpt3sas_config.c       |  64 +++++--
 drivers/scsi/mpt3sas/mpt3sas_ctl.c          | 182 +++++++++++++++++---
 drivers/scsi/mpt3sas/mpt3sas_debugfs.c      |  49 +++++-
 drivers/scsi/mpt3sas/mpt3sas_scsih.c        | 170 +++++++++++++++---
 drivers/scsi/mpt3sas/mpt3sas_transport.c    |  16 +-
 drivers/scsi/mpt3sas/mpt3sas_trigger_diag.c |   2 +
 8 files changed, 495 insertions(+), 92 deletions(-)

-- 
2.53.0

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

[Shivasharan S]

Reject an IOCFacts-reported request frame size smaller than
Mpi25SCSIIORequest_t, which would otherwise corrupt adjacent memory.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 1af25a22611a..9bf92c4a14b7 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;
+	}
 
 	/* reply frame size */
 	ioc->reply_sz = facts->ReplyFrameSize * 4;
-- 
2.53.0

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

[sashiko-bot]

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

[PATCH 02/37] scsi: mpt3sas: Validate RAID configuration page length

[Shivasharan S]

Check the RAID config page length and NumElements against the
allocated buffer before use, to avoid an out-of-bounds read.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_config.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_config.c b/drivers/scsi/mpt3sas/mpt3sas_config.c
index b0d5ef893600..75312d43e386 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_config.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_config.c
@@ -2655,6 +2655,7 @@ mpt3sas_config_get_volume_handle(struct MPT3SAS_ADAPTER *ioc, u16 pd_handle,
 	int config_num;
 	u16 element_type;
 	u16 phys_disk_dev_handle;
+	int max_num_elements;
 
 	*volume_handle = 0;
 	memset(&mpi_request, 0, sizeof(Mpi2ConfigRequest_t));
@@ -2672,6 +2673,13 @@ mpt3sas_config_get_volume_handle(struct MPT3SAS_ADAPTER *ioc, u16 pd_handle,
 
 	mpi_request.Action = MPI2_CONFIG_ACTION_PAGE_READ_CURRENT;
 	config_page_sz = (le16_to_cpu(mpi_reply.ExtPageLength) * 4);
+	if (config_page_sz < offsetof(Mpi2RaidConfigurationPage0_t,
+				      ConfigElement)) {
+		ioc_err(ioc, "%s: RAID configuration page length (%d) is too small\n",
+			__func__, config_page_sz);
+		r = -EIO;
+		goto out;
+	}
 	config_page = kmalloc(config_page_sz, GFP_KERNEL);
 	if (!config_page) {
 		r = -1;
@@ -2692,7 +2700,11 @@ mpt3sas_config_get_volume_handle(struct MPT3SAS_ADAPTER *ioc, u16 pd_handle,
 		    MPI2_IOCSTATUS_MASK;
 		if (ioc_status != MPI2_IOCSTATUS_SUCCESS)
 			goto out;
-		for (i = 0; i < config_page->NumElements; i++) {
+		max_num_elements = (config_page_sz -
+		    offsetof(Mpi2RaidConfigurationPage0_t, ConfigElement)) /
+		    sizeof(config_page->ConfigElement[0]);
+		for (i = 0; i < config_page->NumElements &&
+		     i < max_num_elements; i++) {
 			element_type = le16_to_cpu(config_page->
 			    ConfigElement[i].ElementFlags) &
 			    MPI2_RAIDCONFIG0_EFLAGS_MASK_ELEMENT_TYPE;
-- 
2.53.0

Re: [PATCH 02/37] scsi: mpt3sas: Validate RAID configuration page length

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: mpt3sas: u16 truncation of config_page_sz in _config_request()
--

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

scsi: mpt3sas: Validate RAID configuration page length

This commit validates that the RAID configuration page length is large enough
to hold Mpi2RaidConfigurationPage0_t before allocating the buffer, and bounds
the element loop by the number of elements that fit in the allocated buffer.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_config.c b/drivers/scsi/mpt3sas/mpt3sas_config.c
> index b0d5ef8936008..75312d43e3862 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_config.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_config.c
> @@ -2655,6 +2655,7 @@ mpt3sas_config_get_volume_handle(struct MPT3SAS_ADAPTER *ioc, u16 pd_handle,
>  	int config_num;
>  	u16 element_type;
>  	u16 phys_disk_dev_handle;
> +	int max_num_elements;
>  
>  	*volume_handle = 0;
>  	memset(&mpi_request, 0, sizeof(Mpi2ConfigRequest_t));
> @@ -2672,6 +2673,13 @@ mpt3sas_config_get_volume_handle(struct MPT3SAS_ADAPTER *ioc, u16 pd_handle,
>  
>  	mpi_request.Action = MPI2_CONFIG_ACTION_PAGE_READ_CURRENT;
>  	config_page_sz = (le16_to_cpu(mpi_reply.ExtPageLength) * 4);
> +	if (config_page_sz < offsetof(Mpi2RaidConfigurationPage0_t,
> +				      ConfigElement)) {
> +		ioc_err(ioc, "%s: RAID configuration page length (%d) is too small\n",
> +			__func__, config_page_sz);
> +		r = -EIO;
> +		goto out;
> +	}
>  	config_page = kmalloc(config_page_sz, GFP_KERNEL);
>  	if (!config_page) {
>  		r = -1;
> @@ -2692,7 +2700,11 @@ mpt3sas_config_get_volume_handle(struct MPT3SAS_ADAPTER *ioc, u16 pd_handle,
>  		    MPI2_IOCSTATUS_MASK;
>  		if (ioc_status != MPI2_IOCSTATUS_SUCCESS)
>  			goto out;
> -		for (i = 0; i < config_page->NumElements; i++) {
> +		max_num_elements = (config_page_sz -
> +		    offsetof(Mpi2RaidConfigurationPage0_t, ConfigElement)) /
> +		    sizeof(config_page->ConfigElement[0]);
> +		for (i = 0; i < config_page->NumElements &&
> +		     i < max_num_elements; i++) {

[Severity: Medium]
Can this read uninitialized heap memory from config_page when ExtPageLength is
0x4000 or larger?

In mpt3sas_config_get_volume_handle(), config_page_sz is a 32-bit int computed
as le16_to_cpu(mpi_reply.ExtPageLength) * 4, which can reach up to 262140.
The new check only enforces a lower bound before calling kmalloc(), which
allocates uninitialized memory.

When mpt3sas_config_get_volume_handle() then calls _config_request(), the
size is truncated to u16 both in the parameter list and in mem.sz:

drivers/scsi/mpt3sas/mpt3sas_config.c:_config_request() {
	...
	static int
	_config_request(struct MPT3SAS_ADAPTER *ioc, Mpi2ConfigRequest_t
		*mpi_request, Mpi2ConfigReply_t *mpi_reply, int timeout,
		void *config_page, u16 config_page_sz)
	...
		if (mpi_request->Header.PageLength)
			mem.sz = mpi_request->Header.PageLength * 4;
		else
			mem.sz = le16_to_cpu(mpi_reply->ExtPageLength) * 4;
}

Inside _config_request(), only (u16)config_page_sz bytes of config_page are
zeroed with memset() and populated with memcpy().

If ExtPageLength is between 0x4000 and 0x400b, (u16)config_page_sz wraps to
between 0 and 44 bytes, so _config_request() never initializes
config_page->NumElements, config_page->ConfigElement[], or (when wrapped to
11 bytes or less) config_page->ConfigNum. Even for larger values where
ExtPageLength >= 0x400c, max_num_elements is computed from the untruncated
32-bit config_page_sz, allowing the loop to read ConfigElement[i] entries
beyond the (u16)config_page_sz bytes actually populated by _config_request().

Also, because _config_request() overwrites mpi_reply on each iteration of the
while (1) loop and recomputes mem.sz from the new mpi_reply.ExtPageLength on
the next pass, a smaller ExtPageLength on a subsequent iteration would copy
fewer bytes than the initial config_page_sz used for max_num_elements.

Should mpt3sas_config_get_volume_handle() also reject config_page_sz > U16_MAX
(or allocate with kzalloc()) so that config_page_sz cannot truncate when
passed to _config_request()?

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

[PATCH 03/37] scsi: mpt3sas: Pin Scsi_Host while an ioctl is using its ioc

[Shivasharan S]

Take a Scsi_Host reference in _ctl_verify_adapter() and release it
once the ioctl is done, so the adapter can't be torn down while an
ioctl is in flight.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index 8bb947004885..f254e9984da8 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -447,6 +447,11 @@ _ctl_verify_adapter(int ioc_number, struct MPT3SAS_ADAPTER **iocpp,
 				continue;
 		}
 out:
+		if (!scsi_host_get(ioc->shost)) {
+			spin_unlock(&gioc_lock);
+			*iocpp = NULL;
+			return -1;
+		}
 		spin_unlock(&gioc_lock);
 		*iocpp = ioc;
 		return ioc_number;
@@ -2847,6 +2852,7 @@ _ctl_ioctl_main(struct file *file, unsigned int cmd, void __user *arg,
 	mutex_unlock(&ioc->ctl_cmds.mutex);
 out_unlock_pciaccess:
 	mutex_unlock(&ioc->pci_access_mutex);
+	scsi_host_put(ioc->shost);
 	return ret;
 }
 
-- 
2.53.0

Re: [PATCH 03/37] scsi: mpt3sas: Pin Scsi_Host while an ioctl is using its ioc

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: mpt3sas: NULL ioc->shost dereference in _ctl_verify_adapter()
--

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

scsi: mpt3sas: Pin Scsi_Host while an ioctl is using its ioc

This commit acquires a Scsi_Host reference in _ctl_verify_adapter() and
releases it at the end of _ctl_ioctl_main() to prevent the adapter from
being torn down while an ioctl is in flight.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> index 8bb947004885b..f254e9984da80 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> @@ -447,6 +447,11 @@ _ctl_verify_adapter(int ioc_number, struct MPT3SAS_ADAPTER **iocpp,
>  				continue;
>  		}
>  out:
> +		if (!scsi_host_get(ioc->shost)) {
> +			spin_unlock(&gioc_lock);
> +			*iocpp = NULL;
> +			return -1;
> +		}

[Severity: High]
Can this dereference a NULL ioc->shost pointer if _ctl_ioctl_main() runs
concurrently with _scsih_probe()?

In _mpt3sas_init(), mpt3sas_ctl_init() registers the /dev/mpt3ctl and
/dev/mpt2ctl misc devices before pci_register_driver() probes adapters, and
the device nodes also remain accessible during subsequent PCI hotplug or
driver bind operations.

In drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsih_probe(), ioc is added to
mpt3sas_ioc_list under gioc_lock before ioc->shost is assigned:

	INIT_LIST_HEAD(&ioc->list);
	spin_lock(&gioc_lock);
	list_add_tail(&ioc->list, &mpt3sas_ioc_list);
	spin_unlock(&gioc_lock);
	ioc->shost = shost;

If _ctl_verify_adapter() finds ioc on mpt3sas_ioc_list before
ioc->shost = shost runs, it passes NULL to scsi_host_get():

struct Scsi_Host *scsi_host_get(struct Scsi_Host *shost)
{
	if (scsi_get_host_state(shost) == SHOST_DEL ||
		!get_device(&shost->shost_gendev))
		return NULL;
	return shost;
}

which immediately dereferences shost while holding gioc_lock.

Even right after ioc->shost = shost is assigned in _scsih_probe(), the host
is in SHOST_CREATED state (not SHOST_DEL), so scsi_host_get(ioc->shost)
succeeds before _scsih_probe() initializes ioc->pci_access_mutex, sets
ioc->is_driver_loading = 1, or initializes ioc->ctl_cmds.mutex in
mpt3sas_base_attach().

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

[PATCH 04/37] scsi: mpt3sas: Fix locking and bounds checks on host_trace_buffer sysfs files

[Shivasharan S]

Serialize the host_trace_buffer* sysfs handlers on ctl_cmds.mutex and
clamp the reported/used buffer size to the actual allocation size.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c          | 50 +++++++++++++++++----
 drivers/scsi/mpt3sas/mpt3sas_trigger_diag.c |  2 +
 2 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index f254e9984da8..d696cb12047c 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -1791,6 +1791,10 @@ _ctl_diag_register_2(struct MPT3SAS_ADAPTER *ioc,
 	if (request_data == NULL) {
 		ioc->diag_buffer_sz[buffer_type] = 0;
 		ioc->diag_buffer_dma[buffer_type] = 0;
+		if (buffer_type == MPI2_DIAG_BUF_TYPE_TRACE) {
+			ioc->ring_buffer_sz = 0;
+			ioc->ring_buffer_offset = 0;
+		}
 		request_data = dma_alloc_coherent(&ioc->pdev->dev,
 				request_data_sz, &request_data_dma, GFP_KERNEL);
 		if (request_data == NULL) {
@@ -3628,9 +3632,12 @@ host_trace_buffer_size_show(struct device *cdev,
 	u32 size = 0;
 	struct DIAG_BUFFER_START *request_data;
 
+	mutex_lock(&ioc->ctl_cmds.mutex);
+
 	if (!ioc->diag_buffer[MPI2_DIAG_BUF_TYPE_TRACE]) {
 		ioc_err(ioc, "%s: host_trace_buffer is not registered\n",
 			__func__);
+		mutex_unlock(&ioc->ctl_cmds.mutex);
 		return 0;
 	}
 
@@ -3638,6 +3645,7 @@ host_trace_buffer_size_show(struct device *cdev,
 	    MPT3_DIAG_BUFFER_IS_REGISTERED) == 0) {
 		ioc_err(ioc, "%s: host_trace_buffer is not registered\n",
 			__func__);
+		mutex_unlock(&ioc->ctl_cmds.mutex);
 		return 0;
 	}
 
@@ -3649,7 +3657,11 @@ host_trace_buffer_size_show(struct device *cdev,
 	    le32_to_cpu(request_data->Reserved3) == 0x4742444c)
 		size = le32_to_cpu(request_data->Size);
 
+	/* Never trust firmware size beyond what was actually allocated */
+	size = min_t(u32, size,
+		     ioc->diag_buffer_sz[MPI2_DIAG_BUF_TYPE_TRACE]);
 	ioc->ring_buffer_sz = size;
+	mutex_unlock(&ioc->ctl_cmds.mutex);
 	return snprintf(buf, PAGE_SIZE, "%d\n", size);
 }
 static DEVICE_ATTR_RO(host_trace_buffer_size);
@@ -3674,10 +3686,15 @@ host_trace_buffer_show(struct device *cdev, struct device_attribute *attr,
 	struct MPT3SAS_ADAPTER *ioc = shost_priv(shost);
 	void *request_data;
 	u32 size;
+	u32 offset;
+	u32 buffer_sz;
+
+	mutex_lock(&ioc->ctl_cmds.mutex);
 
 	if (!ioc->diag_buffer[MPI2_DIAG_BUF_TYPE_TRACE]) {
 		ioc_err(ioc, "%s: host_trace_buffer is not registered\n",
 			__func__);
+		mutex_unlock(&ioc->ctl_cmds.mutex);
 		return 0;
 	}
 
@@ -3685,16 +3702,24 @@ host_trace_buffer_show(struct device *cdev, struct device_attribute *attr,
 	    MPT3_DIAG_BUFFER_IS_REGISTERED) == 0) {
 		ioc_err(ioc, "%s: host_trace_buffer is not registered\n",
 			__func__);
+		mutex_unlock(&ioc->ctl_cmds.mutex);
 		return 0;
 	}
 
-	if (ioc->ring_buffer_offset > ioc->ring_buffer_sz)
+	offset = ioc->ring_buffer_offset;
+	/* Never read beyond the actual allocated diag buffer */
+	buffer_sz = min_t(u32, ioc->ring_buffer_sz,
+			  ioc->diag_buffer_sz[MPI2_DIAG_BUF_TYPE_TRACE]);
+	if (offset > buffer_sz) {
+		mutex_unlock(&ioc->ctl_cmds.mutex);
 		return 0;
+	}
 
-	size = ioc->ring_buffer_sz - ioc->ring_buffer_offset;
+	size = buffer_sz - offset;
 	size = (size >= PAGE_SIZE) ? (PAGE_SIZE - 1) : size;
-	request_data = ioc->diag_buffer[0] + ioc->ring_buffer_offset;
+	request_data = ioc->diag_buffer[MPI2_DIAG_BUF_TYPE_TRACE] + offset;
 	memcpy(buf, request_data, size);
+	mutex_unlock(&ioc->ctl_cmds.mutex);
 	return size;
 }
 
@@ -3704,12 +3729,14 @@ host_trace_buffer_store(struct device *cdev, struct device_attribute *attr,
 {
 	struct Scsi_Host *shost = class_to_shost(cdev);
 	struct MPT3SAS_ADAPTER *ioc = shost_priv(shost);
-	int val = 0;
+	unsigned int val;
 
-	if (sscanf(buf, "%d", &val) != 1)
+	if (kstrtouint(buf, 0, &val))
 		return -EINVAL;
 
+	mutex_lock(&ioc->ctl_cmds.mutex);
 	ioc->ring_buffer_offset = val;
+	mutex_unlock(&ioc->ctl_cmds.mutex);
 	return strlen(buf);
 }
 static DEVICE_ATTR_RW(host_trace_buffer);
@@ -3755,13 +3782,17 @@ host_trace_buffer_enable_store(struct device *cdev,
 	struct mpt3_diag_register diag_register;
 	u8 issue_reset = 0;
 
+	if (sscanf(buf, "%9s", str) != 1)
+		return -EINVAL;
+
+	mutex_lock(&ioc->ctl_cmds.mutex);
+
 	/* don't allow post/release occurr while recovery is active */
 	if (ioc->shost_recovery || ioc->remove_host ||
-	    ioc->pci_error_recovery || ioc->is_driver_loading)
+	    ioc->pci_error_recovery || ioc->is_driver_loading) {
+		mutex_unlock(&ioc->ctl_cmds.mutex);
 		return -EBUSY;
-
-	if (sscanf(buf, "%9s", str) != 1)
-		return -EINVAL;
+	}
 
 	if (!strcmp(str, "post")) {
 		/* exit out if host buffers are already posted */
@@ -3832,6 +3863,7 @@ host_trace_buffer_enable_store(struct device *cdev,
 	}
 
  out:
+	mutex_unlock(&ioc->ctl_cmds.mutex);
 	return strlen(buf);
 }
 static DEVICE_ATTR_RW(host_trace_buffer_enable);
diff --git a/drivers/scsi/mpt3sas/mpt3sas_trigger_diag.c b/drivers/scsi/mpt3sas/mpt3sas_trigger_diag.c
index d9b7d0ee25b0..bda404a3ff3d 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_trigger_diag.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_trigger_diag.c
@@ -118,6 +118,7 @@ mpt3sas_process_trigger_data(struct MPT3SAS_ADAPTER *ioc,
 	dTriggerDiagPrintk(ioc, ioc_info(ioc, "%s: enter\n", __func__));
 
 	/* release the diag buffer trace */
+	mutex_lock(&ioc->ctl_cmds.mutex);
 	if ((ioc->diag_buffer_status[MPI2_DIAG_BUF_TYPE_TRACE] &
 	    MPT3_DIAG_BUFFER_IS_RELEASED) == 0) {
 		/*
@@ -131,6 +132,7 @@ mpt3sas_process_trigger_data(struct MPT3SAS_ADAPTER *ioc,
 		mpt3sas_send_diag_release(ioc, MPI2_DIAG_BUF_TYPE_TRACE,
 		    &issue_reset);
 	}
+	mutex_unlock(&ioc->ctl_cmds.mutex);
 
 	ioc->htb_rel.buffer_rel_condition = MPT3_DIAG_BUFFER_REL_TRIGGER;
 	if (event_data) {
-- 
2.53.0

Re: [PATCH 04/37] scsi: mpt3sas: Fix locking and bounds checks on host_trace_buffer sysfs files

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] scsi: mpt3sas: deadlock acquiring ctl_cmds.mutex in mpt3sas_process_trigger_data()
- [Medium] scsi: mpt3sas: unlocked ioc->htb_rel update in mpt3sas_process_trigger_data()
- [Medium] scsi: mpt3sas: stale ring_buffer_offset on buffer reuse in _ctl_diag_register_2()
- [Medium] scsi: mpt3sas: missing ctl_cmds.mutex lock in host_trace_buffer_enable_show()
--

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

scsi: mpt3sas: Fix locking and bounds checks on host_trace_buffer sysfs files

This commit serializes the host_trace_buffer sysfs handlers and diagnostic
trigger release path on ctl_cmds.mutex, and clamps the trace buffer size and
read bounds to the allocated DMA buffer size.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> index f254e9984da80..d696cb12047ce 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> @@ -1791,6 +1791,10 @@ _ctl_diag_register_2(struct MPT3SAS_ADAPTER *ioc,
>  	if (request_data == NULL) {
>  		ioc->diag_buffer_sz[buffer_type] = 0;
>  		ioc->diag_buffer_dma[buffer_type] = 0;
> +		if (buffer_type == MPI2_DIAG_BUF_TYPE_TRACE) {
> +			ioc->ring_buffer_sz = 0;
> +			ioc->ring_buffer_offset = 0;
> +		}
>  		request_data = dma_alloc_coherent(&ioc->pdev->dev,
>  				request_data_sz, &request_data_dma, GFP_KERNEL);
>  		if (request_data == NULL) {

[Severity: Medium]
What happens when a previously allocated trace buffer is released and then
re-posted with the same size, such as through host_trace_buffer_enable_store()
or the MPT3DIAGREGISTER ioctl?

In _ctl_diag_register_2(), when ioc->diag_buffer[MPI2_DIAG_BUF_TYPE_TRACE] is
already allocated and request_data_sz matches
ioc->diag_buffer_sz[MPI2_DIAG_BUF_TYPE_TRACE]:

	request_data = ioc->diag_buffer[buffer_type];
	request_data_sz = diag_register->requested_buffer_size;
	...
	if (request_data) {
		request_data_dma = ioc->diag_buffer_dma[buffer_type];
		if (request_data_sz != ioc->diag_buffer_sz[buffer_type]) {
			dma_free_coherent(&ioc->pdev->dev,
					ioc->diag_buffer_sz[buffer_type],
					request_data, request_data_dma);
			request_data = NULL;
		}
	}

request_data remains non-NULL and the existing DMA buffer is reused without
entering the request_data == NULL block. Would that leave ioc->ring_buffer_sz
and ioc->ring_buffer_offset with stale values from the previous trace capture
when reading host_trace_buffer?

[ ... ]

> @@ -3755,13 +3782,17 @@ host_trace_buffer_enable_store(struct device *cdev,
>  	struct mpt3_diag_register diag_register;
>  	u8 issue_reset = 0;
>  
> +	if (sscanf(buf, "%9s", str) != 1)
> +		return -EINVAL;
> +
> +	mutex_lock(&ioc->ctl_cmds.mutex);

[Severity: Medium]
The commit message notes that this change serializes the host_trace_buffer*
sysfs handlers on ctl_cmds.mutex, and host_trace_buffer_enable_store() now
acquires ioc->ctl_cmds.mutex before modifying
ioc->diag_buffer[MPI2_DIAG_BUF_TYPE_TRACE] and
ioc->diag_buffer_status[MPI2_DIAG_BUF_TYPE_TRACE].

However, host_trace_buffer_enable_show() in drivers/scsi/mpt3sas/mpt3sas_ctl.c
still reads those fields without holding ioc->ctl_cmds.mutex:

	if ((!ioc->diag_buffer[MPI2_DIAG_BUF_TYPE_TRACE]) ||
	   ((ioc->diag_buffer_status[MPI2_DIAG_BUF_TYPE_TRACE] &
	    MPT3_DIAG_BUFFER_IS_REGISTERED) == 0))
		return snprintf(buf, PAGE_SIZE, "off\n");

Since host_trace_buffer_enable_store() clears
ioc->diag_buffer_status[MPI2_DIAG_BUF_TYPE_TRACE] = 0 and then sleeps in
_ctl_diag_register_2() waiting for firmware command completion before setting
MPT3_DIAG_BUFFER_IS_REGISTERED, can a concurrent read of
host_trace_buffer_enable race with post/release operations and observe
transient state? Should host_trace_buffer_enable_show() also acquire
ioc->ctl_cmds.mutex?

[ ... ]

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_trigger_diag.c b/drivers/scsi/mpt3sas/mpt3sas_trigger_diag.c
> index d9b7d0ee25b0f..bda404a3ff3db 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_trigger_diag.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_trigger_diag.c
> @@ -118,6 +118,7 @@ mpt3sas_process_trigger_data(struct MPT3SAS_ADAPTER *ioc,
>  	dTriggerDiagPrintk(ioc, ioc_info(ioc, "%s: enter\n", __func__));
>  
>  	/* release the diag buffer trace */
> +	mutex_lock(&ioc->ctl_cmds.mutex);
>  	if ((ioc->diag_buffer_status[MPI2_DIAG_BUF_TYPE_TRACE] &
>  	    MPT3_DIAG_BUFFER_IS_RELEASED) == 0) {
>  		/*

[Severity: High]
Can acquiring ioc->ctl_cmds.mutex in mpt3sas_process_trigger_data() cause an
ABBA deadlock with controller reset paths?

mpt3sas_process_trigger_data() runs on the ioc->firmware_event_thread
workqueue via:

  _firmware_event_work() -> _mpt3sas_fw_work() -> mpt3sas_process_trigger_data()

Taking ioc->ctl_cmds.mutex here establishes the lock order:

  (&fw_event->work) -> (&ioc->ctl_cmds.mutex)

Conversely, several paths in drivers/scsi/mpt3sas/mpt3sas_ctl.c hold
ioc->ctl_cmds.mutex while calling mpt3sas_base_hard_reset_handler(), including
_ctl_do_reset(), _ctl_do_mpt_command(), _ctl_diag_register_2() (called from
_ctl_diag_register() and host_trace_buffer_enable_store()),
_ctl_diag_release(), _ctl_diag_read_buffer(), and
mpt3sas_send_mctp_passthru_req(). In particular, _ctl_do_mpt_command() queues
a MPT3SAS_PROCESS_TRIGGER_DIAG work item via mpt3sas_trigger_master() and can
immediately jump to issue_host_reset while still holding ioc->ctl_cmds.mutex:

	mpt3sas_trigger_master(ioc, MASTER_TRIGGER_TASK_MANAGMENT);
	...
	if (!(ioc->ctl_cmds.status & MPT3_CMD_COMPLETE)) {
		mpt3sas_check_cmd_timeout(ioc,
		    ioc->ctl_cmds.status, mpi_request,
		    karg.data_sge_offset, issue_reset);
		goto issue_host_reset;
	}

Inside mpt3sas_base_hard_reset_handler(), the reset path flushes the firmware
event queue while ioc->ctl_cmds.mutex is held:

  mpt3sas_base_hard_reset_handler()
    -> _base_clear_outstanding_commands()
      -> mpt3sas_scsih_clear_outstanding_scsi_tm_commands()
        -> _scsih_fw_event_cleanup_queue()
          -> cancel_work_sync(&fw_event->work)

This establishes the reverse lock order:

  (&ioc->ctl_cmds.mutex) -> (&fw_event->work)

If a MPT3SAS_PROCESS_TRIGGER_DIAG work item has started executing
_firmware_event_work() when _scsih_fw_event_cleanup_queue() dequeues it and
calls cancel_work_sync(&fw_event->work), won't _scsih_fw_event_cleanup_queue()
block waiting for fw_event->work to finish while
mpt3sas_process_trigger_data() blocks waiting for ioc->ctl_cmds.mutex?

> @@ -131,6 +132,7 @@ mpt3sas_process_trigger_data(struct MPT3SAS_ADAPTER *ioc,
>  		mpt3sas_send_diag_release(ioc, MPI2_DIAG_BUF_TYPE_TRACE,
>  		    &issue_reset);
>  	}
> +	mutex_unlock(&ioc->ctl_cmds.mutex);
>  
>  	ioc->htb_rel.buffer_rel_condition = MPT3_DIAG_BUFFER_REL_TRIGGER;
>  	if (event_data) {

[Severity: Medium]
Should the update to ioc->htb_rel also be protected by ioc->ctl_cmds.mutex?

Here in mpt3sas_process_trigger_data(), ioc->ctl_cmds.mutex is unlocked right
after mpt3sas_send_diag_release() sets MPT3_DIAG_BUFFER_IS_RELEASED, before
ioc->htb_rel.buffer_rel_condition, ioc->htb_rel.trigger_type, and
ioc->htb_rel.trigger_info_dwords are populated.

Other readers and writers of ioc->htb_rel in drivers/scsi/mpt3sas/mpt3sas_ctl.c
access it under ioc->ctl_cmds.mutex: _ctl_addnl_diag_query() checks
MPT3_DIAG_BUFFER_IS_RELEASED and copies ioc->htb_rel to userspace,
_ctl_diag_register_2() zeroes ioc->htb_rel with memset(), and
host_trace_buffer_enable_store() sets
ioc->htb_rel.buffer_rel_condition = MPT3_DIAG_BUFFER_REL_SYSFS.

Because the lock is dropped before ioc->htb_rel is populated, could a
concurrent _ctl_addnl_diag_query() ioctl observe MPT3_DIAG_BUFFER_IS_RELEASED
and copy incomplete or stale ioc->htb_rel data, or could a concurrent buffer
re-registration in _ctl_diag_register_2() zero ioc->htb_rel only for
mpt3sas_process_trigger_data() to overwrite it immediately afterward?

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

[PATCH 05/37] scsi: mpt3sas: Fix unlocked use of raid_device in enable_sdev_max_qd_store

[Shivasharan S]

Take ioc->raid_device_lock around the raid_device lookup and check
for NULL before dereferencing it.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index d696cb12047c..39e063d5a55c 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -4250,13 +4250,28 @@ enable_sdev_max_qd_store(struct device *cdev,
 
 			if (sas_target_priv_data->flags &
 			    MPT_TARGET_FLAGS_VOLUME) {
+				unsigned long flags;
+				u8 volume_type;
+				u32 device_info;
+
+				spin_lock_irqsave(&ioc->raid_device_lock,
+						  flags);
 				raid_device =
 				    mpt3sas_raid_device_find_by_handle(ioc,
 				    sas_target_priv_data->handle);
+				if (!raid_device) {
+					spin_unlock_irqrestore(
+					    &ioc->raid_device_lock, flags);
+					continue;
+				}
+				volume_type = raid_device->volume_type;
+				device_info = raid_device->device_info;
+				spin_unlock_irqrestore(&ioc->raid_device_lock,
+						       flags);
 
-				switch (raid_device->volume_type) {
+				switch (volume_type) {
 				case MPI2_RAID_VOL_TYPE_RAID0:
-					if (raid_device->device_info &
+					if (device_info &
 					    MPI2_SAS_DEVICE_INFO_SSP_TARGET)
 						qdepth =
 						    MPT3SAS_SAS_QUEUE_DEPTH;
-- 
2.53.0

[PATCH 06/37] scsi: mpt3sas: Fix invalid kfree() of fw_event_work event_data

[Shivasharan S]

Drop an errant kfree() of fw_event->event_data, a flexible array
member of fw_event_work rather than its own allocation.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 085d4672db85..f2a1cedcdae4 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -12270,7 +12270,6 @@ mpt3sas_scsih_event_callback(struct MPT3SAS_ADAPTER *ioc, u8 msix_index,
 		if (!fw_event->retries) {
 
 			ioc_err(ioc, "failure at %s:%d/%s()!\n",  __FILE__, __LINE__, __func__);
-			kfree(fw_event->event_data);
 			fw_event_work_put(fw_event);
 			return 1;
 		}
-- 
2.53.0

[PATCH 07/37] scsi: mpt3sas: Validate SMP request/response length in bsg handler

[Shivasharan S]

Reject SMP requests whose mapped request or response buffer is
smaller than 4 bytes, avoiding a length underflow when building the
SGE.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_transport.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c b/drivers/scsi/mpt3sas/mpt3sas_transport.c
index e74a526efa8d..abb46a5e1c1a 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c
@@ -2095,6 +2095,13 @@ _transport_smp_handler(struct bsg_job *job, struct Scsi_Host *shost,
 	if (rc)
 		goto unmap_out;
 
+	if (dma_len_out < 4 || dma_len_in < 4) {
+		ioc_err(ioc, "%s: SMP request or response too small\n",
+			__func__);
+		rc = -EINVAL;
+		goto unmap_in;
+	}
+
 	rc = mpt3sas_wait_for_ioc(ioc, IOC_OPERATIONAL_WAIT_COUNT);
 	if (rc)
 		goto unmap_in;
-- 
2.53.0

Re: [PATCH 07/37] scsi: mpt3sas: Validate SMP request/response length in bsg handler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: mpt3sas: zero-length SGE in _transport_smp_handler() for 4-byte buffers
--

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

scsi: mpt3sas: Validate SMP request/response length in bsg handler

This commit adds a length check in _transport_smp_handler() to reject SMP
requests whose mapped request or response buffer is smaller than 4 bytes,
preventing a length underflow when subtracting the 4-byte CRC.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c b/drivers/scsi/mpt3sas/mpt3sas_transport.c
> index e74a526efa8db..abb46a5e1c1af 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c
> @@ -2095,6 +2095,13 @@ _transport_smp_handler(struct bsg_job *job, struct Scsi_Host *shost,
>  	if (rc)
>  		goto unmap_out;
>  
> +	if (dma_len_out < 4 || dma_len_in < 4) {
> +		ioc_err(ioc, "%s: SMP request or response too small\n",
> +			__func__);
> +		rc = -EINVAL;
> +		goto unmap_in;
> +	}

[Severity: Medium]
What happens if dma_len_out == 4 or dma_len_in == 4?

Later in _transport_smp_handler(), 4 bytes are subtracted for the trailing
CRC when setting RequestDataLength and calling ioc->build_sg():

drivers/scsi/mpt3sas/mpt3sas_transport.c:_transport_smp_handler() {
	...
	mpi_request->RequestDataLength = cpu_to_le16(dma_len_out - 4);
	psge = &mpi_request->SGL;

	ioc->build_sg(ioc, psge, dma_addr_out, dma_len_out - 4, dma_addr_in,
			dma_len_in - 4);
	...
}

When either buffer is exactly 4 bytes, data_out_sz or data_in_sz is passed as
0 to _base_build_sg() or _base_build_sg_ieee() in
drivers/scsi/mpt3sas/mpt3sas_base.c:

	if (!data_out_sz && !data_in_sz) {
		_base_build_zero_len_sge_ieee(ioc, psge);
		return;
	}

	if (data_out_sz && data_in_sz) {
		...
	}

Because both _base_build_sg() and _base_build_sg_ieee() only construct the
two-element (outbound request + inbound response) SGL required by
MPI2_FUNCTION_SMP_PASSTHROUGH when both sizes are non-zero, a 4-byte buffer
skips the two-SGE branch and submits a single SGE to the firmware (for
example, placing the inbound response SGE into the first outbound request SGE
slot when dma_len_out == 4 and dma_len_in > 4).

Should this check reject buffers with <= 4 bytes (or < 8 bytes to account for
the 4-byte SMP header and 4-byte CRC) so that a zero-length SGE is never
passed to ioc->build_sg()?

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

[PATCH 08/37] scsi: mpt3sas: Validate firmware/user supplied device handles against MaxDevHandle

[Shivasharan S]

Firmware-supplied (and ioctl-supplied) device handles are used to index
driver bitmaps and look up devices without being checked against
ioc->facts.MaxDevHandle. Add the check in the event, discovery, post-reset
scan (including the physical disk handle) and TM/control completion paths.

MaxDevHandle is an exclusive upper bound, so also change the passthrough
handle check in _ctl_do_mpt_command() from > to >= so handles at the
boundary are rejected.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c   |  2 +-
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 74 ++++++++++++++++++++++++++--
 2 files changed, 72 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index 39e063d5a55c..a3d725e5f640 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -787,7 +787,7 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command karg,
 	    mpi_request->Function == MPI2_FUNCTION_NVME_ENCAPSULATED) {
 
 		device_handle = le16_to_cpu(mpi_request->FunctionDependent1);
-		if (!device_handle || (device_handle >
+		if (!device_handle || (device_handle >=
 		    ioc->facts.MaxDevHandle)) {
 			ret = -EINVAL;
 			mpt3sas_base_free_smid(ioc, smid);
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index f2a1cedcdae4..aaab6c4673c1 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -4399,6 +4399,12 @@ _scsih_tm_tr_send(struct MPT3SAS_ADAPTER *ioc, u16 handle)
 	u8 tr_method = 0;
 	struct hba_port *port = NULL;
 
+	if (handle >= ioc->facts.MaxDevHandle) {
+		ioc_err(ioc, "handle(0x%04x) out of bounds in %s\n",
+			handle, __func__);
+		return;
+	}
+
 	if (ioc->pci_error_recovery) {
 		dewtprintk(ioc,
 			   ioc_info(ioc, "%s: host in pci error recovery: handle(0x%04x)\n",
@@ -4659,17 +4665,25 @@ _scsih_sas_control_complete(struct MPT3SAS_ADAPTER *ioc, u16 smid,
 {
 	Mpi2SasIoUnitControlReply_t *mpi_reply =
 	    mpt3sas_base_get_reply_virt_addr(ioc, reply);
+	u16 dev_handle;
 
 	if (likely(mpi_reply)) {
+		dev_handle = le16_to_cpu(mpi_reply->DevHandle);
 		dewtprintk(ioc,
 			   ioc_info(ioc, "sc_complete:handle(0x%04x), (open) smid(%d), ioc_status(0x%04x), loginfo(0x%08x)\n",
-				    le16_to_cpu(mpi_reply->DevHandle), smid,
+				    dev_handle, smid,
 				    le16_to_cpu(mpi_reply->IOCStatus),
 				    le32_to_cpu(mpi_reply->IOCLogInfo)));
 		if (le16_to_cpu(mpi_reply->IOCStatus) ==
 		     MPI2_IOCSTATUS_SUCCESS) {
-			clear_bit(le16_to_cpu(mpi_reply->DevHandle),
-			    ioc->device_remove_in_progress);
+			if (dev_handle >= ioc->facts.MaxDevHandle) {
+				ioc_err(ioc,
+					"handle(0x%04x) out of bounds in %s\n",
+					dev_handle, __func__);
+			} else {
+				clear_bit(dev_handle,
+					  ioc->device_remove_in_progress);
+			}
 		}
 	} else {
 		ioc_err(ioc, "mpi_reply not valid at %s:%d/%s()!\n",
@@ -5220,6 +5234,11 @@ _scsih_check_ir_config_unhide_events(struct MPT3SAS_ADAPTER *ioc,
 		if (element->ReasonCode != MPI2_EVENT_IR_CHANGE_RC_UNHIDE)
 			continue;
 		handle = le16_to_cpu(element->PhysDiskDevHandle);
+		if (handle >= ioc->facts.MaxDevHandle) {
+			ioc_err(ioc, "handle(0x%04x) out of bounds in %s\n",
+				handle, __func__);
+			continue;
+		}
 		volume_handle = le16_to_cpu(element->VolDevHandle);
 		clear_bit(handle, ioc->pd_handles);
 		if (!volume_handle)
@@ -8397,6 +8416,12 @@ _scsih_add_device(struct MPT3SAS_ADAPTER *ioc, u16 handle, u8 retry_count,
 	u8 connector_name[5];
 	u8 port_id;
 
+	if (handle >= ioc->facts.MaxDevHandle) {
+		ioc_err(ioc, "handle(0x%04x) out of bounds in %s\n",
+			handle, __func__);
+		return -1;
+	}
+
 	if ((mpt3sas_config_get_sas_device_pg0(ioc, &mpi_reply, &sas_device_pg0,
 	    MPI2_SAS_DEVICE_PGAD_FORM_HANDLE, handle))) {
 		ioc_err(ioc, "failure at %s:%d/%s()!\n",
@@ -9184,6 +9209,12 @@ _scsih_pcie_check_device(struct MPT3SAS_ADAPTER *ioc, u16 handle)
 	struct MPT3SAS_TARGET *sas_target_priv_data;
 	u32 device_info;
 
+	if (handle >= ioc->facts.MaxDevHandle) {
+		ioc_err(ioc, "handle(0x%04x) out of bounds in %s\n",
+			handle, __func__);
+		return;
+	}
+
 	if ((mpt3sas_config_get_pcie_device_pg0(ioc, &mpi_reply,
 		&pcie_device_pg0, MPI26_PCIE_DEVICE_PGAD_FORM_HANDLE, handle)))
 		return;
@@ -10450,6 +10481,12 @@ _scsih_sas_pd_add(struct MPT3SAS_ADAPTER *ioc,
 	u64 sas_address;
 	u16 parent_handle;
 
+	if (handle >= ioc->facts.MaxDevHandle) {
+		ioc_err(ioc, "handle(0x%04x) out of bounds in %s\n",
+			handle, __func__);
+		return;
+	}
+
 	set_bit(handle, ioc->pd_handles);
 
 	sas_device = mpt3sas_get_sdev_by_handle(ioc, handle);
@@ -10741,6 +10778,11 @@ _scsih_sas_ir_physical_disk_event(struct MPT3SAS_ADAPTER *ioc,
 		return;
 
 	handle = le16_to_cpu(event_data->PhysDiskDevHandle);
+	if (handle >= ioc->facts.MaxDevHandle) {
+		ioc_err(ioc, "handle(0x%04x) out of bounds in %s\n",
+			handle, __func__);
+		return;
+	}
 	state = le32_to_cpu(event_data->NewValue);
 
 	if (!ioc->hide_ir_msg)
@@ -11104,6 +11146,11 @@ _scsih_search_responding_sas_devices(struct MPT3SAS_ADAPTER *ioc)
 		if (ioc_status != MPI2_IOCSTATUS_SUCCESS)
 			break;
 		handle = le16_to_cpu(sas_device_pg0.DevHandle);
+		if (handle >= ioc->facts.MaxDevHandle) {
+			ioc_err(ioc, "handle(0x%04x) out of bounds in %s\n",
+				handle, __func__);
+			continue;
+		}
 		device_info = le32_to_cpu(sas_device_pg0.DeviceInfo);
 		if (!(_scsih_is_end_device(device_info)))
 			continue;
@@ -11224,6 +11271,11 @@ _scsih_search_responding_pcie_devices(struct MPT3SAS_ADAPTER *ioc)
 			break;
 		}
 		handle = le16_to_cpu(pcie_device_pg0.DevHandle);
+		if (handle >= ioc->facts.MaxDevHandle) {
+			ioc_err(ioc, "handle(0x%04x) out of bounds in %s\n",
+				handle, __func__);
+			continue;
+		}
 		device_info = le32_to_cpu(pcie_device_pg0.DeviceInfo);
 		if (!(_scsih_is_nvme_pciescsi_device(device_info)))
 			continue;
@@ -11324,6 +11376,11 @@ _scsih_search_responding_raid_devices(struct MPT3SAS_ADAPTER *ioc)
 		if (ioc_status != MPI2_IOCSTATUS_SUCCESS)
 			break;
 		handle = le16_to_cpu(volume_pg1.DevHandle);
+		if (handle >= ioc->facts.MaxDevHandle) {
+			ioc_err(ioc, "handle(0x%04x) out of bounds in %s\n",
+				handle, __func__);
+			continue;
+		}
 
 		if (mpt3sas_config_get_raid_volume_pg0(ioc, &mpi_reply,
 		    &volume_pg0, MPI2_RAID_VOLUME_PGAD_FORM_HANDLE, handle,
@@ -11350,6 +11407,11 @@ _scsih_search_responding_raid_devices(struct MPT3SAS_ADAPTER *ioc)
 				break;
 			phys_disk_num = pd_pg0.PhysDiskNum;
 			handle = le16_to_cpu(pd_pg0.DevHandle);
+			if (handle >= ioc->facts.MaxDevHandle) {
+				ioc_err(ioc, "handle(0x%04x) out of bounds in %s\n",
+					handle, __func__);
+				continue;
+			}
 			set_bit(handle, ioc->pd_handles);
 		}
 	}
@@ -11685,6 +11747,12 @@ _scsih_scan_for_devices_after_reset(struct MPT3SAS_ADAPTER *ioc)
 		}
 		phys_disk_num = pd_pg0.PhysDiskNum;
 		handle = le16_to_cpu(pd_pg0.DevHandle);
+		if (handle >= ioc->facts.MaxDevHandle) {
+			ioc_err(ioc,
+				"handle(0x%04x) out of bounds at %s:%d/%s()!\n",
+				handle, __FILE__, __LINE__, __func__);
+			continue;
+		}
 		sas_device = mpt3sas_get_sdev_by_handle(ioc, handle);
 		if (sas_device) {
 			sas_device_put(sas_device);
-- 
2.53.0

Re: [PATCH 08/37] scsi: mpt3sas: Validate firmware/user supplied device handles against MaxDevHandle

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: mpt3sas: infinite retry loop when _scsih_add_device() returns -1
--

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

scsi: mpt3sas: Validate firmware/user supplied device handles against MaxDevHandle

This commit validates firmware-supplied and ioctl-supplied device handles
against ioc->facts.MaxDevHandle across event handling, device discovery,
post-reset scanning, and task management/control completion paths. It also
changes the passthrough handle boundary check in _ctl_do_mpt_command() from
> to >=.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index 8dcbe60db542e..a697200844078 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c

[ ... ]

> @@ -8399,6 +8418,12 @@ _scsih_add_device(struct MPT3SAS_ADAPTER *ioc, u16 handle, u8 retry_count,
>  	u8 connector_name[5];
>  	u8 port_id;
>  
> +	if (handle >= ioc->facts.MaxDevHandle) {
> +		ioc_err(ioc, "handle(0x%04x) out of bounds in %s\n",
> +			handle, __func__);
> +		return -1;
> +	}

[Severity: High]
Does returning -1 here cause an infinite retry loop in callers that treat
any non-zero return from _scsih_add_device() as a retryable condition?

In _scsih_scan_for_devices_after_reset(), the SAS end-device scan loop
reads handle = le16_to_cpu(sas_device_pg0.DevHandle) without checking it
against ioc->facts.MaxDevHandle before calling _scsih_add_device():

	retry_count = 0;
	/* This will retry adding the end device.
	 * _scsih_add_device() will decide on retries and
	 * return "1" when it should be retried
	 */
	while (_scsih_add_device(ioc, handle, retry_count++,
	    0)) {
		ssleep(1);
	}

Similarly, _scsih_sas_topology_change_event() checks the return value when
handling MPI2_EVENT_SAS_TOPO_RC_TARG_ADDED:

	rc = _scsih_add_device(ioc, handle,
	    fw_event->retries[i], 0);
	if (rc) {/* retry due to busy device */
		fw_event->retries[i]++;
		requeue_event = 1;
	} else {/* mark entry vacant */
		event_data->PHY[i].PhyStatus |=
	    MPI2_EVENT_SAS_TOPO_PHYSTATUS_VACANT;
	}

Because _scsih_add_device() returns -1 at the start of the function before
retry_count is checked in _scsih_wait_for_target_to_become_ready(),
wouldn't an out-of-bounds handle cause _scsih_scan_for_devices_after_reset()
to loop forever holding ioc->reset_in_progress_mutex, and cause
_scsih_sas_topology_change_event() to requeue fw_event indefinitely?

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

[PATCH 09/37] scsi: mpt3sas: Clamp topology/IR event entry counts to EventDataLength

[Shivasharan S]

Clamp NumEntries/NumElements in SAS/PCIe topology and IR config
change events to what EventDataLength actually covers.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 51 ++++++++++++++++++++++++----
 1 file changed, 45 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index aaab6c4673c1..1d483a560d75 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -12210,9 +12210,21 @@ mpt3sas_scsih_event_callback(struct MPT3SAS_ADAPTER *ioc, u8 msix_index,
 	}
 
 	case MPI2_EVENT_SAS_TOPOLOGY_CHANGE_LIST:
-		_scsih_check_topo_delete_events(ioc,
+	{
+		Mpi2EventDataSasTopologyChangeList_t *topo_data =
 		    (Mpi2EventDataSasTopologyChangeList_t *)
-		    mpi_reply->EventData);
+		    mpi_reply->EventData;
+		u32 data_len = le16_to_cpu(mpi_reply->EventDataLength) * 4;
+		size_t hdr_size =
+		    offsetof(Mpi2EventDataSasTopologyChangeList_t, PHY);
+		u16 max_entries = 0;
+
+		if (data_len > hdr_size)
+			max_entries = (data_len - hdr_size) /
+			    sizeof(Mpi2EventSasTopoPhyEntry_t);
+		if (topo_data->NumEntries > max_entries)
+			topo_data->NumEntries = max_entries;
+		_scsih_check_topo_delete_events(ioc, topo_data);
 		/*
 		 * No need to add the topology change list
 		 * event to fw event work queue when
@@ -12224,18 +12236,45 @@ mpt3sas_scsih_event_callback(struct MPT3SAS_ADAPTER *ioc, u8 msix_index,
 		if (ioc->shost_recovery)
 			return 1;
 		break;
+	}
 	case MPI2_EVENT_PCIE_TOPOLOGY_CHANGE_LIST:
-	_scsih_check_pcie_topo_remove_events(ioc,
+	{
+		Mpi26EventDataPCIeTopologyChangeList_t *pcie_topo_data =
 		    (Mpi26EventDataPCIeTopologyChangeList_t *)
-		    mpi_reply->EventData);
+		    mpi_reply->EventData;
+		u32 data_len = le16_to_cpu(mpi_reply->EventDataLength) * 4;
+		size_t hdr_size =
+		    offsetof(Mpi26EventDataPCIeTopologyChangeList_t, PortEntry);
+		u16 max_entries = 0;
+
+		if (data_len > hdr_size)
+			max_entries = (data_len - hdr_size) /
+			    sizeof(MPI26_EVENT_PCIE_TOPO_PORT_ENTRY);
+		if (pcie_topo_data->NumEntries > max_entries)
+			pcie_topo_data->NumEntries = max_entries;
+		_scsih_check_pcie_topo_remove_events(ioc, pcie_topo_data);
 		if (ioc->shost_recovery)
 			return 1;
 		break;
+	}
 	case MPI2_EVENT_IR_CONFIGURATION_CHANGE_LIST:
-		_scsih_check_ir_config_unhide_events(ioc,
+	{
+		Mpi2EventDataIrConfigChangeList_t *ir_config_data =
 		    (Mpi2EventDataIrConfigChangeList_t *)
-		    mpi_reply->EventData);
+		    mpi_reply->EventData;
+		u32 data_len = le16_to_cpu(mpi_reply->EventDataLength) * 4;
+		size_t hdr_size =
+		    offsetof(Mpi2EventDataIrConfigChangeList_t, ConfigElement);
+		u16 max_elements = 0;
+
+		if (data_len > hdr_size)
+			max_elements = (data_len - hdr_size) /
+			    sizeof(MPI2_EVENT_IR_CONFIG_ELEMENT);
+		if (ir_config_data->NumElements > max_elements)
+			ir_config_data->NumElements = max_elements;
+		_scsih_check_ir_config_unhide_events(ioc, ir_config_data);
 		break;
+	}
 	case MPI2_EVENT_IR_VOLUME:
 		_scsih_check_volume_delete_events(ioc,
 		    (Mpi2EventDataIrVolume_t *)
-- 
2.53.0

Re: [PATCH 09/37] scsi: mpt3sas: Clamp topology/IR event entry counts to EventDataLength

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: mpt3sas: u16 sz truncation bypasses u32 data_len clamp in mpt3sas_scsih_event_callback()
- [High] scsi: mpt3sas: out-of-bounds access when EventDataLength is smaller than hdr_size
--

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

scsi: mpt3sas: Clamp topology/IR event entry counts to EventDataLength

This commit clamps NumEntries and NumElements in SAS topology, PCIe topology,
and IR configuration change events to the maximum number of entries that fit
within EventDataLength.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index a697200844078..4730e2d344f00 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -12212,9 +12212,21 @@ mpt3sas_scsih_event_callback(struct MPT3SAS_ADAPTER *ioc, u8 msix_index,
>  	}
>  
>  	case MPI2_EVENT_SAS_TOPOLOGY_CHANGE_LIST:
> -		_scsih_check_topo_delete_events(ioc,
> +	{
> +		Mpi2EventDataSasTopologyChangeList_t *topo_data =
>  		    (Mpi2EventDataSasTopologyChangeList_t *)
> -		    mpi_reply->EventData);
> +		    mpi_reply->EventData;
> +		u32 data_len = le16_to_cpu(mpi_reply->EventDataLength) * 4;

[Severity: High]
Can a 16-bit truncation of sz later in mpt3sas_scsih_event_callback() bypass
this u32 data_len clamp and still overflow fw_event->event_data[]?

Here and in the MPI2_EVENT_PCIE_TOPOLOGY_CHANGE_LIST and
MPI2_EVENT_IR_CONFIGURATION_CHANGE_LIST cases, data_len is computed as a u32,
whereas sz used for allocating and populating fw_event->event_data is declared
as a u16 in mpt3sas_scsih_event_callback():

	u16 event;
	u16 sz;
	...
	sz = le16_to_cpu(mpi_reply->EventDataLength) * 4;
	fw_event = alloc_fw_event_work(sz);
	...
	memcpy(fw_event->event_data, mpi_reply->EventData, sz);

If EventDataLength is >= 0x4000 (for example, 0x4003), data_len is 65548, so
max_entries is 8192 and an 8-bit NumEntries or NumElements value (up to 255)
is not clamped. However, sz truncates modulo 65536 to 12 bytes, so
alloc_fw_event_work(sz) allocates only 12 bytes for fw_event->event_data and
copies the header containing the unclamped NumEntries or NumElements. When
_mpt3sas_fw_work() later runs _scsih_sas_topology_change_event(),
_scsih_pcie_topology_change_event(), or _scsih_sas_ir_config_change_event(),
won't the loops over PHY[], PortEntry[], or ConfigElement[] read and write
past the end of fw_event->event_data?

> +		size_t hdr_size =
> +		    offsetof(Mpi2EventDataSasTopologyChangeList_t, PHY);
> +		u16 max_entries = 0;
> +
> +		if (data_len > hdr_size)
> +			max_entries = (data_len - hdr_size) /
> +			    sizeof(Mpi2EventSasTopoPhyEntry_t);
> +		if (topo_data->NumEntries > max_entries)
> +			topo_data->NumEntries = max_entries;
> +		_scsih_check_topo_delete_events(ioc, topo_data);

[Severity: High]
What happens if EventDataLength is smaller than hdr_size (for example,
EventDataLength is 0 or 1, so data_len < 12 for SAS/PCIe topology events or
data_len < 8 for IR configuration change events)?

When data_len < hdr_size, max_entries (or max_elements) is 0, but the event is
not rejected before calling _scsih_check_topo_delete_events(),
_scsih_check_pcie_topo_remove_events(), or
_scsih_check_ir_config_unhide_events(), and execution continues to allocate
and queue fw_event with sz = data_len:

	sz = le16_to_cpu(mpi_reply->EventDataLength) * 4;
	fw_event = alloc_fw_event_work(sz);
	...
	memcpy(fw_event->event_data, mpi_reply->EventData, sz);

Because memcpy() only copies sz bytes into fw_event->event_data, fixed header
fields such as ExpStatus/SwitchStatus (at offset 10), NumEntries (at offset 8),
or Flags (at offset 4) lie beyond the copied payload and, when sz is 0 or 4,
beyond the end of the fw_event slab allocation. When
_scsih_sas_topology_change_event(), _scsih_pcie_topology_change_event(), or
_scsih_sas_ir_config_change_event() (as well as the fw_event_list walks in
_scsih_check_topo_delete_events() and _scsih_check_pcie_topo_remove_events())
access those header fields in fw_event->event_data, can this read out of
bounds past fw_event->event_data?

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

[PATCH 10/37] scsi: mpt3sas: NULL ioc->reply_post after freeing it

[Shivasharan S]

Match the pattern used for other pool pointers in
_base_release_memory_pools() so a repeat call can't reuse it.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 9bf92c4a14b7..40624b3c972c 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -5869,6 +5869,7 @@ _base_release_memory_pools(struct MPT3SAS_ADAPTER *ioc)
 		}
 		dma_pool_destroy(ioc->reply_post_free_array_dma_pool);
 		kfree(ioc->reply_post);
+		ioc->reply_post = NULL;
 	}
 
 	if (ioc->pcie_sgl_dma_pool) {
-- 
2.53.0

[PATCH 11/37] scsi: mpt3sas: Fix port_table memory leak in _scsih_sas_port_refresh

[Shivasharan S]

Free port_table on every exit path, including the early return when
no ports are found.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 1d483a560d75..788b611c7c52 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -6783,7 +6783,7 @@ _scsih_sas_port_refresh(struct MPT3SAS_ADAPTER *ioc)
 
 	port_count = _scsih_get_port_table_after_reset(ioc, port_table);
 	if (!port_count)
-		return;
+		goto out;
 
 	drsprintk(ioc, ioc_info(ioc, "New Port table\n"));
 	for (j = 0; j < port_count; j++)
@@ -6846,6 +6846,8 @@ _scsih_sas_port_refresh(struct MPT3SAS_ADAPTER *ioc)
 	}
 
 	port_table_entry = NULL;
+out:
+	kfree(port_table);
 }
 
 /**
-- 
2.53.0

[PATCH 12/37] scsi: mpt3sas: Fix leak and dangling pointer for io_uring_poll_queues

[Shivasharan S]

Free ioc->io_uring_poll_queues on the legacy-IRQ fallback path and
NULL it after freeing.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 40624b3c972c..060482ba20ae 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -3362,6 +3362,7 @@ mpt3sas_base_disable_msix(struct MPT3SAS_ADAPTER *ioc)
 	pci_free_irq_vectors(ioc->pdev);
 	ioc->msix_enable = 0;
 	kfree(ioc->io_uring_poll_queues);
+	ioc->io_uring_poll_queues = NULL;
 }
 
 /**
@@ -3541,6 +3542,8 @@ _base_enable_msix(struct MPT3SAS_ADAPTER *ioc)
 
 /* failback to io_apic interrupt routing */
  try_ioapic:
+	kfree(ioc->io_uring_poll_queues);
+	ioc->io_uring_poll_queues = NULL;
 	ioc->high_iops_queues = 0;
 	ioc_info(ioc, "High IOPs queues : disabled\n");
 	ioc->reply_queue_count = 1;
-- 
2.53.0

[PATCH 13/37] scsi: mpt3sas: Validate max_data_size in MPT3EVENTREPORT ioctl

[Shivasharan S]

Reject a max_data_size smaller than the ioctl header size before
using it in size arithmetic.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index a3d725e5f640..4866b3c7d1c1 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -1367,6 +1367,9 @@ _ctl_eventreport(struct MPT3SAS_ADAPTER *ioc, void __user *arg)
 	dctlprintk(ioc, ioc_info(ioc, "%s: enter\n",
 				 __func__));
 
+	if (karg.hdr.max_data_size < sizeof(struct mpt3_ioctl_header))
+		return -EINVAL;
+
 	number_bytes = karg.hdr.max_data_size -
 	    sizeof(struct mpt3_ioctl_header);
 	max_events = number_bytes/sizeof(struct MPT3_IOCTL_EVENTS);
-- 
2.53.0

[PATCH 14/37] scsi: mpt3sas: Fix buffer overflow in _scsih_scsi_ioc_info

[Shivasharan S]

Build the SCSIState description string with a bounds-checked
snprintf() instead of a chain of unbounded strcat()s.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 788b611c7c52..47bc48f7b9a3 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -5777,19 +5777,20 @@ _scsih_scsi_ioc_info(struct MPT3SAS_ADAPTER *ioc, struct scsi_cmnd *scmd,
 		break;
 	}
 
-	desc_scsi_state[0] = '\0';
 	if (!scsi_state)
 		desc_scsi_state = " ";
-	if (scsi_state & MPI2_SCSI_STATE_RESPONSE_INFO_VALID)
-		strcat(desc_scsi_state, "response info ");
-	if (scsi_state & MPI2_SCSI_STATE_TERMINATED)
-		strcat(desc_scsi_state, "state terminated ");
-	if (scsi_state & MPI2_SCSI_STATE_NO_SCSI_STATUS)
-		strcat(desc_scsi_state, "no status ");
-	if (scsi_state & MPI2_SCSI_STATE_AUTOSENSE_FAILED)
-		strcat(desc_scsi_state, "autosense failed ");
-	if (scsi_state & MPI2_SCSI_STATE_AUTOSENSE_VALID)
-		strcat(desc_scsi_state, "autosense valid ");
+	else
+		snprintf(desc_scsi_state, MPT_STRING_LENGTH, "%s%s%s%s%s",
+			 (scsi_state & MPI2_SCSI_STATE_RESPONSE_INFO_VALID) ?
+			 "response info " : "",
+			 (scsi_state & MPI2_SCSI_STATE_TERMINATED) ?
+			 "state terminated " : "",
+			 (scsi_state & MPI2_SCSI_STATE_NO_SCSI_STATUS) ?
+			 "no status " : "",
+			 (scsi_state & MPI2_SCSI_STATE_AUTOSENSE_FAILED) ?
+			 "autosense failed " : "",
+			 (scsi_state & MPI2_SCSI_STATE_AUTOSENSE_VALID) ?
+			 "autosense valid " : "");
 
 	scsi_print_command(scmd);
 
-- 
2.53.0

Re: [PATCH 14/37] scsi: mpt3sas: Fix buffer overflow in _scsih_scsi_ioc_info

[kernel test robot]

Hi Shivasharan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jejb-scsi/for-next]
[also build test WARNING on mkp-scsi/for-next linus/master v7.4-rc1 next-20261105]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Shivasharan-S/scsi-mpt3sas-Validate-firmware-reported-IOCRequestFrameSize/20261105-164216
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
patch link:    https://lore.kernel.org/r/1793925772-1303-15-git-send-email-shivasharan.srikanteshwara%40broadcom.com
patch subject: [PATCH 14/37] scsi: mpt3sas: Fix buffer overflow in _scsih_scsi_ioc_info
config: arm-randconfig-1000-20261106 (https://download.01.org/0day-ci/archive/20261106/202611061833.3AceeVtV-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 10.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20261106/202611061833.3AceeVtV-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202611061833.3AceeVtV-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/scsi/mpt3sas/mpt3sas_scsih.c: In function '_scsih_scsi_ioc_info.constprop':
>> drivers/scsi/mpt3sas/mpt3sas_scsih.c:5783:57: warning: '%s' directive output may be truncated writing 16 bytes into a region of size between 6 and 64 [-Wformat-truncation=]
    5783 |   snprintf(desc_scsi_state, MPT_STRING_LENGTH, "%s%s%s%s%s",
         |                                                         ^~
   ......
    5793 |     "autosense valid " : "");
         |     ~~~~~~~~~~~~~~~~~~                                   
   drivers/scsi/mpt3sas/mpt3sas_scsih.c:5783:3: note: 'snprintf' output between 17 and 75 bytes into a destination of size 64
    5783 |   snprintf(desc_scsi_state, MPT_STRING_LENGTH, "%s%s%s%s%s",
         |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    5784 |     (scsi_state & MPI2_SCSI_STATE_RESPONSE_INFO_VALID) ?
         |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    5785 |     "response info " : "",
         |     ~~~~~~~~~~~~~~~~~~~~~~
    5786 |     (scsi_state & MPI2_SCSI_STATE_TERMINATED) ?
         |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    5787 |     "state terminated " : "",
         |     ~~~~~~~~~~~~~~~~~~~~~~~~~
    5788 |     (scsi_state & MPI2_SCSI_STATE_NO_SCSI_STATUS) ?
         |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    5789 |     "no status " : "",
         |     ~~~~~~~~~~~~~~~~~~
    5790 |     (scsi_state & MPI2_SCSI_STATE_AUTOSENSE_FAILED) ?
         |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    5791 |     "autosense failed " : "",
         |     ~~~~~~~~~~~~~~~~~~~~~~~~~
    5792 |     (scsi_state & MPI2_SCSI_STATE_AUTOSENSE_VALID) ?
         |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    5793 |     "autosense valid " : "");
         |     ~~~~~~~~~~~~~~~~~~~~~~~~


vim +5783 drivers/scsi/mpt3sas/mpt3sas_scsih.c

  5639	
  5640	/**
  5641	 * _scsih_scsi_ioc_info - translated non-successful SCSI_IO request
  5642	 * @ioc: per adapter object
  5643	 * @scmd: pointer to scsi command object
  5644	 * @mpi_reply: reply mf payload returned from firmware
  5645	 * @smid: ?
  5646	 *
  5647	 * scsi_status - SCSI Status code returned from target device
  5648	 * scsi_state - state info associated with SCSI_IO determined by ioc
  5649	 * ioc_status - ioc supplied status info
  5650	 */
  5651	static void
  5652	_scsih_scsi_ioc_info(struct MPT3SAS_ADAPTER *ioc, struct scsi_cmnd *scmd,
  5653		Mpi2SCSIIOReply_t *mpi_reply, u16 smid)
  5654	{
  5655		u32 response_info;
  5656		u8 *response_bytes;
  5657		u16 ioc_status = le16_to_cpu(mpi_reply->IOCStatus) &
  5658		    MPI2_IOCSTATUS_MASK;
  5659		u8 scsi_state = mpi_reply->SCSIState;
  5660		u8 scsi_status = mpi_reply->SCSIStatus;
  5661		char *desc_ioc_state = NULL;
  5662		char *desc_scsi_status = NULL;
  5663		char *desc_scsi_state = ioc->tmp_string;
  5664		u32 log_info = le32_to_cpu(mpi_reply->IOCLogInfo);
  5665		struct _sas_device *sas_device = NULL;
  5666		struct _pcie_device *pcie_device = NULL;
  5667		struct scsi_target *starget = scmd->device->sdev_target;
  5668		struct MPT3SAS_TARGET *priv_target = starget->hostdata;
  5669		char *device_str = NULL;
  5670	
  5671		if (!priv_target)
  5672			return;
  5673		if (ioc->hide_ir_msg)
  5674			device_str = "WarpDrive";
  5675		else
  5676			device_str = "volume";
  5677	
  5678		if (log_info == 0x31170000)
  5679			return;
  5680	
  5681		switch (ioc_status) {
  5682		case MPI2_IOCSTATUS_SUCCESS:
  5683			desc_ioc_state = "success";
  5684			break;
  5685		case MPI2_IOCSTATUS_INVALID_FUNCTION:
  5686			desc_ioc_state = "invalid function";
  5687			break;
  5688		case MPI2_IOCSTATUS_SCSI_RECOVERED_ERROR:
  5689			desc_ioc_state = "scsi recovered error";
  5690			break;
  5691		case MPI2_IOCSTATUS_SCSI_INVALID_DEVHANDLE:
  5692			desc_ioc_state = "scsi invalid dev handle";
  5693			break;
  5694		case MPI2_IOCSTATUS_SCSI_DEVICE_NOT_THERE:
  5695			desc_ioc_state = "scsi device not there";
  5696			break;
  5697		case MPI2_IOCSTATUS_SCSI_DATA_OVERRUN:
  5698			desc_ioc_state = "scsi data overrun";
  5699			break;
  5700		case MPI2_IOCSTATUS_SCSI_DATA_UNDERRUN:
  5701			desc_ioc_state = "scsi data underrun";
  5702			break;
  5703		case MPI2_IOCSTATUS_SCSI_IO_DATA_ERROR:
  5704			desc_ioc_state = "scsi io data error";
  5705			break;
  5706		case MPI2_IOCSTATUS_SCSI_PROTOCOL_ERROR:
  5707			desc_ioc_state = "scsi protocol error";
  5708			break;
  5709		case MPI2_IOCSTATUS_SCSI_TASK_TERMINATED:
  5710			desc_ioc_state = "scsi task terminated";
  5711			break;
  5712		case MPI2_IOCSTATUS_SCSI_RESIDUAL_MISMATCH:
  5713			desc_ioc_state = "scsi residual mismatch";
  5714			break;
  5715		case MPI2_IOCSTATUS_SCSI_TASK_MGMT_FAILED:
  5716			desc_ioc_state = "scsi task mgmt failed";
  5717			break;
  5718		case MPI2_IOCSTATUS_SCSI_IOC_TERMINATED:
  5719			desc_ioc_state = "scsi ioc terminated";
  5720			break;
  5721		case MPI2_IOCSTATUS_SCSI_EXT_TERMINATED:
  5722			desc_ioc_state = "scsi ext terminated";
  5723			break;
  5724		case MPI2_IOCSTATUS_EEDP_GUARD_ERROR:
  5725			desc_ioc_state = "eedp guard error";
  5726			break;
  5727		case MPI2_IOCSTATUS_EEDP_REF_TAG_ERROR:
  5728			desc_ioc_state = "eedp ref tag error";
  5729			break;
  5730		case MPI2_IOCSTATUS_EEDP_APP_TAG_ERROR:
  5731			desc_ioc_state = "eedp app tag error";
  5732			break;
  5733		case MPI2_IOCSTATUS_INSUFFICIENT_POWER:
  5734			desc_ioc_state = "insufficient power";
  5735			break;
  5736		default:
  5737			desc_ioc_state = "unknown";
  5738			break;
  5739		}
  5740	
  5741		switch (scsi_status) {
  5742		case MPI2_SCSI_STATUS_GOOD:
  5743			desc_scsi_status = "good";
  5744			break;
  5745		case MPI2_SCSI_STATUS_CHECK_CONDITION:
  5746			desc_scsi_status = "check condition";
  5747			break;
  5748		case MPI2_SCSI_STATUS_CONDITION_MET:
  5749			desc_scsi_status = "condition met";
  5750			break;
  5751		case MPI2_SCSI_STATUS_BUSY:
  5752			desc_scsi_status = "busy";
  5753			break;
  5754		case MPI2_SCSI_STATUS_INTERMEDIATE:
  5755			desc_scsi_status = "intermediate";
  5756			break;
  5757		case MPI2_SCSI_STATUS_INTERMEDIATE_CONDMET:
  5758			desc_scsi_status = "intermediate condmet";
  5759			break;
  5760		case MPI2_SCSI_STATUS_RESERVATION_CONFLICT:
  5761			desc_scsi_status = "reservation conflict";
  5762			break;
  5763		case MPI2_SCSI_STATUS_COMMAND_TERMINATED:
  5764			desc_scsi_status = "command terminated";
  5765			break;
  5766		case MPI2_SCSI_STATUS_TASK_SET_FULL:
  5767			desc_scsi_status = "task set full";
  5768			break;
  5769		case MPI2_SCSI_STATUS_ACA_ACTIVE:
  5770			desc_scsi_status = "aca active";
  5771			break;
  5772		case MPI2_SCSI_STATUS_TASK_ABORTED:
  5773			desc_scsi_status = "task aborted";
  5774			break;
  5775		default:
  5776			desc_scsi_status = "unknown";
  5777			break;
  5778		}
  5779	
  5780		if (!scsi_state)
  5781			desc_scsi_state = " ";
  5782		else
> 5783			snprintf(desc_scsi_state, MPT_STRING_LENGTH, "%s%s%s%s%s",
  5784				 (scsi_state & MPI2_SCSI_STATE_RESPONSE_INFO_VALID) ?
  5785				 "response info " : "",
  5786				 (scsi_state & MPI2_SCSI_STATE_TERMINATED) ?
  5787				 "state terminated " : "",
  5788				 (scsi_state & MPI2_SCSI_STATE_NO_SCSI_STATUS) ?
  5789				 "no status " : "",
  5790				 (scsi_state & MPI2_SCSI_STATE_AUTOSENSE_FAILED) ?
  5791				 "autosense failed " : "",
  5792				 (scsi_state & MPI2_SCSI_STATE_AUTOSENSE_VALID) ?
  5793				 "autosense valid " : "");
  5794	
  5795		scsi_print_command(scmd);
  5796	
  5797		if (priv_target->flags & MPT_TARGET_FLAGS_VOLUME) {
  5798			ioc_warn(ioc, "\t%s wwid(0x%016llx)\n",
  5799				 device_str, (u64)priv_target->sas_address);
  5800		} else if (priv_target->flags & MPT_TARGET_FLAGS_PCIE_DEVICE) {
  5801			pcie_device = mpt3sas_get_pdev_from_target(ioc, priv_target);
  5802			if (pcie_device) {
  5803				ioc_info(ioc, "\twwid(0x%016llx), port(%d)\n",
  5804					 (u64)pcie_device->wwid, pcie_device->port_num);
  5805				if (pcie_device->enclosure_handle != 0)
  5806					ioc_info(ioc, "\tenclosure logical id(0x%016llx), slot(%d)\n",
  5807						 (u64)pcie_device->enclosure_logical_id,
  5808						 pcie_device->slot);
  5809				if (pcie_device->connector_name[0])
  5810					ioc_info(ioc, "\tenclosure level(0x%04x), connector name( %s)\n",
  5811						 pcie_device->enclosure_level,
  5812						 pcie_device->connector_name);
  5813				pcie_device_put(pcie_device);
  5814			}
  5815		} else {
  5816			sas_device = mpt3sas_get_sdev_from_target(ioc, priv_target);
  5817			if (sas_device) {
  5818				ioc_warn(ioc, "\tsas_address(0x%016llx), phy(%d)\n",
  5819					 (u64)sas_device->sas_address, sas_device->phy);
  5820	
  5821				_scsih_display_enclosure_chassis_info(ioc, sas_device,
  5822				    NULL, NULL);
  5823	
  5824				sas_device_put(sas_device);
  5825			}
  5826		}
  5827	
  5828		ioc_warn(ioc, "\thandle(0x%04x), ioc_status(%s)(0x%04x), smid(%d)\n",
  5829			 le16_to_cpu(mpi_reply->DevHandle),
  5830			 desc_ioc_state, ioc_status, smid);
  5831		ioc_warn(ioc, "\trequest_len(%d), underflow(%d), resid(%d)\n",
  5832			 scsi_bufflen(scmd), scmd->underflow, scsi_get_resid(scmd));
  5833		ioc_warn(ioc, "\ttag(%d), transfer_count(%d), sc->result(0x%08x)\n",
  5834			 le16_to_cpu(mpi_reply->TaskTag),
  5835			 le32_to_cpu(mpi_reply->TransferCount), scmd->result);
  5836		ioc_warn(ioc, "\tscsi_status(%s)(0x%02x), scsi_state(%s)(0x%02x)\n",
  5837			 desc_scsi_status, scsi_status, desc_scsi_state, scsi_state);
  5838	
  5839		if (scsi_state & MPI2_SCSI_STATE_AUTOSENSE_VALID) {
  5840			struct sense_info data;
  5841			_scsih_normalize_sense(scmd->sense_buffer, &data);
  5842			ioc_warn(ioc, "\t[sense_key,asc,ascq]: [0x%02x,0x%02x,0x%02x], count(%d)\n",
  5843				 data.skey, data.asc, data.ascq,
  5844				 le32_to_cpu(mpi_reply->SenseCount));
  5845		}
  5846		if (scsi_state & MPI2_SCSI_STATE_RESPONSE_INFO_VALID) {
  5847			response_info = le32_to_cpu(mpi_reply->ResponseInfo);
  5848			response_bytes = (u8 *)&response_info;
  5849			_scsih_response_code(ioc, response_bytes[0]);
  5850		}
  5851	}
  5852	

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 14/37] scsi: mpt3sas: Fix buffer overflow in _scsih_scsi_ioc_info

[kernel test robot]

Hi Shivasharan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jejb-scsi/for-next]
[also build test WARNING on mkp-scsi/for-next linus/master v7.4-rc1 next-20261105]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Shivasharan-S/scsi-mpt3sas-Validate-firmware-reported-IOCRequestFrameSize/20261105-164216
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
patch link:    https://lore.kernel.org/r/1793925772-1303-15-git-send-email-shivasharan.srikanteshwara%40broadcom.com
patch subject: [PATCH 14/37] scsi: mpt3sas: Fix buffer overflow in _scsih_scsi_ioc_info
config: x86_64-rhel-9.4 (https://download.01.org/0day-ci/archive/20261106/202611061924.PaXwZZ0x-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20261106/202611061924.PaXwZZ0x-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202611061924.PaXwZZ0x-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/scsi/mpt3sas/mpt3sas_scsih.c: In function '_scsih_scsi_ioc_info':
>> drivers/scsi/mpt3sas/mpt3sas_scsih.c:5783:71: warning: '%s' directive output may be truncated writing up to 16 bytes into a region of size between 6 and 64 [-Wformat-truncation=]
    5783 |                 snprintf(desc_scsi_state, MPT_STRING_LENGTH, "%s%s%s%s%s",
         |                                                                       ^~
   drivers/scsi/mpt3sas/mpt3sas_scsih.c:5783:17: note: 'snprintf' output between 1 and 75 bytes into a destination of size 64
    5783 |                 snprintf(desc_scsi_state, MPT_STRING_LENGTH, "%s%s%s%s%s",
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    5784 |                          (scsi_state & MPI2_SCSI_STATE_RESPONSE_INFO_VALID) ?
         |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    5785 |                          "response info " : "",
         |                          ~~~~~~~~~~~~~~~~~~~~~~
    5786 |                          (scsi_state & MPI2_SCSI_STATE_TERMINATED) ?
         |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    5787 |                          "state terminated " : "",
         |                          ~~~~~~~~~~~~~~~~~~~~~~~~~
    5788 |                          (scsi_state & MPI2_SCSI_STATE_NO_SCSI_STATUS) ?
         |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    5789 |                          "no status " : "",
         |                          ~~~~~~~~~~~~~~~~~~
    5790 |                          (scsi_state & MPI2_SCSI_STATE_AUTOSENSE_FAILED) ?
         |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    5791 |                          "autosense failed " : "",
         |                          ~~~~~~~~~~~~~~~~~~~~~~~~~
    5792 |                          (scsi_state & MPI2_SCSI_STATE_AUTOSENSE_VALID) ?
         |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    5793 |                          "autosense valid " : "");
         |                          ~~~~~~~~~~~~~~~~~~~~~~~~


vim +5783 drivers/scsi/mpt3sas/mpt3sas_scsih.c

  5639	
  5640	/**
  5641	 * _scsih_scsi_ioc_info - translated non-successful SCSI_IO request
  5642	 * @ioc: per adapter object
  5643	 * @scmd: pointer to scsi command object
  5644	 * @mpi_reply: reply mf payload returned from firmware
  5645	 * @smid: ?
  5646	 *
  5647	 * scsi_status - SCSI Status code returned from target device
  5648	 * scsi_state - state info associated with SCSI_IO determined by ioc
  5649	 * ioc_status - ioc supplied status info
  5650	 */
  5651	static void
  5652	_scsih_scsi_ioc_info(struct MPT3SAS_ADAPTER *ioc, struct scsi_cmnd *scmd,
  5653		Mpi2SCSIIOReply_t *mpi_reply, u16 smid)
  5654	{
  5655		u32 response_info;
  5656		u8 *response_bytes;
  5657		u16 ioc_status = le16_to_cpu(mpi_reply->IOCStatus) &
  5658		    MPI2_IOCSTATUS_MASK;
  5659		u8 scsi_state = mpi_reply->SCSIState;
  5660		u8 scsi_status = mpi_reply->SCSIStatus;
  5661		char *desc_ioc_state = NULL;
  5662		char *desc_scsi_status = NULL;
  5663		char *desc_scsi_state = ioc->tmp_string;
  5664		u32 log_info = le32_to_cpu(mpi_reply->IOCLogInfo);
  5665		struct _sas_device *sas_device = NULL;
  5666		struct _pcie_device *pcie_device = NULL;
  5667		struct scsi_target *starget = scmd->device->sdev_target;
  5668		struct MPT3SAS_TARGET *priv_target = starget->hostdata;
  5669		char *device_str = NULL;
  5670	
  5671		if (!priv_target)
  5672			return;
  5673		if (ioc->hide_ir_msg)
  5674			device_str = "WarpDrive";
  5675		else
  5676			device_str = "volume";
  5677	
  5678		if (log_info == 0x31170000)
  5679			return;
  5680	
  5681		switch (ioc_status) {
  5682		case MPI2_IOCSTATUS_SUCCESS:
  5683			desc_ioc_state = "success";
  5684			break;
  5685		case MPI2_IOCSTATUS_INVALID_FUNCTION:
  5686			desc_ioc_state = "invalid function";
  5687			break;
  5688		case MPI2_IOCSTATUS_SCSI_RECOVERED_ERROR:
  5689			desc_ioc_state = "scsi recovered error";
  5690			break;
  5691		case MPI2_IOCSTATUS_SCSI_INVALID_DEVHANDLE:
  5692			desc_ioc_state = "scsi invalid dev handle";
  5693			break;
  5694		case MPI2_IOCSTATUS_SCSI_DEVICE_NOT_THERE:
  5695			desc_ioc_state = "scsi device not there";
  5696			break;
  5697		case MPI2_IOCSTATUS_SCSI_DATA_OVERRUN:
  5698			desc_ioc_state = "scsi data overrun";
  5699			break;
  5700		case MPI2_IOCSTATUS_SCSI_DATA_UNDERRUN:
  5701			desc_ioc_state = "scsi data underrun";
  5702			break;
  5703		case MPI2_IOCSTATUS_SCSI_IO_DATA_ERROR:
  5704			desc_ioc_state = "scsi io data error";
  5705			break;
  5706		case MPI2_IOCSTATUS_SCSI_PROTOCOL_ERROR:
  5707			desc_ioc_state = "scsi protocol error";
  5708			break;
  5709		case MPI2_IOCSTATUS_SCSI_TASK_TERMINATED:
  5710			desc_ioc_state = "scsi task terminated";
  5711			break;
  5712		case MPI2_IOCSTATUS_SCSI_RESIDUAL_MISMATCH:
  5713			desc_ioc_state = "scsi residual mismatch";
  5714			break;
  5715		case MPI2_IOCSTATUS_SCSI_TASK_MGMT_FAILED:
  5716			desc_ioc_state = "scsi task mgmt failed";
  5717			break;
  5718		case MPI2_IOCSTATUS_SCSI_IOC_TERMINATED:
  5719			desc_ioc_state = "scsi ioc terminated";
  5720			break;
  5721		case MPI2_IOCSTATUS_SCSI_EXT_TERMINATED:
  5722			desc_ioc_state = "scsi ext terminated";
  5723			break;
  5724		case MPI2_IOCSTATUS_EEDP_GUARD_ERROR:
  5725			desc_ioc_state = "eedp guard error";
  5726			break;
  5727		case MPI2_IOCSTATUS_EEDP_REF_TAG_ERROR:
  5728			desc_ioc_state = "eedp ref tag error";
  5729			break;
  5730		case MPI2_IOCSTATUS_EEDP_APP_TAG_ERROR:
  5731			desc_ioc_state = "eedp app tag error";
  5732			break;
  5733		case MPI2_IOCSTATUS_INSUFFICIENT_POWER:
  5734			desc_ioc_state = "insufficient power";
  5735			break;
  5736		default:
  5737			desc_ioc_state = "unknown";
  5738			break;
  5739		}
  5740	
  5741		switch (scsi_status) {
  5742		case MPI2_SCSI_STATUS_GOOD:
  5743			desc_scsi_status = "good";
  5744			break;
  5745		case MPI2_SCSI_STATUS_CHECK_CONDITION:
  5746			desc_scsi_status = "check condition";
  5747			break;
  5748		case MPI2_SCSI_STATUS_CONDITION_MET:
  5749			desc_scsi_status = "condition met";
  5750			break;
  5751		case MPI2_SCSI_STATUS_BUSY:
  5752			desc_scsi_status = "busy";
  5753			break;
  5754		case MPI2_SCSI_STATUS_INTERMEDIATE:
  5755			desc_scsi_status = "intermediate";
  5756			break;
  5757		case MPI2_SCSI_STATUS_INTERMEDIATE_CONDMET:
  5758			desc_scsi_status = "intermediate condmet";
  5759			break;
  5760		case MPI2_SCSI_STATUS_RESERVATION_CONFLICT:
  5761			desc_scsi_status = "reservation conflict";
  5762			break;
  5763		case MPI2_SCSI_STATUS_COMMAND_TERMINATED:
  5764			desc_scsi_status = "command terminated";
  5765			break;
  5766		case MPI2_SCSI_STATUS_TASK_SET_FULL:
  5767			desc_scsi_status = "task set full";
  5768			break;
  5769		case MPI2_SCSI_STATUS_ACA_ACTIVE:
  5770			desc_scsi_status = "aca active";
  5771			break;
  5772		case MPI2_SCSI_STATUS_TASK_ABORTED:
  5773			desc_scsi_status = "task aborted";
  5774			break;
  5775		default:
  5776			desc_scsi_status = "unknown";
  5777			break;
  5778		}
  5779	
  5780		if (!scsi_state)
  5781			desc_scsi_state = " ";
  5782		else
> 5783			snprintf(desc_scsi_state, MPT_STRING_LENGTH, "%s%s%s%s%s",
  5784				 (scsi_state & MPI2_SCSI_STATE_RESPONSE_INFO_VALID) ?
  5785				 "response info " : "",
  5786				 (scsi_state & MPI2_SCSI_STATE_TERMINATED) ?
  5787				 "state terminated " : "",
  5788				 (scsi_state & MPI2_SCSI_STATE_NO_SCSI_STATUS) ?
  5789				 "no status " : "",
  5790				 (scsi_state & MPI2_SCSI_STATE_AUTOSENSE_FAILED) ?
  5791				 "autosense failed " : "",
  5792				 (scsi_state & MPI2_SCSI_STATE_AUTOSENSE_VALID) ?
  5793				 "autosense valid " : "");
  5794	
  5795		scsi_print_command(scmd);
  5796	
  5797		if (priv_target->flags & MPT_TARGET_FLAGS_VOLUME) {
  5798			ioc_warn(ioc, "\t%s wwid(0x%016llx)\n",
  5799				 device_str, (u64)priv_target->sas_address);
  5800		} else if (priv_target->flags & MPT_TARGET_FLAGS_PCIE_DEVICE) {
  5801			pcie_device = mpt3sas_get_pdev_from_target(ioc, priv_target);
  5802			if (pcie_device) {
  5803				ioc_info(ioc, "\twwid(0x%016llx), port(%d)\n",
  5804					 (u64)pcie_device->wwid, pcie_device->port_num);
  5805				if (pcie_device->enclosure_handle != 0)
  5806					ioc_info(ioc, "\tenclosure logical id(0x%016llx), slot(%d)\n",
  5807						 (u64)pcie_device->enclosure_logical_id,
  5808						 pcie_device->slot);
  5809				if (pcie_device->connector_name[0])
  5810					ioc_info(ioc, "\tenclosure level(0x%04x), connector name( %s)\n",
  5811						 pcie_device->enclosure_level,
  5812						 pcie_device->connector_name);
  5813				pcie_device_put(pcie_device);
  5814			}
  5815		} else {
  5816			sas_device = mpt3sas_get_sdev_from_target(ioc, priv_target);
  5817			if (sas_device) {
  5818				ioc_warn(ioc, "\tsas_address(0x%016llx), phy(%d)\n",
  5819					 (u64)sas_device->sas_address, sas_device->phy);
  5820	
  5821				_scsih_display_enclosure_chassis_info(ioc, sas_device,
  5822				    NULL, NULL);
  5823	
  5824				sas_device_put(sas_device);
  5825			}
  5826		}
  5827	
  5828		ioc_warn(ioc, "\thandle(0x%04x), ioc_status(%s)(0x%04x), smid(%d)\n",
  5829			 le16_to_cpu(mpi_reply->DevHandle),
  5830			 desc_ioc_state, ioc_status, smid);
  5831		ioc_warn(ioc, "\trequest_len(%d), underflow(%d), resid(%d)\n",
  5832			 scsi_bufflen(scmd), scmd->underflow, scsi_get_resid(scmd));
  5833		ioc_warn(ioc, "\ttag(%d), transfer_count(%d), sc->result(0x%08x)\n",
  5834			 le16_to_cpu(mpi_reply->TaskTag),
  5835			 le32_to_cpu(mpi_reply->TransferCount), scmd->result);
  5836		ioc_warn(ioc, "\tscsi_status(%s)(0x%02x), scsi_state(%s)(0x%02x)\n",
  5837			 desc_scsi_status, scsi_status, desc_scsi_state, scsi_state);
  5838	
  5839		if (scsi_state & MPI2_SCSI_STATE_AUTOSENSE_VALID) {
  5840			struct sense_info data;
  5841			_scsih_normalize_sense(scmd->sense_buffer, &data);
  5842			ioc_warn(ioc, "\t[sense_key,asc,ascq]: [0x%02x,0x%02x,0x%02x], count(%d)\n",
  5843				 data.skey, data.asc, data.ascq,
  5844				 le32_to_cpu(mpi_reply->SenseCount));
  5845		}
  5846		if (scsi_state & MPI2_SCSI_STATE_RESPONSE_INFO_VALID) {
  5847			response_info = le32_to_cpu(mpi_reply->ResponseInfo);
  5848			response_bytes = (u8 *)&response_info;
  5849			_scsih_response_code(ioc, response_bytes[0]);
  5850		}
  5851	}
  5852	

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 15/37] scsi: mpt3sas: Fix Scsi_Host leak on invalid/tampered device probe

[Shivasharan S]

Release the Scsi_Host before returning on the invalid/tampered
device-ID paths in _scsih_probe().

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 47bc48f7b9a3..066378a00fa4 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -13549,6 +13549,7 @@ _scsih_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 			    "HBA with DeviceId 0x%04x, sub VendorId 0x%04x, sub DeviceId 0x%04x is Invalid",
 			    pdev->device, pdev->subsystem_vendor,
 			    pdev->subsystem_device);
+			scsi_host_put(shost);
 			return 1;
 		case MPI26_MFGPAGE_DEVID_INVALID1_3816:
 		case MPI26_MFGPAGE_DEVID_INVALID1_3916:
@@ -13556,6 +13557,7 @@ _scsih_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 			    "HBA with DeviceId 0x%04x, sub VendorId 0x%04x, sub DeviceId 0x%04x is Tampered",
 			    pdev->device, pdev->subsystem_vendor,
 			    pdev->subsystem_device);
+			scsi_host_put(shost);
 			return 1;
 		case MPI26_MFGPAGE_DEVID_CFG_SEC_3816:
 		case MPI26_MFGPAGE_DEVID_CFG_SEC_3916:
-- 
2.53.0

[PATCH 16/37] scsi: mpt3sas: Fix page fault from stale ioc->chip after PCI error recovery

[Shivasharan S]

NULL ioc->chip after unmapping it and guard mask/unmask_interrupts()
against a NULL chip pointer.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 060482ba20ae..f0ed1bea64bb 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -1645,6 +1645,8 @@ mpt3sas_base_mask_interrupts(struct MPT3SAS_ADAPTER *ioc)
 	u32 him_register;
 
 	ioc->mask_interrupts = 1;
+	if (!ioc->chip)
+		return;
 	him_register = ioc->base_readl(&ioc->chip->HostInterruptMask);
 	him_register |= MPI2_HIM_DIM + MPI2_HIM_RIM + MPI2_HIM_RESET_IRQ_MASK;
 	writel(him_register, &ioc->chip->HostInterruptMask);
@@ -1662,6 +1664,8 @@ mpt3sas_base_unmask_interrupts(struct MPT3SAS_ADAPTER *ioc)
 {
 	u32 him_register;
 
+	if (!ioc->chip)
+		return;
 	him_register = ioc->base_readl(&ioc->chip->HostInterruptMask);
 	him_register &= ~MPI2_HIM_RIM;
 	writel(him_register, &ioc->chip->HostInterruptMask);
@@ -3579,6 +3583,7 @@ mpt3sas_base_unmap_resources(struct MPT3SAS_ADAPTER *ioc)
 
 	if (ioc->chip_phys) {
 		iounmap(ioc->chip);
+		ioc->chip = NULL;
 		ioc->chip_phys = 0;
 	}
 
@@ -3682,9 +3687,11 @@ mpt3sas_base_map_resources(struct MPT3SAS_ADAPTER *ioc)
 		} else if (pci_resource_flags(pdev, i) & IORESOURCE_MEM) {
 			if (memap_sz)
 				continue;
+			memap_sz = pci_resource_len(pdev, i);
+			if (!memap_sz)
+				continue;
 			ioc->chip_phys = pci_resource_start(pdev, i);
 			chip_phys = ioc->chip_phys;
-			memap_sz = pci_resource_len(pdev, i);
 			ioc->chip = ioremap(ioc->chip_phys, memap_sz);
 		}
 	}
-- 
2.53.0

Re: [PATCH 16/37] scsi: mpt3sas: Fix page fault from stale ioc->chip after PCI error recovery

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: mpt3sas: NULL ioc->chip dereference in teardown and reset paths
--

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

scsi: mpt3sas: Fix page fault from stale ioc->chip after PCI error recovery

This commit sets ioc->chip to NULL when unmapping controller resources in
mpt3sas_base_unmap_resources(), guards mpt3sas_base_mask_interrupts() and
mpt3sas_base_unmask_interrupts() against a NULL ioc->chip pointer, and skips
zero-length memory BARs in mpt3sas_base_map_resources().

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 060482ba20aef..f0ed1bea64bbc 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -1645,6 +1645,8 @@ mpt3sas_base_mask_interrupts(struct MPT3SAS_ADAPTER *ioc)
>  	u32 him_register;
>  
>  	ioc->mask_interrupts = 1;
> +	if (!ioc->chip)
> +		return;
>  	him_register = ioc->base_readl(&ioc->chip->HostInterruptMask);
>  	him_register |= MPI2_HIM_DIM + MPI2_HIM_RIM + MPI2_HIM_RESET_IRQ_MASK;
>  	writel(him_register, &ioc->chip->HostInterruptMask);
> @@ -1662,6 +1664,8 @@ mpt3sas_base_unmask_interrupts(struct MPT3SAS_ADAPTER *ioc)
>  {
>  	u32 him_register;
>  
> +	if (!ioc->chip)
> +		return;
>  	him_register = ioc->base_readl(&ioc->chip->HostInterruptMask);
>  	him_register &= ~MPI2_HIM_RIM;
>  	writel(him_register, &ioc->chip->HostInterruptMask);
> @@ -3579,6 +3583,7 @@ mpt3sas_base_unmap_resources(struct MPT3SAS_ADAPTER *ioc)
>  
>  	if (ioc->chip_phys) {
>  		iounmap(ioc->chip);
> +		ioc->chip = NULL;
>  		ioc->chip_phys = 0;
>  	}

[Severity: High]
When mpt3sas_base_unmap_resources() sets ioc->chip = NULL (such as during
scsih_pci_error_detected() on pci_channel_io_frozen, scsih_suspend(), or when
mpt3sas_base_map_resources() fails in scsih_pci_slot_reset() or
scsih_resume()), can the surrounding teardown and reset paths still
dereference a NULL ioc->chip pointer?

In drivers/scsi/mpt3sas/mpt3sas_scsih.c, scsih_shutdown() and scsih_remove()
run several helpers right around the newly guarded
mpt3sas_base_mask_interrupts() call without checking if ioc->chip is NULL:

drivers/scsi/mpt3sas/mpt3sas_scsih.c:scsih_shutdown() {
	...
	if (ioc->is_aero_ioc)
		mpt3sas_config_set_ioc_pg1(ioc, &mpi_reply,
				&ioc->ioc_pg1_copy);

	_scsih_ir_shutdown(ioc);
	_scsih_nvme_shutdown(ioc);
	mpt3sas_base_mask_interrupts(ioc);
	mpt3sas_base_stop_watchdog(ioc);
	ioc->shost_recovery = 1;
	mpt3sas_base_make_ioc_ready(ioc, SOFT_RESET);
	...
}

drivers/scsi/mpt3sas/mpt3sas_scsih.c:scsih_remove() {
	...
	if (ioc->is_aero_ioc)
		mpt3sas_config_set_ioc_pg1(ioc, &mpi_reply,
				&ioc->ioc_pg1_copy);
	/* release all the volumes */
	_scsih_ir_shutdown(ioc);
	...
}

Specifically:

1. mpt3sas_config_set_ioc_pg1() (when ioc->is_aero_ioc is set) calls
_config_request() -> mpt3sas_wait_for_ioc() -> mpt3sas_base_get_iocstate(),
which reads &ioc->chip->Doorbell in drivers/scsi/mpt3sas/mpt3sas_base.c:

	s = ioc->base_readl_ext_retry(&ioc->chip->Doorbell);

2. _scsih_ir_shutdown() (when ioc->ir_firmware is set and raid_device_list is
non-empty) and _scsih_nvme_shutdown() (when pcie_device_list is non-empty)
both call ioc->put_smid_default(ioc, smid), which writes to
&ioc->chip->RequestDescriptorPostLow or
&ioc->chip->AtomicRequestDescriptorPost without checking ioc->chip or
ioc->pci_error_recovery.

3. In scsih_shutdown(), mpt3sas_base_make_ioc_ready(ioc, SOFT_RESET) is
called unconditionally without the if (ioc->chip_phys && ioc->chip) check used
in mpt3sas_base_free_resources(). Because mpt3sas_base_make_ioc_ready() only
checks ioc->pci_error_recovery and not ioc->chip, any path where ioc->chip is
NULL and ioc->pci_error_recovery is 0 (for example, after
scsih_pci_slot_reset() clears ioc->pci_error_recovery = 0 and fails
mpt3sas_base_map_resources(), or after scsih_suspend() / failed
scsih_resume()) calls mpt3sas_base_get_iocstate(ioc, 0) and dereferences
&ioc->chip->Doorbell.

4. Similarly, in mpt3sas_base_hard_reset_handler() in
drivers/scsi/mpt3sas/mpt3sas_base.c, mpt3sas_wait_for_commands_to_complete()
and mpt3sas_base_make_ioc_ready() immediately precede and follow
mpt3sas_base_mask_interrupts() and both call mpt3sas_base_get_iocstate(),
dereferencing &ioc->chip->Doorbell when ioc->chip is NULL.

Should these callers and mpt3sas_base_get_iocstate() /
mpt3sas_base_make_ioc_ready() also guard against !ioc->chip?

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

[PATCH 17/37] scsi: mpt3sas: Clamp ValidEntries in driver trigger page update helpers

[Shivasharan S]

Clamp ValidEntries to NUM_VALID_ENTRIES in the pg2/pg3/pg4 trigger
update helpers before it's used as a copy count.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_config.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_config.c b/drivers/scsi/mpt3sas/mpt3sas_config.c
index 75312d43e386..ddbccbac69b3 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_config.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_config.c
@@ -2278,6 +2278,8 @@ mpt3sas_config_update_driver_trigger_pg2(struct MPT3SAS_ADAPTER *ioc,
 
 	if (set) {
 		count = event_tg->ValidEntries;
+		if (count > NUM_VALID_ENTRIES)
+			count = NUM_VALID_ENTRIES;
 		tg_pg2.NumMPIEventTrigger = cpu_to_le16(count);
 		for (i = 0; i < count; i++) {
 			tg_pg2.MPIEventTriggers[i].MPIEventCode =
@@ -2437,6 +2439,8 @@ mpt3sas_config_update_driver_trigger_pg3(struct MPT3SAS_ADAPTER *ioc,
 
 	if (set) {
 		count = scsi_tg->ValidEntries;
+		if (count > NUM_VALID_ENTRIES)
+			count = NUM_VALID_ENTRIES;
 		tg_pg3.NumSCSISenseTrigger = cpu_to_le16(count);
 		for (i = 0; i < count; i++) {
 			tg_pg3.SCSISenseTriggers[i].ASCQ =
@@ -2595,6 +2599,8 @@ mpt3sas_config_update_driver_trigger_pg4(struct MPT3SAS_ADAPTER *ioc,
 
 	if (set) {
 		count = mpi_tg->ValidEntries;
+		if (count > NUM_VALID_ENTRIES)
+			count = NUM_VALID_ENTRIES;
 		tg_pg4.NumIOCStatusLogInfoTrigger = cpu_to_le16(count);
 		for (i = 0; i < count; i++) {
 			tg_pg4.IOCStatusLoginfoTriggers[i].IOCStatus =
-- 
2.53.0

Re: [PATCH 17/37] scsi: mpt3sas: Clamp ValidEntries in driver trigger page update helpers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: mpt3sas: signed count bypasses clamp in trigger update helpers
--

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

scsi: mpt3sas: Clamp ValidEntries in driver trigger page update helpers

This commit clamps ValidEntries to NUM_VALID_ENTRIES in the pg2, pg3, and
pg4 driver trigger update helpers before using it as a loop bound and trigger
count.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_config.c b/drivers/scsi/mpt3sas/mpt3sas_config.c
> index 75312d43e3862..ddbccbac69b34 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_config.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_config.c
> @@ -2278,6 +2278,8 @@ mpt3sas_config_update_driver_trigger_pg2(struct MPT3SAS_ADAPTER *ioc,
>  
>  	if (set) {
>  		count = event_tg->ValidEntries;
> +		if (count > NUM_VALID_ENTRIES)
> +			count = NUM_VALID_ENTRIES;
>  		tg_pg2.NumMPIEventTrigger = cpu_to_le16(count);
>  		for (i = 0; i < count; i++) {
>  			tg_pg2.MPIEventTriggers[i].MPIEventCode =
> @@ -2437,6 +2439,8 @@ mpt3sas_config_update_driver_trigger_pg3(struct MPT3SAS_ADAPTER *ioc,
>  
>  	if (set) {
>  		count = scsi_tg->ValidEntries;
> +		if (count > NUM_VALID_ENTRIES)
> +			count = NUM_VALID_ENTRIES;
>  		tg_pg3.NumSCSISenseTrigger = cpu_to_le16(count);
>  		for (i = 0; i < count; i++) {
>  			tg_pg3.SCSISenseTriggers[i].ASCQ =
> @@ -2595,6 +2599,8 @@ mpt3sas_config_update_driver_trigger_pg4(struct MPT3SAS_ADAPTER *ioc,
>  
>  	if (set) {
>  		count = mpi_tg->ValidEntries;
> +		if (count > NUM_VALID_ENTRIES)
> +			count = NUM_VALID_ENTRIES;
>  		tg_pg4.NumIOCStatusLogInfoTrigger = cpu_to_le16(count);

[Severity: Medium]
Can a ValidEntries value with bit 31 set (>= 0x80000000) bypass this clamp in
mpt3sas_config_update_driver_trigger_pg2(),
mpt3sas_config_update_driver_trigger_pg3(), and
mpt3sas_config_update_driver_trigger_pg4()?

In drivers/scsi/mpt3sas/mpt3sas_trigger_diag.h, ValidEntries is declared as
uint32_t in struct SL_WH_EVENT_TRIGGERS_T, struct SL_WH_SCSI_TRIGGERS_T, and
struct SL_WH_MPI_TRIGGERS_T, whereas count in these three helpers in
drivers/scsi/mpt3sas/mpt3sas_config.c is a signed int:

	int rc, i, count;

The sysfs store handlers diag_trigger_event_store(),
diag_trigger_scsi_store(), and diag_trigger_mpi_store() in
drivers/scsi/mpt3sas/mpt3sas_ctl.c copy user data into event_tg, scsi_tg, or
mpi_tg and call these update helpers before clamping ValidEntries:

	memcpy(event_tg, buf, sz);
	if (!event_tg->ValidEntries)
		set = 0;
	if (mpt3sas_config_update_driver_trigger_pg2(ioc, event_tg,
	    set)) {

When a user passes ValidEntries >= 0x80000000, assigning it to the signed int
count makes count negative. The signed comparison count > NUM_VALID_ENTRIES
then evaluates to false, the for (i = 0; i < count; i++) loop copies zero
entries, and cpu_to_le16(count) writes the truncated unclamped u16 value into
tg_pg2.NumMPIEventTrigger, tg_pg3.NumSCSISenseTrigger, or
tg_pg4.NumIOCStatusLogInfoTrigger before committing the page to the
controller.

Should count (and i) be unsigned, or should event_tg->ValidEntries,
scsi_tg->ValidEntries, and mpi_tg->ValidEntries be clamped using min_t(u32,
..., NUM_VALID_ENTRIES) before assigning to count?

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

[PATCH 18/37] scsi: mpt3sas: Clamp firmware reply copies to the allocated reply buffer

[Shivasharan S]

Clamp MsgLength-based memcpy()s of firmware replies to ioc->reply_sz
across the driver's completion handlers.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c      | 6 ++++--
 drivers/scsi/mpt3sas/mpt3sas_config.c    | 2 +-
 drivers/scsi/mpt3sas/mpt3sas_ctl.c       | 3 ++-
 drivers/scsi/mpt3sas/mpt3sas_scsih.c     | 5 +++--
 drivers/scsi/mpt3sas/mpt3sas_transport.c | 2 +-
 5 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index f0ed1bea64bb..4e2cee2e13fc 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -1459,7 +1459,8 @@ mpt3sas_base_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index,
 	ioc->base_cmds.status |= MPT3_CMD_COMPLETE;
 	if (mpi_reply) {
 		ioc->base_cmds.status |= MPT3_CMD_REPLY_VALID;
-		memcpy(ioc->base_cmds.reply, mpi_reply, mpi_reply->MsgLength*4);
+		memcpy(ioc->base_cmds.reply, mpi_reply,
+		       min_t(u32, mpi_reply->MsgLength * 4, ioc->reply_sz));
 	}
 	ioc->base_cmds.status &= ~MPT3_CMD_PENDING;
 
@@ -7679,7 +7680,8 @@ mpt3sas_port_enable_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index,
 	ioc->port_enable_cmds.status &= ~MPT3_CMD_PENDING;
 	ioc->port_enable_cmds.status |= MPT3_CMD_COMPLETE;
 	ioc->port_enable_cmds.status |= MPT3_CMD_REPLY_VALID;
-	memcpy(ioc->port_enable_cmds.reply, mpi_reply, mpi_reply->MsgLength*4);
+	memcpy(ioc->port_enable_cmds.reply, mpi_reply,
+	       min_t(u32, mpi_reply->MsgLength * 4, ioc->reply_sz));
 	ioc_status = le16_to_cpu(mpi_reply->IOCStatus) & MPI2_IOCSTATUS_MASK;
 	if (ioc_status != MPI2_IOCSTATUS_SUCCESS)
 		ioc->port_enable_failed = 1;
diff --git a/drivers/scsi/mpt3sas/mpt3sas_config.c b/drivers/scsi/mpt3sas/mpt3sas_config.c
index ddbccbac69b3..2b5e42255d3b 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_config.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_config.c
@@ -263,7 +263,7 @@ mpt3sas_config_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index,
 	if (mpi_reply) {
 		ioc->config_cmds.status |= MPT3_CMD_REPLY_VALID;
 		memcpy(ioc->config_cmds.reply, mpi_reply,
-		    mpi_reply->MsgLength*4);
+		       min_t(u32, mpi_reply->MsgLength * 4, ioc->reply_sz));
 	}
 	ioc->config_cmds.status &= ~MPT3_CMD_PENDING;
 	if (ioc->logging_level & MPT_DEBUG_CONFIG)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index 4866b3c7d1c1..6df51aa7bcaa 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -274,7 +274,8 @@ mpt3sas_ctl_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index,
 	ioc->ctl_cmds.status |= MPT3_CMD_COMPLETE;
 	mpi_reply = mpt3sas_base_get_reply_virt_addr(ioc, reply);
 	if (mpi_reply) {
-		memcpy(ioc->ctl_cmds.reply, mpi_reply, mpi_reply->MsgLength*4);
+		memcpy(ioc->ctl_cmds.reply, mpi_reply,
+		       min_t(u32, mpi_reply->MsgLength * 4, ioc->reply_sz));
 		ioc->ctl_cmds.status |= MPT3_CMD_REPLY_VALID;
 		/* get sense data */
 		if (mpi_reply->Function == MPI2_FUNCTION_SCSI_IO_REQUEST ||
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 066378a00fa4..59ec75e0736e 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -2941,7 +2941,8 @@ _scsih_tm_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index, u32 reply)
 	ioc->tm_cmds.status |= MPT3_CMD_COMPLETE;
 	mpi_reply =  mpt3sas_base_get_reply_virt_addr(ioc, reply);
 	if (mpi_reply) {
-		memcpy(ioc->tm_cmds.reply, mpi_reply, mpi_reply->MsgLength*4);
+		memcpy(ioc->tm_cmds.reply, mpi_reply,
+		       min_t(u32, mpi_reply->MsgLength * 4, ioc->reply_sz));
 		ioc->tm_cmds.status |= MPT3_CMD_REPLY_VALID;
 	}
 	ioc->tm_cmds.status &= ~MPT3_CMD_PENDING;
@@ -7433,7 +7434,7 @@ _scsih_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index, u32 reply)
 	ioc->scsih_cmds.status |= MPT3_CMD_COMPLETE;
 	if (mpi_reply) {
 		memcpy(ioc->scsih_cmds.reply, mpi_reply,
-		    mpi_reply->MsgLength*4);
+		       min_t(u32, mpi_reply->MsgLength * 4, ioc->reply_sz));
 		ioc->scsih_cmds.status |= MPT3_CMD_REPLY_VALID;
 	}
 	ioc->scsih_cmds.status &= ~MPT3_CMD_PENDING;
diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c b/drivers/scsi/mpt3sas/mpt3sas_transport.c
index abb46a5e1c1a..32db07ed808a 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c
@@ -305,7 +305,7 @@ mpt3sas_transport_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index,
 	ioc->transport_cmds.status |= MPT3_CMD_COMPLETE;
 	if (mpi_reply) {
 		memcpy(ioc->transport_cmds.reply, mpi_reply,
-		    mpi_reply->MsgLength*4);
+		       min_t(u32, mpi_reply->MsgLength * 4, ioc->reply_sz));
 		ioc->transport_cmds.status |= MPT3_CMD_REPLY_VALID;
 	}
 	ioc->transport_cmds.status &= ~MPT3_CMD_PENDING;
-- 
2.53.0

[PATCH 19/37] scsi: mpt3sas: Reject implausibly small MaxReplyDescriptorPostQueueDepth

[Shivasharan S]

Avoid an unsigned underflow in the hba_queue_depth calculation when
firmware reports a too-small queue depth.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 7 +++++++
 drivers/scsi/mpt3sas/mpt3sas_base.h | 9 +++++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 4e2cee2e13fc..12295ab544ad 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -6452,6 +6452,13 @@ _base_allocate_memory_pools(struct MPT3SAS_ADAPTER *ioc)
 				(ioc->reply_post_queue_depth % 16);
 	}
 
+	if (facts->MaxReplyDescriptorPostQueueDepth <
+	    MPT3SAS_MIN_REPLY_POST_QUEUE_DEPTH) {
+		ioc_err(ioc, "MaxReplyDescriptorPostQueueDepth is too small (%u)\n",
+			facts->MaxReplyDescriptorPostQueueDepth);
+		return -EINVAL;
+	}
+
 	if (ioc->reply_post_queue_depth >
 	    facts->MaxReplyDescriptorPostQueueDepth) {
 		ioc->reply_post_queue_depth =
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h
index 47255bf9cdda..192b8e8a5b4d 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -145,6 +145,15 @@
 #define MPT_MAX_HBA_NUM_PHYS		32
 
 #define INTERNAL_CMDS_COUNT		10	/* reserved cmds */
+
+/*
+ * Minimum reply post queue depth for hba_queue_depth to stay non-negative.
+ * Values 66-79 floor to 64 after 16-alignment, making ((64-64)/2)-1 = -1
+ * which wraps to 65535 as u16. The smallest value whose aligned floor exceeds
+ * 64 is 80 (80 % 16 == 0, giving ((80-64)/2)-1 == 7).
+ */
+#define MPT3SAS_MIN_REPLY_POST_QUEUE_DEPTH	80
+
 /* reserved for issuing internally framed scsi io cmds */
 #define INTERNAL_SCSIIO_CMDS_COUNT	3
 #define INTERNAL_SCSIIO_FOR_DISCOVERY	2
-- 
2.53.0

Re: [PATCH 19/37] scsi: mpt3sas: Reject implausibly small MaxReplyDescriptorPostQueueDepth

[sashiko-bot]

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

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

scsi: mpt3sas: Reject implausibly small MaxReplyDescriptorPostQueueDepth

This commit adds a minimum bound check on MaxReplyDescriptorPostQueueDepth in
_base_allocate_memory_pools() to prevent an unsigned underflow when computing
ioc->hba_queue_depth.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 4e2cee2e13fc2..12295ab544ad5 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -6452,6 +6452,13 @@ _base_allocate_memory_pools(struct MPT3SAS_ADAPTER *ioc)
>  				(ioc->reply_post_queue_depth % 16);
>  	}
>  
> +	if (facts->MaxReplyDescriptorPostQueueDepth <
> +	    MPT3SAS_MIN_REPLY_POST_QUEUE_DEPTH) {
> +		ioc_err(ioc, "MaxReplyDescriptorPostQueueDepth is too small (%u)\n",
> +			facts->MaxReplyDescriptorPostQueueDepth);
> +		return -EINVAL;
> +	}
> +
>  	if (ioc->reply_post_queue_depth >
>  	    facts->MaxReplyDescriptorPostQueueDepth) {
>  		ioc->reply_post_queue_depth =
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h
> index 47255bf9cdda1..192b8e8a5b4d4 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.h
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
> @@ -145,6 +145,15 @@
>  #define MPT_MAX_HBA_NUM_PHYS		32
>  
>  #define INTERNAL_CMDS_COUNT		10	/* reserved cmds */
> +
> +/*
> + * Minimum reply post queue depth for hba_queue_depth to stay non-negative.
> + * Values 66-79 floor to 64 after 16-alignment, making ((64-64)/2)-1 = -1
> + * which wraps to 65535 as u16. The smallest value whose aligned floor exceeds
> + * 64 is 80 (80 % 16 == 0, giving ((80-64)/2)-1 == 7).
> + */
> +#define MPT3SAS_MIN_REPLY_POST_QUEUE_DEPTH	80

[Severity: High]
Does a minimum of 80 for MPT3SAS_MIN_REPLY_POST_QUEUE_DEPTH still leave
ioc->scsiio_depth and ioc->shost->can_queue vulnerable to underflow in
_base_allocate_memory_pools()?

Earlier in _base_allocate_memory_pools(), ioc->internal_depth and
ioc->hi_priority_depth are initialized as:

	ioc->internal_depth = min_t(int, (facts->HighPriorityCredit + (5)),
		(facts->RequestCredit / 4));
	if (ioc->internal_depth < INTERNAL_CMDS_COUNT) {
		...
		ioc->internal_depth = 10;
	}

	ioc->hi_priority_depth = ioc->internal_depth - (5);

So ioc->internal_depth is at least 10 and ioc->hi_priority_depth is at least
5 (and both can be larger when facts->HighPriorityCredit > 5).

When facts->MaxReplyDescriptorPostQueueDepth is between 80 and 95, it passes
the MPT3SAS_MIN_REPLY_POST_QUEUE_DEPTH check and clamps ioc->hba_queue_depth
to 7:

	if (ioc->reply_post_queue_depth >
	    facts->MaxReplyDescriptorPostQueueDepth) {
		ioc->reply_post_queue_depth =
				facts->MaxReplyDescriptorPostQueueDepth -
		    (facts->MaxReplyDescriptorPostQueueDepth % 16);
		ioc->hba_queue_depth =
				((ioc->reply_post_queue_depth - 64) / 2) - 1;
		ioc->reply_free_queue_depth = ioc->hba_queue_depth + 64;
	}

Later in _base_allocate_memory_pools(), ioc->hi_priority_depth and
ioc->internal_depth are subtracted from ioc->hba_queue_depth:

	ioc->scsiio_depth = ioc->hba_queue_depth -
	    ioc->hi_priority_depth - ioc->internal_depth;

	/* set the scsi host can_queue depth
	 * with some internal commands that could be outstanding
	 */
	ioc->shost->can_queue = ioc->scsiio_depth - INTERNAL_SCSIIO_CMDS_COUNT;

With ioc->hba_queue_depth == 7, subtracting ioc->hi_priority_depth (>= 5) and
ioc->internal_depth (>= 10) underflows the u16 ioc->scsiio_depth to at most
65528, setting ioc->shost->can_queue to 65525 and sizing the request and
lookup allocations for ~65k entries. Similarly, if
facts->MaxReplyDescriptorPostQueueDepth is in 96..111, ioc->hba_queue_depth
becomes 15, which yields ioc->scsiio_depth == 0 and a negative
ioc->shost->can_queue of -3 (or underflows ioc->scsiio_depth if
ioc->internal_depth > 10).

Should _base_allocate_memory_pools() also verify that the resulting
ioc->hba_queue_depth is greater than ioc->hi_priority_depth +
ioc->internal_depth + INTERNAL_SCSIIO_CMDS_COUNT?

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

[PATCH 20/37] scsi: mpt3sas: Validate CurrentHostPageSize before using it as a shift amount

[Shivasharan S]

Reject an out-of-range CurrentHostPageSize instead of using it
directly as a shift amount.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 12295ab544ad..3622d8a42725 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -7532,8 +7532,15 @@ _base_get_ioc_facts(struct MPT3SAS_ADAPTER *ioc)
 	facts->CurrentHostPageSize = mpi_reply.CurrentHostPageSize;
 
 	/*
-	 * Get the Page Size from IOC Facts. If it's 0, default to 4k.
+	 * Get the Page Size from IOC Facts. If it's 0 or too large to be a
+	 * valid shift amount, default to 4k.
 	 */
+	if ((facts->CurrentHostPageSize >= 32) ||
+	    (facts->CurrentHostPageSize != 0 && facts->CurrentHostPageSize < 12)) {
+		ioc_warn(ioc, "CurrentHostPageSize(%d) is invalid: Setting default host page size to 4k\n",
+			 facts->CurrentHostPageSize);
+		facts->CurrentHostPageSize = MPT3SAS_HOST_PAGE_SIZE_4K;
+	}
 	ioc->page_size = 1 << facts->CurrentHostPageSize;
 	if (ioc->page_size == 1) {
 		ioc_info(ioc, "CurrentHostPageSize is 0: Setting default host page size to 4k\n");
-- 
2.53.0

Re: [PATCH 20/37] scsi: mpt3sas: Validate CurrentHostPageSize before using it as a shift amount

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: mpt3sas: signed shift overflow and bad bound in _base_get_ioc_facts()
--

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

scsi: mpt3sas: Validate CurrentHostPageSize before using it as a shift amount

This commit validates facts->CurrentHostPageSize read from the IOCFacts reply
in _base_get_ioc_facts() and falls back to MPT3SAS_HOST_PAGE_SIZE_4K if it is
outside the allowed range before computing ioc->page_size.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 12295ab544ad5..3622d8a42725b 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -7532,8 +7532,15 @@ _base_get_ioc_facts(struct MPT3SAS_ADAPTER *ioc)
>  	facts->CurrentHostPageSize = mpi_reply.CurrentHostPageSize;
>  
>  	/*
> -	 * Get the Page Size from IOC Facts. If it's 0, default to 4k.
> +	 * Get the Page Size from IOC Facts. If it's 0 or too large to be a
> +	 * valid shift amount, default to 4k.
>  	 */
> +	if ((facts->CurrentHostPageSize >= 32) ||
> +	    (facts->CurrentHostPageSize != 0 && facts->CurrentHostPageSize < 12)) {
> +		ioc_warn(ioc, "CurrentHostPageSize(%d) is invalid: Setting default host page size to 4k\n",
> +			 facts->CurrentHostPageSize);
> +		facts->CurrentHostPageSize = MPT3SAS_HOST_PAGE_SIZE_4K;
> +	}
>  	ioc->page_size = 1 << facts->CurrentHostPageSize;

[Severity: Medium]
Can facts->CurrentHostPageSize == 31 still trigger a signed 32-bit shift
overflow here, and can values in 13..31 break downstream page_size consumers?

Because the left operand of 1 << facts->CurrentHostPageSize is a signed 32-bit
int literal (rather than 1U or BIT()), a value of 31 passes the >= 32 check
and shifts 1 into the sign bit, which is undefined behavior in C and still
triggers UBSAN shift-out-of-bounds.

In addition, _base_send_ioc_init() always programs the controller for a 4 KiB
host page size:

drivers/scsi/mpt3sas/mpt3sas_base.c:_base_send_ioc_init() {
	...
	mpi_request.HostPageSize = MPT3SAS_HOST_PAGE_SIZE_4K;
	...
}

Allowing CurrentHostPageSize values up to 31 produces ioc->page_size values up
to 0x80000000U (2 GiB). During probe in _base_allocate_memory_pools(), this
can overflow 32-bit pool sizing arithmetic and cause
_base_allocate_pcie_sgl_pool() to fail in dma_pool_create() with -ENOMEM
instead of falling back to 4 KiB:

drivers/scsi/mpt3sas/mpt3sas_base.c:_base_allocate_memory_pools() {
	...
		sz = nvme_blocks_needed * ioc->page_size;
		rc = _base_allocate_pcie_sgl_pool(ioc, sz);
		if (rc == -ENOMEM)
			return -ENOMEM;
		else if (rc == -EAGAIN)
			goto try_32bit_dma;
		total_sz += sz * ioc->scsiio_depth;
	...
}

If a controller reset via
mpt3sas_base_hard_reset_handler()->_base_get_ioc_facts() updates
ioc->page_size to 0x80000000U after pools are already allocated,
base_make_prp_nvme() subtracts u32 nvme_pg_size (ioc->page_size) from signed
int data_len and sge_len:

drivers/scsi/mpt3sas/mpt3sas_base.c:base_make_prp_nvme() {
	...
		sge_addr += nvme_pg_size;
		sge_len -= nvme_pg_size;
		data_len -= nvme_pg_size;

		if (data_len <= 0)
			break;
	...
}

When nvme_pg_size is 0x80000000U, subtracting it wraps negative signed 32-bit
values back to positive, which can walk sg_next(sg_scmd) past the end of the
scatterlist and overrun the 4 KiB pcie_sgl DMA buffer.

Should this use 1U << facts->CurrentHostPageSize (or BIT()) and restrict
CurrentHostPageSize to MPT3SAS_HOST_PAGE_SIZE_4K (or a safe upper bound that
cannot overflow DMA pool and NVMe PRP calculations)?

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

[PATCH 21/37] scsi: mpt3sas: Clamp sense_length before copying into transfer_packet->sense

[Shivasharan S]

Clamp the firmware-reported SenseCount to sizeof(sense) before the
copy.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 59ec75e0736e..073a487430e5 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -7579,7 +7579,8 @@ _scsi_send_scsi_io(struct MPT3SAS_ADAPTER *ioc, struct _scsi_io_transfer
 		transfer_packet->valid_reply = 1;
 		mpi_reply = ioc->scsih_cmds.reply;
 		transfer_packet->sense_length =
-		   le32_to_cpu(mpi_reply->SenseCount);
+		   min_t(u32, le32_to_cpu(mpi_reply->SenseCount),
+			 sizeof(transfer_packet->sense));
 		if (transfer_packet->sense_length)
 			memcpy(transfer_packet->sense, priv_sense,
 			    transfer_packet->sense_length);
-- 
2.53.0

[PATCH 22/37] scsi: mpt3sas: Clamp num_luns in _scsih_wait_for_target_to_become_ready

[Shivasharan S]

Clamp num_luns to the lun_data allocation size before using it as a
loop bound.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 073a487430e5..df5f16db8f48 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -8187,6 +8187,8 @@ _scsih_wait_for_target_to_become_ready(struct MPT3SAS_ADAPTER *ioc, u16 handle,
 		(data[2] << 8) | (data[3] << 0));
 
 	num_luns = (length / sizeof(struct scsi_lun));
+	/* Clamp to last valid index: loop uses inclusive bound lun_data[num_luns] */
+	num_luns = min_t(u32, num_luns, MPT3_MAX_LUNS - 1);
 
 	lunp = &lun_data[1];
 	lun = (num_luns) ? mpt_scsilun_to_int(&lun_data[1]) : 0;
-- 
2.53.0

[PATCH 23/37] scsi: mpt3sas: Validate phy_number in mpt3sas_transport_update_links

[Shivasharan S]

Check phy_number against sas_node->num_phys before indexing the phy
array.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_transport.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c b/drivers/scsi/mpt3sas/mpt3sas_transport.c
index 32db07ed808a..1041ee7591bf 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c
@@ -1176,6 +1176,11 @@ mpt3sas_transport_update_links(struct MPT3SAS_ADAPTER *ioc,
 		return;
 	}
 
+	if (phy_number >= sas_node->nr_phys_allocated) {
+		spin_unlock_irqrestore(&ioc->sas_node_lock, flags);
+		return;
+	}
+
 	mpt3sas_phy = &sas_node->phy[phy_number];
 	mpt3sas_phy->attached_handle = handle;
 	spin_unlock_irqrestore(&ioc->sas_node_lock, flags);
-- 
2.53.0

Re: [PATCH 23/37] scsi: mpt3sas: Validate phy_number in mpt3sas_transport_update_links

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: mpt3sas: nr_phys_allocated check breaks mpt3sas_transport_update_links()
--

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

scsi: mpt3sas: Validate phy_number in mpt3sas_transport_update_links

This commit adds a bounds check on phy_number in
mpt3sas_transport_update_links() before indexing sas_node->phy.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c b/drivers/scsi/mpt3sas/mpt3sas_transport.c
> index 32db07ed808a9..1041ee7591bfb 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c
> @@ -1176,6 +1176,11 @@ mpt3sas_transport_update_links(struct MPT3SAS_ADAPTER *ioc,
>  		return;
>  	}
>  
> +	if (phy_number >= sas_node->nr_phys_allocated) {
> +		spin_unlock_irqrestore(&ioc->sas_node_lock, flags);
> +		return;
> +	}

[Severity: High]
Should this check against sas_node->num_phys (as described in the commit
message) instead of sas_node->nr_phys_allocated?

In mpt3sas_transport_update_links(), _transport_sas_node_find_by_sas_address()
returns either &ioc->sas_hba or an expander struct _sas_node:

drivers/scsi/mpt3sas/mpt3sas_transport.c:_transport_sas_node_find_by_sas_address()
	if (ioc->sas_hba.sas_address == sas_address)
		return &ioc->sas_hba;
	else
		return mpt3sas_scsih_expander_find_by_sas_address(ioc,
		    sas_address, port);

For expander nodes allocated in _scsih_expander_add(), the phy array is sized
using sas_expander->num_phys, while nr_phys_allocated is never initialized and
remains 0:

drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsih_expander_add()
	sas_expander->num_phys = expander_pg0.NumPhys;
	...
	sas_expander->phy = kzalloc_objs(struct _sas_phy,
					 sas_expander->num_phys);

Because phy_number is unsigned, phy_number >= sas_node->nr_phys_allocated is
always true for expanders, causing callers such as
_scsih_refresh_expander_links() and _scsih_sas_topology_change_event() to
return early without updating expander phy links or adding ports for attached
devices.

Additionally, for ioc->sas_hba, _scsih_sas_host_add() sets nr_phys_allocated
to at least MPT_MAX_HBA_NUM_PHYS (32) while only initializing num_phys
entries:

drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsih_sas_host_add()
	ioc->sas_hba.nr_phys_allocated = max_t(u8,
	    MPT_MAX_HBA_NUM_PHYS, num_phys);
	ioc->sas_hba.phy = kzalloc_objs(struct _sas_phy,
					ioc->sas_hba.nr_phys_allocated);
	...
	ioc->sas_hba.num_phys = num_phys;

Would a phy_number between ioc->sas_hba.num_phys and
ioc->sas_hba.nr_phys_allocated index an uninitialized _sas_phy entry where
mpt3sas_phy->phy is NULL, leading to a NULL pointer dereference in
mpt3sas_transport_add_phy_to_an_existing_port() -> _transport_add_phy() or in
the MPT_DEBUG_TRANSPORT dev_printk() call?

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

[PATCH 24/37] scsi: mpt3sas: Require CAP_SYS_RAWIO for mpt2ctl/mpt3ctl ioctls

[Shivasharan S]

Gate the ioctl entry point on CAP_SYS_RAWIO rather than relying only
on device node permissions.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index 6df51aa7bcaa..60815302daf7 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -56,6 +56,7 @@
 
 #include <linux/io.h>
 #include <linux/uaccess.h>
+#include <linux/capability.h>
 
 #include "mpt3sas_base.h"
 #include "mpt3sas_ctl.h"
@@ -2734,6 +2735,9 @@ _ctl_ioctl_main(struct file *file, unsigned int cmd, void __user *arg,
 	enum block_state state;
 	long ret = -ENOIOCTLCMD;
 
+	if (!capable(CAP_SYS_RAWIO))
+		return -EPERM;
+
 	/* get IOCTL header */
 	if (copy_from_user(&ioctl_header, (char __user *)arg,
 	    sizeof(struct mpt3_ioctl_header))) {
-- 
2.53.0

[PATCH 25/37] scsi: mpt3sas: Fix SGE offset boundary check in _ctl_do_mpt_command

[Shivasharan S]

Ensure data_sge_offset leaves room for two SGE entries within the request
frame by checking that data_sge_offset * 4 does not exceed request_sz - 32.
Reorder the overflow guard before the multiplication to prevent wraparound.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index 60815302daf7..04555eaa09c4 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -714,6 +714,7 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command karg,
 	unsigned long timeout;
 	u8 issue_reset;
 	u32 sz, sz_arg;
+	u32 sge_cnt;
 	void *psge;
 	void *data_out = NULL;
 	dma_addr_t data_out_dma = 0;
@@ -746,8 +747,9 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command karg,
 	}
 
 	/* Check for overflow and wraparound */
-	if (karg.data_sge_offset * 4 > ioc->request_sz ||
-	    karg.data_sge_offset > (UINT_MAX / 4)) {
+	if (karg.data_sge_offset > (UINT_MAX / 4) ||
+	    karg.data_sge_offset * 4 >
+	    ioc->request_sz - max_t(u32, ioc->sge_size, ioc->sge_size_ieee)) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -782,6 +784,23 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command karg,
 	data_out_sz = karg.data_out_size;
 	data_in_sz = karg.data_in_size;
 
+	/*
+	 * SGEs follow the request header and must stay inside the frame.
+	 * MCTP, mem-move and bidirectional data need two, others need one.
+	 */
+	sge_cnt = (data_out_sz && data_in_sz) ? 2 : 1;
+	if (mpi_request->Function == MPI2_FUNCTION_MCTP_PASSTHROUGH ||
+	    (mpi_request->Function == MPI2_FUNCTION_TOOLBOX &&
+	    ((Mpi2ToolboxCleanRequest_t *)mpi_request)->Tool ==
+	    MPI2_TOOLBOX_MEMORY_MOVE_TOOL))
+		sge_cnt = 2;
+	if (karg.data_sge_offset * 4 + sge_cnt *
+	    max_t(u32, ioc->sge_size, ioc->sge_size_ieee) > ioc->request_sz) {
+		mpt3sas_base_free_smid(ioc, smid);
+		ret = -EINVAL;
+		goto out;
+	}
+
 	if (mpi_request->Function == MPI2_FUNCTION_SCSI_IO_REQUEST ||
 	    mpi_request->Function == MPI2_FUNCTION_RAID_SCSI_IO_PASSTHROUGH ||
 	    mpi_request->Function == MPI2_FUNCTION_SCSI_TASK_MGMT ||
-- 
2.53.0

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

[Shivasharan S]

In _base_allocate_memory_pools(), reject a firmware-reported ReplyFrameSize
smaller than MPI2DefaultReply_t and validate IOCMaxChainSegmentSize to avoid
integer overflow and out-of-bounds access during SGL construction.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 39 +++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 3622d8a42725..1af9dc241666 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;
+	}
+
+	/* 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) {
+				ioc_err(ioc,
+					"IOCMaxChainSegmentSize (%u) is out of range, cannot safely attach\n",
+					facts->IOCMaxChainSegmentSize);
+				return -EINVAL;
+			}
+			ioc->chain_segment_sz = chain_sz;
+		} else
+			/* set to 128 bytes size if IOCMaxChainSegmentSize is zero */
 			ioc->chain_segment_sz = DEFAULT_NUM_FWCHAIN_ELEMTS *
 						    MAX_CHAIN_ELEMT_SZ;
 	} else
 		ioc->chain_segment_sz = ioc->request_sz;
 
-	/* calculate the max scatter element size */
-	sge_size = max_t(u16, ioc->sge_size, ioc->sge_size_ieee);
+	/* every branch above must yield a usable chain segment size */
+	if (ioc->chain_segment_sz < 2 * sge_size) {
+		ioc_err(ioc,
+			"invalid chain segment size (%u), cannot safely attach\n",
+			ioc->chain_segment_sz);
+		return -EINVAL;
+	}
 
  retry_allocation:
 	total_sz = 0;
-- 
2.53.0

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

[sashiko-bot]

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

[PATCH 27/37] scsi: mpt3sas: Replace panic() with error return in hard_reset_handler

[Shivasharan S]

When rdpq_array_capable is cleared after a firmware flash, replace the
panic() call with an ioc_err() log and a graceful -EFAULT return.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 1af9dc241666..7cc849dbf706 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -9069,10 +9069,13 @@ mpt3sas_base_hard_reset_handler(struct MPT3SAS_ADAPTER *ioc,
 		    " image and it requires system reboot\n");
 		goto out;
 	}
-	if (ioc->rdpq_array_enable && !ioc->rdpq_array_capable)
-		panic("%s: Issue occurred with flashing controller firmware."
-		      "Please reboot the system and ensure that the correct"
-		      " firmware version is running\n", ioc->name);
+	if (ioc->rdpq_array_enable && !ioc->rdpq_array_capable) {
+		ioc_err(ioc,
+			"%s: Issue occurred with flashing controller firmware. Please reboot the system and ensure that the correct firmware version is running\n",
+		    __func__);
+		r = -EFAULT;
+		goto out;
+	}
 
 	r = _base_make_ioc_operational(ioc);
 	if (!r)
-- 
2.53.0

[PATCH 28/37] scsi: mpt3sas: Replace panic() calls with error returns in _config_request

[Shivasharan S]

Replace the four panic() calls triggered by firmware page-type mismatches
with ioc_err() messages and -EIO returns via goto free_mem.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_config.c | 42 +++++++++++++++++----------
 1 file changed, 27 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_config.c b/drivers/scsi/mpt3sas/mpt3sas_config.c
index 2b5e42255d3b..271d7dff7c07 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_config.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_config.c
@@ -416,10 +416,13 @@ _config_request(struct MPT3SAS_ADAPTER *ioc, Mpi2ConfigRequest_t
 				    smid, "config_request", NULL);
 			_debug_dump_mf(mpi_request, ioc->request_sz/4);
 			_debug_dump_reply(mpi_reply, ioc->reply_sz/4);
-			panic("%s: %s: Firmware BUG: mpi_reply mismatch: Requested PageType(0x%02x) Reply PageType(0x%02x)\n",
-			      ioc->name, __func__,
-			      mpi_request->Header.PageType & 0xF,
-			      mpi_reply->Header.PageType & 0xF);
+			ioc_err(ioc,
+				"%s: Firmware BUG: mpi_reply mismatch: Requested PageType(0x%02x) Reply PageType(0x%02x)\n",
+				__func__,
+				mpi_request->Header.PageType & 0xF,
+				mpi_reply->Header.PageType & 0xF);
+			r = -EIO;
+			goto free_mem;
 		}
 
 		if (((mpi_request->Header.PageType & 0xF) ==
@@ -430,10 +433,13 @@ _config_request(struct MPT3SAS_ADAPTER *ioc, Mpi2ConfigRequest_t
 				    smid, "config_request", NULL);
 			_debug_dump_mf(mpi_request, ioc->request_sz/4);
 			_debug_dump_reply(mpi_reply, ioc->reply_sz/4);
-			panic("%s: %s: Firmware BUG: mpi_reply mismatch: Requested ExtPageType(0x%02x) Reply ExtPageType(0x%02x)\n",
-			      ioc->name, __func__,
-			      mpi_request->ExtPageType,
-			      mpi_reply->ExtPageType);
+			ioc_err(ioc,
+				"%s: Firmware BUG: mpi_reply mismatch: Requested ExtPageType(0x%02x) Reply ExtPageType(0x%02x)\n",
+				__func__,
+				mpi_request->ExtPageType,
+				mpi_reply->ExtPageType);
+			r = -EIO;
+			goto free_mem;
 		}
 		ioc_status = le16_to_cpu(mpi_reply->IOCStatus)
 		    & MPI2_IOCSTATUS_MASK;
@@ -459,10 +465,13 @@ _config_request(struct MPT3SAS_ADAPTER *ioc, Mpi2ConfigRequest_t
 				_debug_dump_reply(mpi_reply, ioc->reply_sz/4);
 				_debug_dump_config(p, min_t(u16, mem.sz,
 				    config_page_sz)/4);
-				panic("%s: %s: Firmware BUG: config page mismatch: Requested PageType(0x%02x) Reply PageType(0x%02x)\n",
-				      ioc->name, __func__,
-				      mpi_request->Header.PageType & 0xF,
-				      p[3] & 0xF);
+				ioc_err(ioc,
+					"%s: Firmware BUG: config page mismatch: Requested PageType(0x%02x) Reply PageType(0x%02x)\n",
+					__func__,
+					mpi_request->Header.PageType & 0xF,
+					p[3] & 0xF);
+				r = -EIO;
+				goto free_mem;
 			}
 
 			if (((mpi_request->Header.PageType & 0xF) ==
@@ -475,9 +484,12 @@ _config_request(struct MPT3SAS_ADAPTER *ioc, Mpi2ConfigRequest_t
 				_debug_dump_reply(mpi_reply, ioc->reply_sz/4);
 				_debug_dump_config(p, min_t(u16, mem.sz,
 				    config_page_sz)/4);
-				panic("%s: %s: Firmware BUG: config page mismatch: Requested ExtPageType(0x%02x) Reply ExtPageType(0x%02x)\n",
-				      ioc->name, __func__,
-				      mpi_request->ExtPageType, p[6]);
+				ioc_err(ioc,
+					"%s: Firmware BUG: config page mismatch: Requested ExtPageType(0x%02x) Reply ExtPageType(0x%02x)\n",
+					__func__,
+					mpi_request->ExtPageType, p[6]);
+				r = -EIO;
+				goto free_mem;
 			}
 		}
 		memcpy(config_page, mem.page, min_t(u16, mem.sz,
-- 
2.53.0

Re: [PATCH 28/37] scsi: mpt3sas: Replace panic() calls with error returns in _config_request

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: mpt3sas: corrupted mpi_reply used in NVRAM write after -EIO return
--

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

scsi: mpt3sas: Replace panic() calls with error returns in _config_request

This commit replaces four panic() calls in _config_request() triggered by
firmware reply or configuration page-type mismatches with ioc_err() logs
and -EIO error returns.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_config.c b/drivers/scsi/mpt3sas/mpt3sas_config.c
> index 2b5e42255d3b5..271d7dff7c079 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_config.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_config.c
> @@ -416,10 +416,13 @@ _config_request(struct MPT3SAS_ADAPTER *ioc, Mpi2ConfigRequest_t
>  				    smid, "config_request", NULL);
>  			_debug_dump_mf(mpi_request, ioc->request_sz/4);
>  			_debug_dump_reply(mpi_reply, ioc->reply_sz/4);
> -			panic("%s: %s: Firmware BUG: mpi_reply mismatch: Requested PageType(0x%02x) Reply PageType(0x%02x)\n",
> -			      ioc->name, __func__,
> -			      mpi_request->Header.PageType & 0xF,
> -			      mpi_reply->Header.PageType & 0xF);
> +			ioc_err(ioc,
> +				"%s: Firmware BUG: mpi_reply mismatch: Requested PageType(0x%02x) Reply PageType(0x%02x)\n",
> +				__func__,
> +				mpi_request->Header.PageType & 0xF,
> +				mpi_reply->Header.PageType & 0xF);
> +			r = -EIO;
> +			goto free_mem;
>  		}
>  
>  		if (((mpi_request->Header.PageType & 0xF) ==
> @@ -430,10 +433,13 @@ _config_request(struct MPT3SAS_ADAPTER *ioc, Mpi2ConfigRequest_t
>  				    smid, "config_request", NULL);
>  			_debug_dump_mf(mpi_request, ioc->request_sz/4);
>  			_debug_dump_reply(mpi_reply, ioc->reply_sz/4);
> -			panic("%s: %s: Firmware BUG: mpi_reply mismatch: Requested ExtPageType(0x%02x) Reply ExtPageType(0x%02x)\n",
> -			      ioc->name, __func__,
> -			      mpi_request->ExtPageType,
> -			      mpi_reply->ExtPageType);
> +			ioc_err(ioc,
> +				"%s: Firmware BUG: mpi_reply mismatch: Requested ExtPageType(0x%02x) Reply ExtPageType(0x%02x)\n",
> +				__func__,
> +				mpi_request->ExtPageType,
> +				mpi_reply->ExtPageType);
> +			r = -EIO;
> +			goto free_mem;
>  		}

[Severity: High]
Can returning -EIO here after overwriting mpi_reply cause subsequent
NVRAM writes to target the wrong configuration page?

In _config_request(), the caller's mpi_reply buffer is overwritten before
the PageType and ExtPageType sanity checks run:

	if (ioc->config_cmds.status & MPT3_CMD_REPLY_VALID) {
		memcpy(mpi_reply, ioc->config_cmds.reply,
		    sizeof(Mpi2ConfigReply_t));

Previously, a mismatch here called panic(). With -EIO returned instead,
callers such as mpt3sas_config_set_sas_iounit_pg1() and
_config_set_driver_trigger_pg0() through _config_set_driver_trigger_pg4()
do not check the return value of the MPI2_CONFIG_ACTION_PAGE_WRITE_CURRENT
call before issuing MPI2_CONFIG_ACTION_PAGE_WRITE_NVRAM with the same
mpi_reply:

	mpi_request.Action = MPI2_CONFIG_ACTION_PAGE_WRITE_CURRENT;
	_config_request(ioc, &mpi_request, mpi_reply,
	    MPT3_CONFIG_PAGE_DEFAULT_TIMEOUT, config_page, sz);
	mpi_request.Action = MPI2_CONFIG_ACTION_PAGE_WRITE_NVRAM;
	r = _config_request(ioc, &mpi_request, mpi_reply,
	    MPT3_CONFIG_PAGE_DEFAULT_TIMEOUT, config_page, sz);

When the second _config_request() call begins with a non-NULL config_page,
it copies the header fields from the mismatched mpi_reply into mpi_request:

	if (config_page) {
		mpi_request->Header.PageVersion = mpi_reply->Header.PageVersion;
		mpi_request->Header.PageNumber = mpi_reply->Header.PageNumber;
		mpi_request->Header.PageType = mpi_reply->Header.PageType;
		mpi_request->Header.PageLength = mpi_reply->Header.PageLength;
		mpi_request->ExtPageLength = mpi_reply->ExtPageLength;
		mpi_request->ExtPageType = mpi_reply->ExtPageType;

Wouldn't this send the MPI2_CONFIG_ACTION_PAGE_WRITE_NVRAM request to
firmware with the corrupted page header from the mismatched reply?

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

[PATCH 29/37] scsi: mpt3sas: Bound event-log copy size against reply frame size

[Shivasharan S]

Cap the event data copy in mpt3sas_ctl_add_to_event_log() against the
space available in the reply frame, not just MPT3_EVENT_DATA_SIZE. Treat
frames too small to hold the Mpi2EventNotificationReply_t header as having
nothing to copy to avoid underflow in the size calculation.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index 04555eaa09c4..a8e92a786265 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -366,8 +366,19 @@ mpt3sas_ctl_add_to_event_log(struct MPT3SAS_ADAPTER *ioc,
 		event_log[i].event = event;
 		event_log[i].context = ioc->event_context++;
 
+		/*
+		 * Bound the copy by the reply frame ctl owns, not just the
+		 * log entry size, in case the scsih-side clamp was skipped.
+		 * Skip the copy when the frame is too small to hold the
+		 * header, since computing a size from it would wrap.
+		 */
 		event_data_sz = le16_to_cpu(mpi_reply->EventDataLength)*4;
-		sz = min_t(u32, event_data_sz, MPT3_EVENT_DATA_SIZE);
+		if (ioc->reply_sz > offsetof(Mpi2EventNotificationReply_t, EventData))
+			sz = min_t(u32, event_data_sz,
+				   min_t(u32, (u32)MPT3_EVENT_DATA_SIZE,
+				   ioc->reply_sz - offsetof(Mpi2EventNotificationReply_t, EventData)));
+		else
+			sz = 0;
 		memset(event_log[i].data, 0, MPT3_EVENT_DATA_SIZE);
 		memcpy(event_log[i].data, mpi_reply->EventData, sz);
 		send_aen = 1;
-- 
2.53.0

[PATCH 30/37] scsi: mpt3sas: Add _ctl_diag_buffer_reset() helper to clear diag buffer state

[Shivasharan S]

Consolidate open-coded diag buffer teardown sequences across
_ctl_diag_register_2(), _ctl_diag_unregister(), and
_ctl_host_trace_buffer_enable_store() into a single helper that clears
the pointer, size, DMA address, status, and unique ID together.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c | 32 +++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index a8e92a786265..6870172c5ebb 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -1651,6 +1651,28 @@ _ctl_diag_get_bufftype(struct MPT3SAS_ADAPTER *ioc, u32 unique_id)
 	return MPT3_DIAG_UID_NOT_FOUND;
 }
 
+/**
+ * _ctl_diag_buffer_reset - clear every field describing a diag buffer
+ * @ioc: per adapter object
+ * @buffer_type: diag buffer type
+ *
+ * Call this wherever a diag buffer's DMA memory is freed, so its size and
+ * DMA address never outlive the pointer that describes them.
+ */
+static void
+_ctl_diag_buffer_reset(struct MPT3SAS_ADAPTER *ioc, u8 buffer_type)
+{
+	ioc->diag_buffer[buffer_type] = NULL;
+	ioc->diag_buffer_sz[buffer_type] = 0;
+	ioc->diag_buffer_dma[buffer_type] = 0;
+	ioc->diag_buffer_status[buffer_type] = 0;
+	ioc->unique_id[buffer_type] = 0;
+	if (buffer_type == MPI2_DIAG_BUF_TYPE_TRACE) {
+		ioc->ring_buffer_sz = 0;
+		ioc->ring_buffer_offset = 0;
+	}
+}
+
 /**
  * _ctl_diag_register_2 - wrapper for registering diag buffer support
  * @ioc: per adapter object
@@ -1904,9 +1926,7 @@ _ctl_diag_register_2(struct MPT3SAS_ADAPTER *ioc,
 	if (rc && request_data) {
 		dma_free_coherent(&ioc->pdev->dev, request_data_sz,
 		    request_data, request_data_dma);
-		ioc->diag_buffer[buffer_type] = NULL;
-		ioc->diag_buffer_status[buffer_type] &=
-		    ~MPT3_DIAG_BUFFER_IS_DRIVER_ALLOCATED;
+		_ctl_diag_buffer_reset(ioc, buffer_type);
 	}
 
 	ioc->ctl_cmds.status = MPT3_CMD_NOT_USED;
@@ -2127,8 +2147,7 @@ _ctl_diag_unregister(struct MPT3SAS_ADAPTER *ioc, void __user *arg)
 		request_data_dma = ioc->diag_buffer_dma[buffer_type];
 		dma_free_coherent(&ioc->pdev->dev, request_data_sz,
 				request_data, request_data_dma);
-		ioc->diag_buffer[buffer_type] = NULL;
-		ioc->diag_buffer_status[buffer_type] = 0;
+		_ctl_diag_buffer_reset(ioc, buffer_type);
 	}
 	return 0;
 }
@@ -3862,8 +3881,7 @@ host_trace_buffer_enable_store(struct device *cdev,
 						  ioc->diag_buffer_sz[MPI2_DIAG_BUF_TYPE_TRACE],
 						  ioc->diag_buffer[MPI2_DIAG_BUF_TYPE_TRACE],
 						  ioc->diag_buffer_dma[MPI2_DIAG_BUF_TYPE_TRACE]);
-				ioc->diag_buffer[MPI2_DIAG_BUF_TYPE_TRACE] =
-				    NULL;
+				_ctl_diag_buffer_reset(ioc, MPI2_DIAG_BUF_TYPE_TRACE);
 			}
 
 			diag_register.requested_buffer_size = (1024 * 1024);
-- 
2.53.0

[PATCH 31/37] scsi: mpt3sas: Restrict ioc_dump debugfs and replace raw ioc dump with text

[Shivasharan S]

The ioc_dump debugfs file previously exposed raw MPT3SAS_ADAPTER bytes,
leaking kernel addresses, and was world-readable.

Create the file with mode 0400 instead of 0444, replace the raw dump
with a small human-readable text summary of key adapter properties, and
fix the memory leak in the release path.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_debugfs.c | 49 +++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_debugfs.c b/drivers/scsi/mpt3sas/mpt3sas_debugfs.c
index 183391f6b8d1..432e120ac81d 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_debugfs.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_debugfs.c
@@ -49,22 +49,62 @@ _debugfs_iocdump_read(struct file *filp, char __user *ubuf, size_t cnt,
 	return simple_read_from_buffer(ubuf, cnt, ppos, debug->buf, debug->len);
 }
 
+/* Buffer size for the text dump */
+#define MPT3SAS_IOCDUMP_BUF_SZ 1024
+
 /*
- * _debugfs_iocdump_open :	open the ioc_dump debugfs attribute file
+ * _debugfs_iocdump_open: open the ioc_dump debugfs attribute file.
+ * Populates the debugfs buffer with a summary of key adapter properties.
  */
 static int
 _debugfs_iocdump_open(struct inode *inode, struct file *file)
 {
 	struct MPT3SAS_ADAPTER *ioc = inode->i_private;
 	struct mpt3sas_debugfs_buffer *debug;
+	char *buf;
+	size_t len = 0;
 
 	debug = kzalloc_obj(struct mpt3sas_debugfs_buffer);
 	if (!debug)
 		return -ENOMEM;
 
-	debug->buf = (void *)ioc;
-	debug->len = sizeof(struct MPT3SAS_ADAPTER);
+	buf = kzalloc(MPT3SAS_IOCDUMP_BUF_SZ, GFP_KERNEL);
+	if (!buf) {
+		kfree(debug);
+		return -ENOMEM;
+	}
+
+	len += scnprintf(buf + len, MPT3SAS_IOCDUMP_BUF_SZ - len,
+	    "name:           %s\n", ioc->name);
+	len += scnprintf(buf + len, MPT3SAS_IOCDUMP_BUF_SZ - len,
+	    "fw_version:     %02d.%02d.%02d.%02d\n",
+	    (ioc->facts.FWVersion.Word & 0xFF000000) >> 24,
+	    (ioc->facts.FWVersion.Word & 0x00FF0000) >> 16,
+	    (ioc->facts.FWVersion.Word & 0x0000FF00) >> 8,
+	    ioc->facts.FWVersion.Word & 0x000000FF);
+	len += scnprintf(buf + len, MPT3SAS_IOCDUMP_BUF_SZ - len,
+	    "request_sz:     %d\n", ioc->request_sz);
+	len += scnprintf(buf + len, MPT3SAS_IOCDUMP_BUF_SZ - len,
+	    "reply_sz:       %d\n", ioc->reply_sz);
+	len += scnprintf(buf + len, MPT3SAS_IOCDUMP_BUF_SZ - len,
+	    "chain_segment_sz: %d\n", ioc->chain_segment_sz);
+	len += scnprintf(buf + len, MPT3SAS_IOCDUMP_BUF_SZ - len,
+	    "hba_queue_depth: %d\n", ioc->hba_queue_depth);
+	len += scnprintf(buf + len, MPT3SAS_IOCDUMP_BUF_SZ - len,
+	    "scsiio_depth:   %d\n", ioc->scsiio_depth);
+	len += scnprintf(buf + len, MPT3SAS_IOCDUMP_BUF_SZ - len,
+	    "max_dev_handle: %d\n", ioc->facts.MaxDevHandle);
+	len += scnprintf(buf + len, MPT3SAS_IOCDUMP_BUF_SZ - len,
+	    "shost_recovery: %d\n", ioc->shost_recovery);
+	len += scnprintf(buf + len, MPT3SAS_IOCDUMP_BUF_SZ - len,
+	    "pci_error_recovery: %d\n", ioc->pci_error_recovery);
+	len += scnprintf(buf + len, MPT3SAS_IOCDUMP_BUF_SZ - len,
+	    "remove_host:    %d\n", ioc->remove_host);
+
+	debug->buf = buf;
+	debug->len = len;
 	file->private_data = debug;
+
 	return 0;
 }
 
@@ -82,6 +122,7 @@ _debugfs_iocdump_release(struct inode *inode, struct file *file)
 		return 0;
 
 	file->private_data = NULL;
+	kfree(debug->buf);
 	kfree(debug);
 	return 0;
 }
@@ -132,7 +173,7 @@ mpt3sas_setup_debugfs(struct MPT3SAS_ADAPTER *ioc)
 	}
 
 	snprintf(name, sizeof(name), "ioc_dump");
-	ioc->ioc_dump =	debugfs_create_file(name, 0444,
+	ioc->ioc_dump =	debugfs_create_file(name, 0400,
 	    ioc->debugfs_root, ioc, &mpt3sas_debugfs_iocdump_fops);
 	if (!ioc->ioc_dump) {
 		dev_err(&ioc->pdev->dev,
-- 
2.53.0

[PATCH 32/37] scsi: mpt3sas: Fix out-of-bounds bit operation in _scsih_check_topo_delete_events

[Shivasharan S]

When find_first_bit() returns a value >= MaxDevHandle (no bit set), the
loop called test_and_clear_bit() with the out-of-range index. Break out
immediately when the returned position is >= MaxDevHandle.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index df5f16db8f48..4585b666d4bb 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -5016,8 +5016,9 @@ _scsih_check_topo_delete_events(struct MPT3SAS_ADAPTER *ioc,
 		do {
 			handle = find_first_bit(ioc->blocking_handles,
 			    ioc->facts.MaxDevHandle);
-			if (handle < ioc->facts.MaxDevHandle)
-				_scsih_block_io_device(ioc, handle);
+			if (handle >= ioc->facts.MaxDevHandle)
+				break;
+			_scsih_block_io_device(ioc, handle);
 		} while (test_and_clear_bit(handle, ioc->blocking_handles));
 	} else if (event_data->ExpStatus == MPI2_EVENT_SAS_TOPO_ES_RESPONDING)
 		_scsih_block_io_to_children_attached_directly(ioc, event_data);
-- 
2.53.0

[PATCH 33/37] scsi: mpt3sas: Bound SMP passthrough buffer reads to actual buffer size

[Shivasharan S]

The SMP passthrough handler reads bytes at offsets 1 and 10 of the data
buffer to detect link-reset commands without checking the buffer length.
Guard each read against the actual buffer size to prevent out-of-bounds
access on undersized non-inline buffers.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index 6870172c5ebb..c80bb2e7c240 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -996,13 +996,15 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command karg,
 		Mpi2SmpPassthroughRequest_t *smp_request =
 		    (Mpi2SmpPassthroughRequest_t *)mpi_request;
 		u8 *data;
+		bool inline_data;
 
 		if (!ioc->multipath_on_hba) {
 			/* ioc determines which port to use */
 			smp_request->PhysicalPort = 0xFF;
 		}
-		if (smp_request->PassthroughFlags &
-		    MPI2_SMP_PT_REQ_PT_FLAGS_IMMEDIATE)
+		inline_data = smp_request->PassthroughFlags &
+		    MPI2_SMP_PT_REQ_PT_FLAGS_IMMEDIATE;
+		if (inline_data)
 			data = (u8 *)&smp_request->SGL;
 		else {
 			if (unlikely(data_out == NULL)) {
@@ -1015,7 +1017,10 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command karg,
 			data = data_out;
 		}
 
-		if (data[1] == 0x91 && (data[10] == 1 || data[10] == 2)) {
+		/* Bound the read below to the caller-supplied buffer size. */
+		if ((inline_data || data_out_sz > 1) && data[1] == 0x91 &&
+		    (inline_data || data_out_sz > 10) &&
+		    (data[10] == 1 || data[10] == 2)) {
 			ioc->ioc_link_reset_in_progress = 1;
 			ioc->ignore_loginfos = 1;
 		}
-- 
2.53.0

[PATCH 34/37] scsi: mpt3sas: Release sas_device reference on error in mpt3sas_transport_port_add

[Shivasharan S]

The reference taken by mpt3sas_get_sdev_by_addr() is never released on
error paths that branch to out_fail. Add sas_device_put() there to
prevent a reference count leak.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_transport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c b/drivers/scsi/mpt3sas/mpt3sas_transport.c
index 1041ee7591bf..4ae11f226014 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c
@@ -866,6 +866,8 @@ mpt3sas_transport_port_add(struct MPT3SAS_ADAPTER *ioc, u16 handle,
 	sas_port_delete(port);
 
 out_fail:
+	if (sas_device)
+		sas_device_put(sas_device);
 	list_for_each_entry_safe(mpt3sas_phy, next, &mpt3sas_port->phy_list,
 	    port_siblings)
 		list_del(&mpt3sas_phy->port_siblings);
-- 
2.53.0

[PATCH 35/37] scsi: mpt3sas: NULL the event_log pointer after freeing it

[Shivasharan S]

Set ioc->event_log = NULL immediately after kfree() in
mpt3sas_ctl_release() to prevent use-after-free if a concurrent path
dereferences the pointer during adapter teardown.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index c80bb2e7c240..df9674fa2af1 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -4615,6 +4615,7 @@ mpt3sas_ctl_release(struct MPT3SAS_ADAPTER *ioc)
 	}
 
 	kfree(ioc->event_log);
+	ioc->event_log = NULL;
 }
 
 /**
-- 
2.53.0

[PATCH 36/37] scsi: mpt3sas: Bound NVMe passthrough transfer size against PRP buffer capacity

[Shivasharan S]

The NVMe ioctl passthrough path builds a PRP list into a fixed per-command
buffer sized as sg_tablesize * page_size. Reject requests where either
data_in_size or data_out_size exceeds that limit to prevent overrunning
the buffer and corrupting adjacent DMA allocations.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index df9674fa2af1..1087802368c5 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -899,6 +899,21 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command karg,
 			ret = -EINVAL;
 			goto out;
 		}
+		/*
+		 * Reject an oversized transfer, since building the PRP list
+		 * would overrun this command's fixed buffer, corrupting
+		 * others sharing it.
+		 */
+		if (data_out_sz > (ioc->shost->sg_tablesize * ioc->page_size) ||
+		    data_in_sz > (ioc->shost->sg_tablesize * ioc->page_size)) {
+			dtmprintk(ioc, ioc_info(ioc,
+				  "%s: data_in_size/data_out_size exceeds NVMe PRP buffer capacity\n",
+				  __func__));
+			mpt3sas_base_free_smid(ioc, smid);
+			ret = -EINVAL;
+			goto out;
+		}
+
 		/*
 		 * Get the Physical Address of the sense buffer.
 		 * Use Error Response buffer address field to hold the sense
-- 
2.53.0

[PATCH 37/37] scsi: mpt3sas: Update driver version to 59.100.00.00

[Shivasharan S]

Updated driver version to 59.100.00.00.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h
index 192b8e8a5b4d..f88f85b054cb 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -77,8 +77,8 @@
 #define MPT3SAS_DRIVER_NAME		"mpt3sas"
 #define MPT3SAS_AUTHOR "Avago Technologies <MPT-FusionLinux.pdl@avagotech.com>"
 #define MPT3SAS_DESCRIPTION	"LSI MPT Fusion SAS 3.0 Device Driver"
-#define MPT3SAS_DRIVER_VERSION		"54.100.00.00"
-#define MPT3SAS_MAJOR_VERSION		54
+#define MPT3SAS_DRIVER_VERSION		"59.100.00.00"
+#define MPT3SAS_MAJOR_VERSION		59
 #define MPT3SAS_MINOR_VERSION		100
 #define MPT3SAS_BUILD_VERSION		00
 #define MPT3SAS_RELEASE_VERSION		00
-- 
2.53.0

Re: [PATCH 37/37] scsi: mpt3sas: Update driver version to 59.100.00.00

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] scsi: mpt3sas: expander link updates broken in mpt3sas_transport_update_links()
- [High] scsi: mpt3sas: infinite retry loop on invalid handle in _scsih_add_device()
- [High] scsi: mpt3sas: out-of-bounds read on short events in mpt3sas_scsih_event_callback()
- [Medium] scsi: mpt3sas: u16 underflow of scsiio_depth in _base_allocate_memory_pools()
- [Medium] scsi: mpt3sas: signed count bypasses clamp in driver trigger page helpers
- [Medium] scsi: mpt3sas: shift and u32 multiply overflow from CurrentHostPageSize
--

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

scsi: mpt3sas: Update driver version to 59.100.00.00

This series hardens bounds, size, and handle validation across the mpt3sas
driver and updates the driver version to 59.100.00.00.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 1af25a22611a1..7cc849dbf7060 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c

[ ... ]

> @@ -6434,6 +6477,13 @@ _base_allocate_memory_pools(struct MPT3SAS_ADAPTER *ioc)
>  				(ioc->reply_post_queue_depth % 16);
>  	}
>  
> +	if (facts->MaxReplyDescriptorPostQueueDepth <
> +	    MPT3SAS_MIN_REPLY_POST_QUEUE_DEPTH) {
> +		ioc_err(ioc, "MaxReplyDescriptorPostQueueDepth is too small (%u)\n",
> +			facts->MaxReplyDescriptorPostQueueDepth);
> +		return -EINVAL;
> +	}
> +
>  	if (ioc->reply_post_queue_depth >
>  	    facts->MaxReplyDescriptorPostQueueDepth) {
>  		ioc->reply_post_queue_depth =

[Severity: Medium]
Can this minimum check still allow an unsigned underflow of ioc->scsiio_depth
further down in _base_allocate_memory_pools()?

MPT3SAS_MIN_REPLY_POST_QUEUE_DEPTH is 80, which gives ioc->hba_queue_depth =
((80 - 64) / 2) - 1 = 7 when ioc->reply_post_queue_depth is clamped to
facts->MaxReplyDescriptorPostQueueDepth.

Earlier in _base_allocate_memory_pools(), ioc->internal_depth is set to at
least INTERNAL_CMDS_COUNT (10) and ioc->hi_priority_depth is set to
ioc->internal_depth - 5 (at least 5):

drivers/scsi/mpt3sas/mpt3sas_base.c:_base_allocate_memory_pools() {
	...
	if (ioc->internal_depth < INTERNAL_CMDS_COUNT) {
		...
		ioc->internal_depth = 10;
	}

	ioc->hi_priority_depth = ioc->internal_depth - (5);
	...
	ioc->scsiio_depth = ioc->hba_queue_depth -
	    ioc->hi_priority_depth - ioc->internal_depth;

	ioc->shost->can_queue = ioc->scsiio_depth - INTERNAL_SCSIIO_CMDS_COUNT;
}

When ioc->hba_queue_depth is less than ioc->hi_priority_depth +
ioc->internal_depth + INTERNAL_SCSIIO_CMDS_COUNT (at least 18), subtracting
ioc->hi_priority_depth and ioc->internal_depth wraps the u16 ioc->scsiio_depth
around to 65528 and sets ioc->shost->can_queue to 65525.

[ ... ]

> @@ -7507,8 +7557,15 @@ _base_get_ioc_facts(struct MPT3SAS_ADAPTER *ioc)
>  	facts->CurrentHostPageSize = mpi_reply.CurrentHostPageSize;
>  
>  	/*
> -	 * Get the Page Size from IOC Facts. If it's 0, default to 4k.
> +	 * Get the Page Size from IOC Facts. If it's 0 or too large to be a
> +	 * valid shift amount, default to 4k.
>  	 */
> +	if ((facts->CurrentHostPageSize >= 32) ||
> +	    (facts->CurrentHostPageSize != 0 && facts->CurrentHostPageSize < 12)) {
> +		ioc_warn(ioc, "CurrentHostPageSize(%d) is invalid: Setting default host page size to 4k\n",
> +			 facts->CurrentHostPageSize);
> +		facts->CurrentHostPageSize = MPT3SAS_HOST_PAGE_SIZE_4K;
> +	}
>  	ioc->page_size = 1 << facts->CurrentHostPageSize;
>  	if (ioc->page_size == 1) {
>  		ioc_info(ioc, "CurrentHostPageSize is 0: Setting default host page size to 4k\n");

[Severity: Medium]
When facts->CurrentHostPageSize is 31, doesn't 1 << facts->CurrentHostPageSize
overflow a signed 32-bit int?

Also, if facts->CurrentHostPageSize is allowed up to 31, can the 32-bit
multiplication ioc->shost->sg_tablesize * ioc->page_size in
_ctl_do_mpt_command() wrap around?

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_config.c b/drivers/scsi/mpt3sas/mpt3sas_config.c
> index b0d5ef8936008..271d7dff7c079 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_config.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_config.c

[ ... ]

> @@ -2278,6 +2290,8 @@ mpt3sas_config_update_driver_trigger_pg2(struct MPT3SAS_ADAPTER *ioc,
>  
>  	if (set) {
>  		count = event_tg->ValidEntries;
> +		if (count > NUM_VALID_ENTRIES)
> +			count = NUM_VALID_ENTRIES;
>  		tg_pg2.NumMPIEventTrigger = cpu_to_le16(count);
>  		for (i = 0; i < count; i++) {
>  			tg_pg2.MPIEventTriggers[i].MPIEventCode =
> @@ -2437,6 +2451,8 @@ mpt3sas_config_update_driver_trigger_pg3(struct MPT3SAS_ADAPTER *ioc,
>  
>  	if (set) {
>  		count = scsi_tg->ValidEntries;
> +		if (count > NUM_VALID_ENTRIES)
> +			count = NUM_VALID_ENTRIES;
>  		tg_pg3.NumSCSISenseTrigger = cpu_to_le16(count);
>  		for (i = 0; i < count; i++) {
>  			tg_pg3.SCSISenseTriggers[i].ASCQ =
> @@ -2595,6 +2611,8 @@ mpt3sas_config_update_driver_trigger_pg4(struct MPT3SAS_ADAPTER *ioc,
>  
>  	if (set) {
>  		count = mpi_tg->ValidEntries;
> +		if (count > NUM_VALID_ENTRIES)
> +			count = NUM_VALID_ENTRIES;
>  		tg_pg4.NumIOCStatusLogInfoTrigger = cpu_to_le16(count);
>  		for (i = 0; i < count; i++) {
>  			tg_pg4.IOCStatusLoginfoTriggers[i].IOCStatus =

[Severity: Medium]
In mpt3sas_config_update_driver_trigger_pg2(),
mpt3sas_config_update_driver_trigger_pg3(), and
mpt3sas_config_update_driver_trigger_pg4(), count is declared as a signed int
while ValidEntries is a uint32_t.

If userspace passes ValidEntries >= 0x80000000 via the diag_trigger_event,
diag_trigger_scsi, or diag_trigger_mpi sysfs attributes, won't count become
negative, bypass the count > NUM_VALID_ENTRIES check, and write the truncated
16-bit value into NumMPIEventTrigger, NumSCSISenseTrigger, or
NumIOCStatusLogInfoTrigger?

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> index 8bb947004885b..1087802368c5b 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c

[ ... ]

> @@ -862,6 +899,21 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command karg,
>  			ret = -EINVAL;
>  			goto out;
>  		}
> +		/*
> +		 * Reject an oversized transfer, since building the PRP list
> +		 * would overrun this command's fixed buffer, corrupting
> +		 * others sharing it.
> +		 */
> +		if (data_out_sz > (ioc->shost->sg_tablesize * ioc->page_size) ||
> +		    data_in_sz > (ioc->shost->sg_tablesize * ioc->page_size)) {
> +			dtmprintk(ioc, ioc_info(ioc,
> +				  "%s: data_in_size/data_out_size exceeds NVMe PRP buffer capacity\n",
> +				  __func__));
> +			mpt3sas_base_free_smid(ioc, smid);
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +

[Severity: Medium]
Since ioc->shost->sg_tablesize is an unsigned short and ioc->page_size is a
u32, this product is evaluated in 32-bit unsigned arithmetic. If
facts->CurrentHostPageSize is large (up to 31 in _base_get_ioc_facts()), can
ioc->shost->sg_tablesize * ioc->page_size overflow 32 bits?

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index 271d7dff7c079..8bb947004885b 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c

[ ... ]

> @@ -8399,6 +8426,12 @@ _scsih_add_device(struct MPT3SAS_ADAPTER *ioc, u16 handle, u8 retry_count,
>  	u8 connector_name[5];
>  	u8 port_id;
>  
> +	if (handle >= ioc->facts.MaxDevHandle) {
> +		ioc_err(ioc, "handle(0x%04x) out of bounds in %s\n",
> +			handle, __func__);
> +		return -1;
> +	}
> +
>  	if ((mpt3sas_config_get_sas_device_pg0(ioc, &mpi_reply, &sas_device_pg0,
>  	    MPI2_SAS_DEVICE_PGAD_FORM_HANDLE, handle))) {
>  		ioc_err(ioc, "failure at %s:%d/%s()!\n",

[Severity: High]
Does returning -1 here cause callers to retry forever when handle >=
ioc->facts.MaxDevHandle?

Callers of _scsih_add_device() treat any non-zero return value as a request to
retry adding a busy device:

drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsih_scan_for_devices_after_reset() {
	...
			/* This will retry adding the end device.
			 * _scsih_add_device() will decide on retries and
			 * return "1" when it should be retried
			 */
			while (_scsih_add_device(ioc, handle, retry_count++,
			    0)) {
				ssleep(1);
			}
}

and in _scsih_sas_topology_change_event():

drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsih_sas_topology_change_event() {
	...
			rc = _scsih_add_device(ioc, handle,
			    fw_event->retries[i], 0);
			if (rc) {/* retry due to busy device */
				fw_event->retries[i]++;
				requeue_event = 1;
			}
}

Because the handle >= ioc->facts.MaxDevHandle check in _scsih_add_device()
returns -1 on every call regardless of retry_count, wouldn't
_scsih_scan_for_devices_after_reset() loop indefinitely and
_scsih_sas_topology_change_event() requeue fw_event indefinitely instead of
dropping the invalid handle by returning 0?

[ ... ]

> @@ -12144,9 +12220,21 @@ mpt3sas_scsih_event_callback(struct MPT3SAS_ADAPTER *ioc, u8 msix_index,
>  	}
>  
>  	case MPI2_EVENT_SAS_TOPOLOGY_CHANGE_LIST:
> -		_scsih_check_topo_delete_events(ioc,
> +	{
> +		Mpi2EventDataSasTopologyChangeList_t *topo_data =
>  		    (Mpi2EventDataSasTopologyChangeList_t *)
> -		    mpi_reply->EventData);
> +		    mpi_reply->EventData;
> +		u32 data_len = le16_to_cpu(mpi_reply->EventDataLength) * 4;
> +		size_t hdr_size =
> +		    offsetof(Mpi2EventDataSasTopologyChangeList_t, PHY);
> +		u16 max_entries = 0;
> +
> +		if (data_len > hdr_size)
> +			max_entries = (data_len - hdr_size) /
> +			    sizeof(Mpi2EventSasTopoPhyEntry_t);
> +		if (topo_data->NumEntries > max_entries)
> +			topo_data->NumEntries = max_entries;
> +		_scsih_check_topo_delete_events(ioc, topo_data);
>  		/*
>  		 * No need to add the topology change list
>  		 * event to fw event work queue when
> @@ -12158,18 +12246,45 @@ mpt3sas_scsih_event_callback(struct MPT3SAS_ADAPTER *ioc, u8 msix_index,
>  		if (ioc->shost_recovery)
>  			return 1;
>  		break;
> +	}
>  	case MPI2_EVENT_PCIE_TOPOLOGY_CHANGE_LIST:
> -	_scsih_check_pcie_topo_remove_events(ioc,
> +	{
> +		Mpi26EventDataPCIeTopologyChangeList_t *pcie_topo_data =
>  		    (Mpi26EventDataPCIeTopologyChangeList_t *)
> -		    mpi_reply->EventData);
> +		    mpi_reply->EventData;
> +		u32 data_len = le16_to_cpu(mpi_reply->EventDataLength) * 4;
> +		size_t hdr_size =
> +		    offsetof(Mpi26EventDataPCIeTopologyChangeList_t, PortEntry);
> +		u16 max_entries = 0;
> +
> +		if (data_len > hdr_size)
> +			max_entries = (data_len - hdr_size) /
> +			    sizeof(MPI26_EVENT_PCIE_TOPO_PORT_ENTRY);
> +		if (pcie_topo_data->NumEntries > max_entries)
> +			pcie_topo_data->NumEntries = max_entries;
> +		_scsih_check_pcie_topo_remove_events(ioc, pcie_topo_data);
>  		if (ioc->shost_recovery)
>  			return 1;
>  		break;
> +	}
>  	case MPI2_EVENT_IR_CONFIGURATION_CHANGE_LIST:
> -		_scsih_check_ir_config_unhide_events(ioc,
> +	{
> +		Mpi2EventDataIrConfigChangeList_t *ir_config_data =
>  		    (Mpi2EventDataIrConfigChangeList_t *)
> -		    mpi_reply->EventData);
> +		    mpi_reply->EventData;
> +		u32 data_len = le16_to_cpu(mpi_reply->EventDataLength) * 4;
> +		size_t hdr_size =
> +		    offsetof(Mpi2EventDataIrConfigChangeList_t, ConfigElement);
> +		u16 max_elements = 0;
> +
> +		if (data_len > hdr_size)
> +			max_elements = (data_len - hdr_size) /
> +			    sizeof(MPI2_EVENT_IR_CONFIG_ELEMENT);
> +		if (ir_config_data->NumElements > max_elements)
> +			ir_config_data->NumElements = max_elements;
> +		_scsih_check_ir_config_unhide_events(ioc, ir_config_data);
>  		break;
> +	}

[Severity: High]
What happens here if data_len < hdr_size for MPI2_EVENT_SAS_TOPOLOGY_CHANGE_LIST,
MPI2_EVENT_PCIE_TOPOLOGY_CHANGE_LIST, or MPI2_EVENT_IR_CONFIGURATION_CHANGE_LIST?

When data_len < hdr_size, the event is not rejected, so execution continues to
alloc_fw_event_work(sz) where sz = le16_to_cpu(mpi_reply->EventDataLength) * 4:

drivers/scsi/mpt3sas/mpt3sas_scsih.c:mpt3sas_scsih_event_callback() {
	...
	sz = le16_to_cpu(mpi_reply->EventDataLength) * 4;
	fw_event = alloc_fw_event_work(sz);
	...
	memcpy(fw_event->event_data, mpi_reply->EventData,
	    mpi_reply->EventDataLength * 4);
}

Because fw_event->event_data is allocated with only sz bytes (smaller than
hdr_size), won't the workqueue handlers _scsih_sas_topology_change_event(),
_scsih_pcie_topology_change_event(), and _scsih_sas_ir_config_change_event()
(as well as _scsih_check_topo_delete_events() and
_scsih_check_pcie_topo_remove_events() when walking queued fw_event_list
entries) read header fields such as ExpanderDevHandle, SwitchDevHandle,
NumEntries, or NumElements past the end of fw_event->event_data?

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c b/drivers/scsi/mpt3sas/mpt3sas_transport.c
> index e74a526efa8db..4ae11f2260143 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c

[ ... ]

> @@ -1176,6 +1178,11 @@ mpt3sas_transport_update_links(struct MPT3SAS_ADAPTER *ioc,
>  		return;
>  	}
>  
> +	if (phy_number >= sas_node->nr_phys_allocated) {
> +		spin_unlock_irqrestore(&ioc->sas_node_lock, flags);
> +		return;
> +	}
> +
>  	mpt3sas_phy = &sas_node->phy[phy_number];
>  	mpt3sas_phy->attached_handle = handle;
>  	spin_unlock_irqrestore(&ioc->sas_node_lock, flags);

[Severity: High]
Doesn't checking phy_number >= sas_node->nr_phys_allocated break link updates
for all SAS expanders?

In mpt3sas_transport_update_links(), _transport_sas_node_find_by_sas_address()
returns either &ioc->sas_hba or a SAS expander node from
ioc->sas_expander_list. While _scsih_sas_host_add() and
_scsih_sas_host_refresh() set ioc->sas_hba.nr_phys_allocated,
_scsih_expander_add() only sets sas_expander->num_phys and leaves
sas_expander->nr_phys_allocated at 0:

drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsih_expander_add() {
	...
	sas_expander = kzalloc_obj(struct _sas_node);
	...
	sas_expander->handle = handle;
	sas_expander->num_phys = expander_pg0.NumPhys;
	...
	sas_expander->phy = kzalloc_objs(struct _sas_phy,
					 sas_expander->num_phys);
}

Whenever sas_node is an expander, sas_node->nr_phys_allocated is 0, so
phy_number >= sas_node->nr_phys_allocated is always true and
mpt3sas_transport_update_links() returns early without updating the expander
phy link state.

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