Re: [PATCH v2] scsi: cxgbi: fix retry_timer use-after-free on adapter detach

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Use-After-Free and Double Free for CTP_ACTIVE_OPEN connections during adapter detach
- [Critical] Race Condition leading to NULL Pointer Dereference in do_act_open_rpl

Pre-existing issues:
- [High] Memory leak and un-cancelled timer on user-initiated disconnect
--

--- Patch [1]: [PATCH v2] scsi: cxgbi: fix retry_timer use-after-free on adapter detach ---
commit 5cacab0f814551fa10e52ff589125571018e3cee
Author: Fan Wu <fanwu01@zju.edu.cn>

scsi: cxgbi: fix retry_timer use-after-free on adapter detach

This commit attempts to fix a use-after-free on the retry_timer by setting
a new flag CTPF_OFFLOAD_DOWN during portmap cleanup. It rejects new timer
arming and forces connections in CTP_ACTIVE_OPEN to be pushed through
cxgbi_sock_fail_act_open() to release resources.

> diff --git a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
> index 0e2bee937fe81..207c8ffb5f188 100644
> --- a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
> +++ b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
[ ... ]
> @@ -584,7 +589,8 @@ static int do_act_open_rpl(struct t3cdev *tdev, struct sk_buff *skb, void *ctx)
>  
>  	cxgbi_sock_get(csk);
>  	spin_lock_bh(&csk->lock);
> -	if (rpl->status == CPL_ERR_CONN_EXIST &&
> +	if (!cxgbi_sock_flag(csk, CTPF_OFFLOAD_DOWN) &&
> +	    rpl->status == CPL_ERR_CONN_EXIST &&
>  	    csk->retry_timer.function != act_open_retry_timer) {
>  		csk->retry_timer.function = act_open_retry_timer;
>  		mod_timer(&csk->retry_timer, jiffies + HZ / 2);

[Severity: Critical]
If CTPF_OFFLOAD_DOWN is set by a concurrent adapter detach, will this logic
fall through to the else branch and call cxgbi_sock_fail_act_open()?

When cxgbi_device_portmap_cleanup() already called cxgbi_sock_fail_act_open(),
csk->cdev gets cleared:

cxgbi_sock_closed()
   csk->cdev->csk_release_offload_resources(csk); 

If do_act_open_rpl() then calls cxgbi_sock_fail_act_open() again, it will set
the state to CTP_CONNECTING and call cxgbi_sock_closed().

Could this result in a NULL pointer dereference when cxgbi_sock_closed()
tries to access csk->cdev again?


> diff --git a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
> index 804806e1cbb4b..2e2a10117afcd 100644
> --- a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
> +++ b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
[ ... ]
> @@ -892,7 +898,8 @@ static void do_act_open_rpl(struct cxgbi_device *cdev, struct sk_buff *skb)
>  	cxgbi_sock_get(csk);
>  	spin_lock_bh(&csk->lock);
>  
> -	if (status == CPL_ERR_CONN_EXIST &&
> +	if (!cxgbi_sock_flag(csk, CTPF_OFFLOAD_DOWN) &&
> +	    status == CPL_ERR_CONN_EXIST &&
>  	    csk->retry_timer.function != csk_act_open_retry_timer) {
>  		csk->retry_timer.function = csk_act_open_retry_timer;
>  		mod_timer(&csk->retry_timer, jiffies + HZ / 2);

[Severity: Critical]
Is this susceptible to the same concurrent execution issue as cxgb3i above?

If do_act_open_rpl() executes concurrently with cxgbi_device_portmap_cleanup()
and falls into the else branch, it seems we might trigger a redundant teardown
and hit a NULL pointer dereference on csk->cdev.


> diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
> index f3bb7af4e984e..7b44d47c03946 100644
> --- a/drivers/scsi/cxgbi/libcxgbi.c
> +++ b/drivers/scsi/cxgbi/libcxgbi.c
[ ... ]
> @@ -99,8 +99,12 @@ void cxgbi_device_portmap_cleanup(struct cxgbi_device *cdev)
>  				csk, cdev);
>  			spin_lock_bh(&csk->lock);
>  			cxgbi_sock_set_flag(csk, CTPF_OFFLOAD_DOWN);
> -			cxgbi_sock_closed(csk);
> +			if (csk->state == CTP_ACTIVE_OPEN)
> +				cxgbi_sock_fail_act_open(csk, -ENODEV);
> +			else
> +				cxgbi_sock_closed(csk);

[Severity: High]
This isn't a bug introduced by this patch, but does the primary user-initiated
disconnect path also need to be fixed for active-open connections?

When the iSCSI daemon initiates a disconnect while the connection is still in
CTP_ACTIVE_OPEN, cxgbi_ep_disconnect() calls cxgbi_sock_closed():

cxgbi_ep_disconnect()
    if (likely(csk->state >= CTP_ESTABLISHED))
        need_active_close(csk);
    else
        cxgbi_sock_closed(csk);

Because the state is CTP_ACTIVE_OPEN, cxgbi_sock_closed() returns early
without doing anything:

cxgbi_sock_closed()
    if (csk->state == CTP_ACTIVE_OPEN || csk->state == CTP_CLOSED)
        return;

Does this leave the retry_timer armed and offload resources unreleased,
causing a memory leak and a background timer loop?

[Severity: Critical]
Could calling cxgbi_sock_fail_act_open() here drop too many references and
lead to a use-after-free?

cxgbi_sock_fail_act_open() sets the state to CTP_CONNECTING and calls
cxgbi_sock_closed(). Because the state is no longer CTP_ACTIVE_OPEN, it
bypasses the early return in cxgbi_sock_closed().

This causes cxgbi_sock_closed() to drop references for the port, the offload
resources, and call cxgbi_sock_put(csk).

If the iSCSI daemon already disconnected, the socket's refcount could reach
0 inside cxgbi_sock_closed(), freeing the memory.


>  			spin_unlock_bh(&csk->lock);
> +			timer_delete_sync(&csk->retry_timer);
>  			cxgbi_sock_put(csk);
>  		}
>  	}

[Severity: Critical]
Are we potentially accessing freed memory here?

If csk was already freed during the cxgbi_sock_fail_act_open() call above,
would spin_unlock_bh(), timer_delete_sync(), and cxgbi_sock_put() operate
on freed memory?

Also, doesn't cxgbi_sock_fail_act_open() steal the endpoint's reference by
bypassing the early return in cxgbi_sock_closed(), resulting in a double-free
when this cxgbi_sock_put(csk) is called?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814020948.197655-1-fanwu01@zju.edu.cn?part=1