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

[sashiko-bot@kernel.org]

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