Re: [PATCH] scsi: target: use kref_get_unless_zero() in core_get_se_deve_from_rtpi()

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

Pre-existing issues:
- [High] The patch addresses a kref revival race in `core_get_se_deve_from_rtpi()`, but identically flawed `kref_get()` patterns remain in `__core_scsi3_do_alloc_registration()` and `core_scsi3_check_aptpl_registration()`.
- [High] Fetching `tfc_sess` under RCU using a plain `kref_get()` in `ft_sess_get()` can revive a 0-refcount object, leading to a double free.
--

--- Patch [1]: [PATCH] scsi: target: use kref_get_unless_zero() in core_get_se_deve_from_rtpi() ---
commit 1ccb09531fbaa5b4a4d4c2e984295bc9d5b45e8e
Author: Yifei Gao <[email protected]>

scsi: target: use kref_get_unless_zero() in core_get_se_deve_from_rtpi()

This commit fixes a use-after-free vulnerability in core_get_se_deve_from_rtpi().
It replaces a plain kref_get() with kref_get_unless_zero() when acquiring a
reference to se_dev_entry under RCU. This prevents the revival of a 0-refcount
object during a concurrent teardown.

> diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> index 9db2201aa553f..bff97c6efffbe 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -220,7 +220,8 @@ struct se_dev_entry *core_get_se_deve_from_rtpi(
>  		if (lun->lun_tpg->tpg_rtpi != rtpi)
>  			continue;
>  
> -		kref_get(&deve->pr_kref);
> +		if (!kref_get_unless_zero(&deve->pr_kref))
> +			continue;

[Severity: High]
This isn't a bug introduced by this patch, but since this addresses a kref 
revival race, could the same flaw exist in other places doing lockless RCU
lookups of se_dev_entry?

For example, drivers/target/target_core_pr.c:__core_scsi3_do_alloc_registration()
does:

		kref_get(&pr_reg->pr_reg_deve->pr_kref);
		rcu_read_unlock();

And similarly in drivers/target/target_core_pr.c:core_scsi3_check_aptpl_registration():

			kref_get(&pr_reg->pr_reg_deve->pr_kref);
			rcu_read_unlock();

If a concurrent disable path unhashes the entry and drops its final reference,
could a delayed reader executing the plain kref_get() resurrect the dying
object and cause a use-after-free when the disable path proceeds to free it
via call_rcu()?

[Severity: High]
This is another pre-existing issue, but while looking at plain kref_get()
usage under RCU, could a similar double-free issue occur in
drivers/target/tcm_fc/tfc_sess.c:ft_sess_get()?

			kref_get(&sess->kref);
			rcu_read_unlock();

If a teardown path (like handling a PRLO) calls ft_sess_delete() to unhash
the session and drops the final reference concurrently, could a reader
executing this plain kref_get() resurrect the session?

When the reader later drops its reference, would this lead to 
target_remove_session() and kfree_rcu() executing a second time?

>  		rcu_read_unlock();
>  
>  		return deve;

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