Re: [PATCH -next 2/2] scsi: iscsi: Fix use-after-free in iscsi_conn_release() for cleanup_work

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

New issues:
- [High] Adding `cancel_work_sync(&conn->cleanup_work)` inside `iscsi_conn_release()` introduces a guaranteed AB-BA deadlock with `session->eh_mutex` when the final connection reference is dropped by the SCSI error handler.

Pre-existing issues:
- [Critical] ep->conn is never cleared when a connection is destroyed, leading to a Use-After-Free in iscsi_if_ep_disconnect().
- [High] LLD-specific background tasks (e.g., `transport_timer`, `xmitwork`) are not canceled before `iscsi_conn_release()` frees the connection memory, leading to a Use-After-Free.
--

--- Patch [2]: [PATCH -next 2/2] scsi: iscsi: Fix use-after-free in iscsi_conn_release() for cleanup_work ---
commit 16664476ef28f32b2b58204098a289184054f45c
Author: Ye Bin <[email protected]>

scsi: iscsi: Fix use-after-free in iscsi_conn_release() for cleanup_work

This patch attempts to resolve a use-after-free by canceling the pending or
running cleanup_work task before freeing the connection memory in
iscsi_conn_release().

> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index aea319c1e72fe..cdc337b0fc26a 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2128,6 +2128,13 @@ static void iscsi_conn_release(struct device *dev)
>  	struct device *parent = conn->dev.parent;
>  
>  	ISCSI_DBG_TRANS_CONN(conn, "Releasing conn\n");
> +	/*
> +	 * iscsi_conn_error_event() may have queued conn->cleanup_work without
> +	 * holding a reference to the connection. Cancel any pending or
> +	 * running work before freeing the connection to prevent a
> +	 * use-after-free.
> +	 */
> +	cancel_work_sync(&conn->cleanup_work);

[Severity: High]
Could this cancel_work_sync() introduce an AB-BA deadlock with
session->eh_mutex?

If a concurrent session teardown drops the main connection references, the
SCSI error handler might drop the final reference while still holding the
eh_mutex:

drivers/scsi/libiscsi.c:iscsi_eh_abort() {
    mutex_lock(&session->eh_mutex);
    ...
    iscsi_put_conn(conn->cls_conn);
    mutex_unlock(&session->eh_mutex);
}

When iscsi_put_conn() triggers iscsi_conn_release(), it will block here
waiting for the workqueue to finish. However, if the worker thread is
already running, it attempts to acquire the exact same mutex:

drivers/scsi/libiscsi.c:iscsi_conn_stop() {
    mutex_lock(&session->eh_mutex);
    ...
}

Would this cause the error handler and the workqueue to wait on each other
indefinitely?

> +	kfree(conn);

[Severity: High]
This is a pre-existing issue, but does freeing the connection here without
canceling LLD-specific background tasks leave a use-after-free risk?

When a session is abruptly torn down, the teardown iterates over active
connections and drops the final reference:

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

Because this bypasses the transport's destroy_conn callback (such as
iscsi_conn_teardown), asynchronous tasks like conn->transport_timer and
conn->xmitwork are never canceled.

Could this regression allow these timers or work tasks to execute later and
dereference the freed connection memory?

[Severity: Critical]
This is a pre-existing issue, but is there another use-after-free possible
when an endpoint disconnects after a connection is destroyed?

If a session is forcefully destroyed via iscsi_remove_session(), the
connection is freed in iscsi_conn_release() as seen above. However,
ep->conn does not appear to be cleared when this happens.

If userspace then issues an ISCSI_UEVENT_TRANSPORT_EP_DISCONNECT command,
the kernel reads the dangling ep->conn pointer:

drivers/scsi/scsi_transport_iscsi.c:iscsi_if_ep_disconnect() {
    conn = ep->conn;
    if (!conn) {
        ...
    }
    
    mutex_lock(&conn->ep_mutex);
}

Could this regression allow a userspace netlink command to trigger a
use-after-free by acquiring the mutex on the freed connection memory?

>  	put_device(parent);
>  }

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