Re: [PATCH] scsi: cxgbit: drop the cnp reference when the close reply never comes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] scsi: cxgbit: double cxgbit_put_cnp() and UAF in __cxgbit_free_cdev_np()
--

commit 6bb8ef7c81c63dadb738bb34808599917602fb9c
Author: Wentao Liang <vulab@iscas.ac.cn>

scsi: cxgbit: drop the cnp reference when the close reply never comes

This commit attempts to fix a reference leak on struct cxgbit_np in
__cxgbit_free_cdev_np() when cxgbit_wait_for_reply() returns an error before
a firmware reply arrives, using completion_done() to check whether
cxgbit_close_listsrv_rpl() already handled the reply and dropped the
reference.

> diff --git a/drivers/target/iscsi/cxgbit/cxgbit_cm.c b/drivers/target/iscsi/cxgbit/cxgbit_cm.c
> index 146705845fa3e..91787bca3ff10 100644
> --- a/drivers/target/iscsi/cxgbit/cxgbit_cm.c
> +++ b/drivers/target/iscsi/cxgbit/cxgbit_cm.c
> @@ -558,6 +558,15 @@ __cxgbit_free_cdev_np(struct cxgbit_device *cdev, struct cxgbit_np *cnp)
>  
>  	cxgb4_free_stid(cdev->lldi.tids, stid,
>  			cnp->com.local_addr.ss_family);
> +
> +	/*
> +	 * A reply carrying an error status also returns -EIO, but that reply
> +	 * has already dropped the reference taken above.  Only a wait that
> +	 * bailed out before any reply arrived still holds it.
> +	 */
> +	if (ret && !completion_done(&cnp->com.wr_wait.completion))
> +		cxgbit_put_cnp(cnp);

[Severity: Critical]
Does completion_done() work as expected here after
wait_for_completion_timeout() has returned?

When CPL_CLOSE_LISTSRV_RPL arrives with a non-zero status,
cxgbit_close_listsrv_rpl() wakes the waiter and unconditionally drops the
reference taken by __cxgbit_free_cdev_np():

drivers/target/iscsi/cxgbit/cxgbit_cm.c:cxgbit_close_listsrv_rpl() {
	...
	cxgbit_wake_up(&cnp->com.wr_wait, __func__, rpl->status);
	cxgbit_put_cnp(cnp);
}

Inside cxgbit_wake_up(), wr_waitp->ret is set to -EIO and complete()
(rather than complete_all()) increments completion.done from 0 to 1:

drivers/target/iscsi/cxgbit/cxgbit_cm.c:cxgbit_wake_up() {
	if (ret == CPL_ERR_NONE)
		wr_waitp->ret = 0;
	else
		wr_waitp->ret = -EIO;
	...
	complete(&wr_waitp->completion);
}

When wait_for_completion_timeout() in cxgbit_wait_for_reply() wakes up,
do_wait_for_common() consumes that completion by decrementing completion.done
back to 0 before returning -EIO. Because completion.done is 0 again,
completion_done(&cnp->com.wr_wait.completion) always returns false on this
path.

Wouldn't __cxgbit_free_cdev_np() then call cxgbit_put_cnp(cnp) a second time,
dropping the caller's reference and freeing cnp before cxgbit_free_np()
accesses cnp->np_accept_lock, drains cnp->np_accept_list, and calls
cxgbit_put_cnp(cnp) again?

Also, if CDEV_STATE_UP is cleared while the CPL_CLOSE_LISTSRV_REQ is already
in flight, cxgbit_wait_for_reply() returns -EIO immediately without waiting.
If cxgbit_close_listsrv_rpl() on another CPU looks up cnp via lookup_stid()
just before cxgb4_free_stid() clears the stid entry, can
__cxgbit_free_cdev_np() observe !completion_done() before
cxgbit_close_listsrv_rpl() reaches complete(), causing both paths to call
cxgbit_put_cnp(cnp) and triggering a use-after-free in
cxgbit_close_listsrv_rpl() and cxgbit_free_np()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104021448.1468404-1-vulab@iscas.ac.cn?part=1