Re: [GIT PULL] SCSI fixes for 7.3-rc7+

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] scsi: libiscsi: premature mgmt response frees pending task on conn->mgmtqueue
- [High] scsi: libsas: smp_execute_task_sg() skips runtime resume for sysfs callers
- [High] scsi: libsas: sas_smp_handler() fails with -EACCES when runtime PM is disabled
- [High] scsi: target: iscsi: UAF race on discovery_tpg param_list during login
--

commit f682ddc340c78ec7c00ca8f8ce335b1612f00410
Author: James Bottomley <James.Bottomley@HansenPartnership.com>

SCSI fixes for 7.3-rc7+

This pull request contains SCSI fixes for libiscsi, libsas, st, xen-scsifront,
target, and bsg.

> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 5cbc51899de00..5bfdb30b9088d 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -1142,13 +1142,15 @@ static int iscsi_handle_reject(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
>  			spin_unlock(&conn->session->frwd_lock);
>  			spin_lock(&conn->session->back_lock);
>  		} else {
> -			struct iscsi_task *task;
> +			struct iscsi_task *task = NULL;
>  			/*
>  			 * Our nop as ping got dropped. We know the target
>  			 * and transport are ok so just clean up
>  			 */
> -			task = iscsi_itt_to_task(conn, rejected_pdu.itt);
> -			if (!task) {
> +			if (!iscsi_verify_itt(conn, rejected_pdu.itt))
> +				task = iscsi_itt_to_task(conn, rejected_pdu.itt);
> +			if (!task || task->state == ISCSI_TASK_FREE ||
> +			    task->sc) {
>  				iscsi_conn_printk(KERN_ERR, conn,
>  						 "Invalid pdu reject. Could "
>  						 "not lookup rejected task.\n");

[ ... ]

> @@ -1284,7 +1286,7 @@ int __iscsi_complete_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
>  	case ISCSI_OP_SCSI_TMFUNC_RSP:
>  	case ISCSI_OP_NOOP_IN:
>  		task = iscsi_itt_to_task(conn, hdr->itt);
> -		if (!task)
> +		if (!task || task->state == ISCSI_TASK_FREE || task->sc)
>  			return ISCSI_ERR_BAD_ITT;
>  		break;
>  	default:

[Severity: Critical]
Can an unsolicited or premature management response PDU or Reject PDU free a
task while it is still queued on conn->mgmtqueue in ISCSI_TASK_PENDING state?

In drivers/scsi/libiscsi.c, iscsi_alloc_mgmt_task() initializes a management
task under session->frwd_lock with refcount = 1, task->sc = NULL, and
task->state = ISCSI_TASK_PENDING:

drivers/scsi/libiscsi.c:iscsi_alloc_mgmt_task() {
	...
	refcount_set(&task->refcount, 1);
	task->conn = conn;
	task->sc = NULL;
	INIT_LIST_HEAD(&task->running);
	task->state = ISCSI_TASK_PENDING;
}

When ihost->workq is used, iscsi_send_mgmt_task() adds task->running to
conn->mgmtqueue while task->state is still ISCSI_TASK_PENDING and refcount is
still 1:

drivers/scsi/libiscsi.c:iscsi_send_mgmt_task() {
	...
	} else {
		list_add_tail(&task->running, &conn->mgmtqueue);
		iscsi_conn_queue_xmit(conn);
	}
}

The task only transitions to ISCSI_TASK_RUNNING later when iscsi_data_xmit()
dequeues task->running from conn->mgmtqueue and calls iscsi_prep_mgmt_task().
In addition, iscsi_alloc_task() for SCSI commands runs under
session->frwd_lock (not session->back_lock) and sets task->state =
ISCSI_TASK_PENDING before assigning task->sc = sc.

Because both iscsi_handle_reject() and __iscsi_complete_pdu() only check for
task->state == ISCSI_TASK_FREE || task->sc under session->back_lock, a target
response (ISCSI_OP_NOOP_IN, ISCSI_OP_LOGOUT_RSP, ISCSI_OP_LOGIN_RSP,
ISCSI_OP_TEXT_RSP, ISCSI_OP_SCSI_TMFUNC_RSP, or ISCSI_OP_REJECT with
ISCSI_REASON_IMM_CMD_REJECT for ISCSI_OP_NOOP_OUT) naming the ITT of a task in
ISCSI_TASK_PENDING passes this check.

That calls iscsi_complete_task() -> __iscsi_put_task() -> iscsi_free_task(),
which drops task->refcount from 1 to 0 and returns the task to
session->cmdpool.queue via kfifo_in() without unlinking task->running from
conn->mgmtqueue:

drivers/scsi/libiscsi.c:iscsi_free_task() {
	...
	session->tt->cleanup_task(task);
	task->state = ISCSI_TASK_FREE;
	task->sc = NULL;
	if (conn->login_task == task)
		return;

	kfifo_in(&session->cmdpool.queue, (void*)&task, sizeof(void*));
}

When iscsi_data_xmit() subsequently processes conn->mgmtqueue or the task is
reallocated from cmdpool.queue, wouldn't this cause a use-after-free and
conn->mgmtqueue list corruption? Should both checks require task->state !=
ISCSI_TASK_RUNNING instead of task->state == ISCSI_TASK_FREE?

> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index 811c9eb4fef11..26c2099c28b97 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -62,7 +62,11 @@ static int smp_execute_task_sg(struct domain_device *dev,
>  		to_sas_internal(dev->port->ha->shost->transportt);
>  	struct sas_ha_struct *ha = dev->port->ha;
>  
> -	pm_runtime_get_sync(ha->dev);
> +	/*
> +	 * Non-blocking: a sync resume here would deadlock against
> +	 * sas_drain_work() during HA resume.
> +	 */
> +	pm_runtime_get_noresume(ha->dev);
>  	mutex_lock(&dev->ex_dev.cmd_mutex);
>  	for (retry = 0; retry < 3; retry++) {
>  		if (test_bit(SAS_DEV_GONE, &dev->state)) {
> @@ -135,7 +139,7 @@ static int smp_execute_task_sg(struct domain_device *dev,
>  		}
>  	}
>  	mutex_unlock(&dev->ex_dev.cmd_mutex);
> -	pm_runtime_put_sync(ha->dev);
> +	pm_runtime_put(ha->dev);
>  
>  	BUG_ON(retry == 3 && task != NULL);
>  	sas_free_task(task);

[Severity: High]
Does replacing pm_runtime_get_sync(ha->dev) with
pm_runtime_get_noresume(ha->dev) in smp_execute_task_sg() leave sysfs and
exported PHY management callers executing SMP tasks while the SAS host is
runtime-suspended?

Only sas_smp_handler() was updated to resume ha->dev before calling
smp_execute_task_sg(), but three other non-discovery paths invoke
smp_execute_task() -> smp_execute_task_sg() on expander PHYs
(!scsi_is_sas_phy_local(phy)) without waking ha->dev:

1. Reading expander PHY link error sysfs attributes
   (show_sas_phy_invalid_dword_count,
   show_sas_phy_running_disparity_error_count,
   show_sas_phy_loss_of_dword_sync_count, and
   show_sas_phy_phy_reset_problem_count in drivers/scsi/scsi_transport_sas.c)
   calls sas_get_linkerrors() -> sas_smp_get_phy_events() ->
   smp_execute_task() -> smp_execute_task_sg():

drivers/scsi/libsas/sas_init.c:sas_get_linkerrors() {
	if (scsi_is_sas_phy_local(phy)) {
		...
	}

	return sas_smp_get_phy_events(phy);
}

2. Writing expander PHY linkrate sysfs attributes
   (store_sas_phy_minimum_linkrate and store_sas_phy_maximum_linkrate in
   drivers/scsi/scsi_transport_sas.c) calls sas_set_phy_speed() ->
   sas_smp_phy_control() -> smp_execute_task() -> smp_execute_task_sg():

drivers/scsi/libsas/sas_init.c:sas_set_phy_speed() {
	...
	} else {
		struct sas_rphy *rphy = dev_to_rphy(phy->dev.parent);
		struct domain_device *ddev = sas_find_dev_by_rphy(rphy);
		ret = sas_smp_phy_control(ddev, phy->number,
					  PHY_FUNC_LINK_RESET, rates);
	}
}

3. Calling the exported sas_phy_reset() on an expander PHY calls
   sas_smp_phy_control() -> smp_execute_task() -> smp_execute_task_sg()
   without resuming ha->dev (unlike the sysfs workqueue wrappers
   queue_phy_reset() and queue_phy_enable() in drivers/scsi/libsas/sas_init.c,
   which call pm_runtime_get_sync(ha->dev)).

When the controller is runtime-suspended (for example on hisi_sas_v3_hw where
sas_suspend_devices() clears dev->lldd_dev and powers down the hardware),
won't these expander PHY operations fail with -ECOMM or access a suspended
HBA?

> @@ -2222,6 +2226,11 @@ void sas_smp_handler(struct bsg_job *job, struct Scsi_Host *shost,
>  		goto out;
>  	}
>  
> +	/* The host may have autosuspended, resume it here. */
> +	ret = pm_runtime_resume_and_get(dev->port->ha->dev);
> +	if (ret)
> +		goto out;
> +
>  	ret = smp_execute_task_sg(dev, job->request_payload.sg_list,
>  			job->reply_payload.sg_list);
>  	if (ret >= 0) {

[Severity: High]
Won't pm_runtime_resume_and_get(dev->port->ha->dev) fail with -EACCES on
platform-bus libsas controllers that do not enable runtime PM, such as
hisi_sas_v1_hw and hisi_sas_v2_hw?

Previously, smp_execute_task_sg() called pm_runtime_get_sync(ha->dev) and did
not check its return value. In drivers/base/power/runtime.c:rpm_resume(), when
dev->power.disable_depth > 0 and !(rpmflags & RPM_TRANSPARENT), it returns 1
only if runtime_status and last_status are both RPM_ACTIVE, and returns
-EACCES otherwise:

drivers/base/power/runtime.c:rpm_resume() {
	if (dev->power.runtime_error) {
		retval = -EINVAL;
	} else if (dev->power.disable_depth > 0) {
		if (dev->power.runtime_status == RPM_ACTIVE &&
		    dev->power.last_status == RPM_ACTIVE)
			retval = 1;
		else if (rpmflags & RPM_TRANSPARENT)
			goto out;
		else
			retval = -EACCES;
	}
}

Unlike PCI devices (where pci_pm_init() calls pm_runtime_set_active()),
platform devices are initialized by pm_runtime_init() with disable_depth = 1,
runtime_status = RPM_SUSPENDED, and last_status = RPM_INVALID. In
drivers/scsi/hisi_sas/hisi_sas_main.c:hisi_sas_probe(), platform controllers
set sha->dev = hisi_hba->dev (&pdev->dev) without calling
pm_runtime_set_active() or pm_runtime_enable(), so every expander BSG request
in sas_smp_handler() will abort at if (ret) goto out with -EACCES when
CONFIG_PM=y.

> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
> index 704ec94383c3c..e1aa0af52c658 100644
> --- a/drivers/target/iscsi/iscsi_target_configfs.c
> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
> @@ -1306,17 +1306,24 @@ static ssize_t iscsi_disc_enforce_discovery_auth_store(struct config_item *item,
>  		return -EINVAL;
>  	}
>  
> +	if (iscsit_get_tpg(discovery_tpg) < 0)
> +		return -EINVAL;
> +
>  	param = iscsi_find_param_from_key(AUTHMETHOD,
>  				discovery_tpg->param_list);
> -	if (!param)
> -		return -EINVAL;
> +	if (!param) {
> +		err = -EINVAL;
> +		goto out;
> +	}
>  
>  	if (op) {
>  		/*
>  		 * Reset the AuthMethod key to CHAP.
>  		 */
> -		if (iscsi_update_param_value(param, CHAP) < 0)
> -			return -EINVAL;
> +		if (iscsi_update_param_value(param, CHAP) < 0) {
> +			err = -EINVAL;
> +			goto out;
> +		}
>  
>  		discovery_tpg->tpg_attrib.authentication = 1;
>  		iscsit_global->discovery_acl.node_auth.enforce_discovery_auth = 1;

[Severity: High]
Does acquiring discovery_tpg->tpg_access_lock via iscsit_get_tpg() prevent the
use-after-free race against concurrent discovery logins?

In drivers/target/iscsi/iscsi_target_parameters.c, iscsi_update_param_value()
frees param->value and reallocates it in place:

drivers/target/iscsi/iscsi_target_parameters.c:iscsi_update_param_value() {
	kfree(param->value);

	param->value = kstrdup(value, GFP_KERNEL);
	...
}

However, the discovery login path does not acquire
discovery_tpg->tpg_access_lock. When an initiator logs into a discovery
session, iscsi_target_locate_portal() in
drivers/target/iscsi/iscsi_target_nego.c sets conn->tpg =
iscsit_global->discovery_tpg and calls iscsit_access_np(np, conn->tpg), which
only takes discovery_tpg->np_login_sem:

drivers/target/iscsi/iscsi_target_nego.c:iscsi_target_locate_portal() {
	...
		conn->tpg = iscsit_global->discovery_tpg;
		if (iscsit_access_np(np, conn->tpg) < 0) {
	...
}

Then iscsi_login_zero_tsih_s2() in drivers/target/iscsi/iscsi_target_login.c
calls iscsi_copy_param_list(), which reads param->value while holding only
discovery_tpg->np_login_sem:

drivers/target/iscsi/iscsi_target_login.c:iscsi_login_zero_tsih_s2() {
	...
	if (iscsi_copy_param_list(&conn->param_list,
				conn->tpg->param_list, 1) < 0) {
	...
}

drivers/target/iscsi/iscsi_target_parameters.c:iscsi_copy_param_list() {
	...
		new_param->name = kstrdup(param->name, GFP_KERNEL);
		new_param->value = kstrdup(param->value, GFP_KERNEL);
	...
}

Since iscsi_disc_enforce_discovery_auth_store() holds only tpg_access_lock and
discovery login holds only np_login_sem, can kfree(param->value) in
iscsi_update_param_value() still race with kstrdup(param->value, GFP_KERNEL)
in iscsi_copy_param_list()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/6a4bfcdc8e60669d95abb70bdaa07b87154c9a95.camel@HansenPartnership.com?part=1