[PATCH] scsi: target: cxgbit: fix refcount leak in __cxgbit_free_cdev_np

[PATCH] scsi: target: cxgbit: fix refcount leak in __cxgbit_free_cdev_np

[WenTao Liang]

__cxgbit_free_cdev_np() calls cxgbit_get_cnp() to take a reference
on cnp for the duration of the firmware remove-server operation.
If cxgb4_remove_server() fails, the reference is correctly released
in the error path with cxgbit_put_cnp(). However, once past that
check, the reference is never released on any other path:
 - when cxgbit_wait_for_reply() times out,
 - when cxgbit_wait_for_reply() succeeds,
 - or when cxgbit_wait_for_reply() returns another error.

All three paths return without dropping the reference, and no
called function consumes or releases the kref. The caller,
cxgbit_free_cdev_np(), does not balance the reference either.

Add the missing cxgbit_put_cnp() on the timeout and success paths,
and use a unified cleanup to handle all non-early-error returns.

Cc: stable@vger.kernel.org
Fixes: 9730ffcb8957 ("cxgbit: add files for cxgbit.ko")
Signed-off-by: WenTao Liang <vulab@iscas.ac.cn>
---
 drivers/target/iscsi/cxgbit/cxgbit_cm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/target/iscsi/cxgbit/cxgbit_cm.c b/drivers/target/iscsi/cxgbit/cxgbit_cm.c
index 146705845fa3..f8cd6457666b 100644
--- a/drivers/target/iscsi/cxgbit/cxgbit_cm.c
+++ b/drivers/target/iscsi/cxgbit/cxgbit_cm.c
@@ -544,8 +544,10 @@ __cxgbit_free_cdev_np(struct cxgbit_device *cdev, struct cxgbit_np *cnp)
 
 	ret = cxgbit_wait_for_reply(cdev, &cnp->com.wr_wait,
 				    0, 10, __func__);
-	if (ret == -ETIMEDOUT)
+	if (ret == -ETIMEDOUT) {
+		cxgbit_put_cnp(cnp);
 		return ret;
+	}
 
 	if (ipv6 && cnp->com.cdev) {
 		struct sockaddr_in6 *sin6;
@@ -558,6 +560,7 @@ __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);
+	cxgbit_put_cnp(cnp);
 	return 0;
 }
 
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] scsi: target: cxgbit: fix refcount leak in __cxgbit_free_cdev_np

[sashiko-bot]

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

New issues:
- [Critical] The patch introduces critical use-after-free and double-free vulnerabilities by unconditionally dropping the `cnp` reference count on both the success and timeout paths, violating the driver's asynchronous reply handling model.

Pre-existing issues:
- [Critical] When `__cxgbit_free_cdev_np()` times out, it aborts cleanup and causes the caller `cxgbit_free_all_np()` to break its loop, leaving dangling STID references to `cnp` on remaining devices.
--

commit f433423e199b1a3438bcf4e0bc324b24ab3b6629
Author: WenTao Liang <vulab@iscas.ac.cn>

scsi: target: cxgbit: fix refcount leak in __cxgbit_free_cdev_np

__cxgbit_free_cdev_np() calls cxgbit_get_cnp() to take a reference
on cnp for the duration of the firmware remove-server operation.
This patch attempts to release the reference on the timeout and
success paths.

Cc: stable@vger.kernel.org
Fixes: 9730ffcb8957 ("cxgbit: add files for cxgbit.ko")

> diff --git a/drivers/target/iscsi/cxgbit/cxgbit_cm.c b/drivers/target/iscsi/cxgbit/cxgbit_cm.c
> index 146705845fa3e..f8cd6457666b0 100644
> --- a/drivers/target/iscsi/cxgbit/cxgbit_cm.c
> +++ b/drivers/target/iscsi/cxgbit/cxgbit_cm.c
> @@ -544,8 +544,10 @@ __cxgbit_free_cdev_np(struct cxgbit_device *cdev, struct cxgbit_np *cnp)
>  
>  	ret = cxgbit_wait_for_reply(cdev, &cnp->com.wr_wait,
>  				    0, 10, __func__);
> -	if (ret == -ETIMEDOUT)
> +	if (ret == -ETIMEDOUT) {
> +		cxgbit_put_cnp(cnp);
>  		return ret;
> +	}

[Severity: Critical]
Does this cause a use-after-free and double-free if the firmware
reply is delayed?

In the Chelsio hardware transaction model, the reference acquired by
cxgbit_get_cnp() before sending the remove-server command is strictly
intended to be consumed by the asynchronous firmware reply handler,
cxgbit_close_listsrv_rpl().

If we time out and drop the reference here, the firmware request is
still pending and the STID is not unlinked. When the delayed reply
eventually arrives, cxgbit_close_listsrv_rpl() will look up the STID,
retrieve the already-freed cnp object, dereference it to wake up the
wait queue, and attempt to drop the reference again:

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

[Severity: Critical]
This is a pre-existing issue, but does this timeout path leave
dangling STID references that lead to a use-after-free on incoming
connections?

If __cxgbit_free_cdev_np() returns -ETIMEDOUT, the caller
cxgbit_free_all_np() will break its loop and abandon the removal
of STIDs on subsequent healthy devices:

drivers/target/iscsi/cxgbit/cxgbit_cm.c:cxgbit_free_all_np() {
    ...
    ret = __cxgbit_free_cdev_np(cdev, cnp);
    if (ret == -ETIMEDOUT)
        break;
    ...
}

The parent caller cxgbit_free_np() then drops its base reference, freeing
the cnp object. Because the remaining devices still have active STID
registrations pointing to this freed cnp, any incoming connection request
handled by cxgbit_pass_accept_req() will look up the dangling STID and
access the freed cnp.

>  
>  	if (ipv6 && cnp->com.cdev) {
>  		struct sockaddr_in6 *sin6;
[ ... ]
> @@ -558,6 +560,7 @@ __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);
> +	cxgbit_put_cnp(cnp);

[Severity: Critical]
Does this addition cause an immediate double-free on the success path?

On the success path where cxgbit_wait_for_reply() returns 0, the
asynchronous reply handler cxgbit_close_listsrv_rpl() has already
executed and dropped the reference originally acquired by
cxgbit_get_cnp(). Adding an unconditional cxgbit_put_cnp(cnp) here
will attempt to drop a reference that has already been consumed.

>  	return 0;
>  }
>

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