Re: [PATCH] usb: gadget: f_tcm: keep port count until LUN teardown completes

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

Pre-existing issues:
- [High] The patch moves the `tpg_port_count` decrement to `fabric_post_unlink` for `f_tcm` to fix a nexus removal UAF race, but fails to apply this fix to sister drivers `vhost_scsi` and `xen-scsiback`.
- [High] Lockless access to `tpg->tpg_nexus` in the I/O path leads to Use-After-Free and NULL pointer dereferences when racing with `tcm_usbg_drop_nexus`.
- [High] The patch's fix for 'free active' and `sess_cmd_map` UAF is incomplete because it ignores pending work items when `tpg_port_count == 0`.
- [High] TOCTOU race between `target_fabric_port_link` and `tcm_usbg_drop_nexus` allows the nexus to be destroyed while a LUN is actively linked.
--

--- Patch [1]: [PATCH] usb: gadget: f_tcm: keep port count until LUN teardown completes ---
commit 052b5e8542d90321dbacef963d1804574c8248ec
Author: Shuangpeng Bai <[email protected]>

usb: gadget: f_tcm: keep port count until LUN teardown completes

This patch introduces an optional fabric_post_unlink callback to the
target core. It uses this callback in f_tcm to defer decrementing the
port count until after core_dev_del_lun has drained active LUN
references, fixing a use-after-free race during nexus removal.

Cc: [email protected]

> diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
> index 166dbf4c40616..ab8f81650710e 100644
> --- a/drivers/target/target_core_fabric_configfs.c
> +++ b/drivers/target/target_core_fabric_configfs.c
> @@ -690,6 +690,14 @@ static void target_fabric_port_unlink(
>  	}
>  
>  	core_dev_del_lun(se_tpg, lun);
> +
> +	if (tf->tf_ops->fabric_post_unlink) {
> +		/*
> +		 * Allow fabrics to release state that must remain valid until
> +		 * core_dev_del_lun() has drained all active LUN references.
> +		 */
> +		tf->tf_ops->fabric_post_unlink(se_tpg, lun);
> +	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the separation of LUN
attachment and port count increment create a time-of-check to time-of-use
race?

In target_fabric_port_link():

drivers/target/target_core_fabric_configfs.c:target_fabric_port_link() {
    ...
    ret = core_dev_add_lun(se_tpg, dev, lun);
    ...
    if (tf->tf_ops->fabric_post_link)
        tf->tf_ops->fabric_post_link(se_tpg, lun);
}

Since tpg_port_count is incremented inside fabric_post_link() for f_tcm, is
there a window after core_dev_add_lun() attaches the LUN where
tcm_usbg_drop_nexus() could observe tpg_port_count == 0?

If it executes concurrently during this window, would it bypass the safety
check and remove the session while a LUN has just been fully linked?

> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> index b3fa5a17fd2db..98414e7611c08 100644
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -2024,7 +2024,7 @@ static const struct target_core_fabric_ops usbg_ops = {
>  	.fabric_enable_tpg		= usbg_enable_tpg,
>  	.fabric_drop_tpg		= usbg_drop_tpg,
>  	.fabric_post_link		= usbg_port_link,
> -	.fabric_pre_unlink		= usbg_port_unlink,
> +	.fabric_post_unlink		= usbg_port_unlink,
>  	.fabric_init_nodeacl		= usbg_init_nodeacl,

[Severity: High]
This isn't a bug introduced by this patch, but does moving this decrement to
fabric_post_unlink for f_tcm leave vhost_scsi and xen-scsiback vulnerable to
the same race?

Looking at vhost_scsi_ops in drivers/vhost/scsi.c:

    .fabric_pre_unlink = vhost_scsi_port_unlink,

And scsiback_ops in drivers/xen/xen-scsiback.c:

    .fabric_pre_unlink = scsiback_port_unlink,

If a nexus removal is triggered concurrently with the removal of the last
LUN on either of these fabrics, won't the early port count decrement in
fabric_pre_unlink allow them to bypass the EBUSY check? It appears they could
proceed to call target_remove_session() before core_dev_del_lun() has finished
waiting for active commands to drain, potentially causing a similar
use-after-free.

[Severity: High]
This is a pre-existing issue, but can lockless reads of tpg->tpg_nexus
in the I/O path race with nexus removal?

In tcm_usbg_drop_nexus(), we free the nexus and clear the pointer:

drivers/usb/gadget/function/f_tcm.c:tcm_usbg_drop_nexus() {
    ...
    tpg->tpg_nexus = NULL;
    kfree(tv_nexus);
    ...
}

Meanwhile, the USB completion callbacks like usbg_submit_command() and
usbg_submit_tmr() read it without holding tpg_mutex or the RCU read lock:

drivers/usb/gadget/function/f_tcm.c:usbg_submit_command() {
    ...
    tv_nexus = tpg->tpg_nexus;
    ...
}

Could this result in a NULL pointer dereference or use-after-free if the
nexus is dropped via configfs while a command is actively being processed?

[Severity: High]
This is a pre-existing issue, but does this fix cover the case where
tpg_port_count is 0 but work is still pending?

If no explicit LUNs are configured, tpg_port_count is 0. If a command is
queued to usbg_cmd_work concurrently with the admin dropping the nexus:

drivers/usb/gadget/function/f_tcm.c:tcm_usbg_drop_nexus() {
    ...
    if (tpg->tpg_port_count != 0) {
        ...
    }
    ...
    target_remove_session(se_sess);
}

Could this flow proceed to remove the session and free sess_cmd_map without
waiting for usbg_cmd_work to complete?

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