Re: [PATCH] scsi: iscsi: fix NULL pointer dereference in iscsi_sw_tcp_release_conn()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-after-free of `iscsi_cls_conn` due to missing cancellation of `cleanup_work` on release.
--

--- Patch [1]: [PATCH] scsi: iscsi: fix NULL pointer dereference in iscsi_sw_tcp_release_conn() ---
commit bdfb05c9a6c008e559f81d0c3e5ffb712699e0cc
Author: Ye Bin <yebin10@huawei.com>

scsi: iscsi: fix NULL pointer dereference in iscsi_sw_tcp_release_conn()

This commit fixes a NULL pointer dereference by setting the
ISCSI_CLS_CONN_BIT_CLEANUP bit when userspace delivers STOP_CONN_TERM.
This prevents concurrent execution of iscsi_stop_conn() and cancels or
flushes the kernel connection cleanup work as needed.

Fixes: 23d6fefbb3f6 ("scsi: iscsi: Fix in-kernel conn failure handling")

> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2266,6 +2266,8 @@ static void iscsi_if_disconnect_bound_ep(struct iscsi_cls_conn *conn,
>  
>  static int iscsi_if_stop_conn(struct iscsi_cls_conn *conn, int flag)
>  {
> +	bool cleanup;
> +
>  	ISCSI_DBG_TRANS_CONN(conn, "iscsi if conn stop.\n");
>  	/*
>  	 * For offload, iscsid may not know about the ep like when iscsid is
[ ... ]
> -	if (flag == STOP_CONN_TERM) {
> -		cancel_work_sync(&conn->cleanup_work);
> -		iscsi_stop_conn(conn, flag);
> -	} else {
> +	spin_lock_irq(&conn->lock);
> +	cleanup = test_and_set_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags);
> +	spin_unlock_irq(&conn->lock);
> +
> +	if (cleanup) {
>  		/*
> -		 * Figure out if it was the kernel or userspace initiating this.
> +		 * If this is a termination we have to call stop_conn with
> +		 * that flag so the correct states get set. If we haven't
> +		 * run the work yet try to avoid the extra run.
>  		 */
> -		spin_lock_irq(&conn->lock);
> -		if (!test_and_set_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
> -			spin_unlock_irq(&conn->lock);
> +		if (flag == STOP_CONN_TERM) {
> +			ISCSI_DBG_TRANS_CONN(conn,
> +					"cancel kernel conn cleanup.\n");
> +			cancel_work_sync(&conn->cleanup_work);

[Severity: High]
This is a pre-existing issue, but since we are modifying cleanup_work
handling here, is there a risk of a use-after-free if the connection is
destroyed before the work executes?

Looking at iscsi_conn_error_event(), it queues conn->cleanup_work without
taking a reference to the connection. If a session is torn down before the
work executes (e.g. via device removal calling iscsi_remove_session()),
iscsi_iter_destroy_conn_fn() drops the references:

drivers/scsi/scsi_transport_iscsi.c:iscsi_iter_destroy_conn_fn()
    ...
    iscsi_remove_conn(iscsi_dev_to_conn(dev));
    iscsi_put_conn(iscsi_dev_to_conn(dev));

This final reference drop triggers iscsi_conn_release(), which frees the
connection memory:

drivers/scsi/scsi_transport_iscsi.c:iscsi_conn_release()
    ...
    kfree(conn);

Since iscsi_conn_release() does not call cancel_work_sync(&conn->cleanup_work),
could iscsi_cleanup_conn_work_fn() execute and dereference the already freed
conn struct? Should we be cancelling or flushing the work during
iscsi_conn_release() to prevent this?

>  			iscsi_stop_conn(conn, flag);
>  		} else {
> -			spin_unlock_irq(&conn->lock);
>  			ISCSI_DBG_TRANS_CONN(conn,
> -					     "flush kernel conn cleanup.\n");
> +					"flush kernel conn cleanup.\n");
>  			flush_work(&conn->cleanup_work);
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820020321.1537641-1-yebin@huaweicloud.com?part=1