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

On 2026/8/20 10:26, [email protected] wrote:
> 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 <[email protected]>
>
> 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:
>
Indeed, this issue does exist, although it is unrelated to this revision.
I will fix this issue and send another version.

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