Re: [PATCH] scsi: target: fix use-after-free on self-preempt in PROUT PREEMPT

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] When processing PREEMPT_AND_ABORT, preempted registrations are added to `preempt_and_abort_list` but are leaked if a specific condition is not met, skipping required task aborts.
- [High] Unsafe `list_for_each_entry_safe` iteration across a spinlock drop in PR clearing and preemption paths leads to a potential Use-After-Free.
- [High] Missing reference decrement in `core_scsi3_emulate_pro_clear` causes a soft lockup if the caller's registration is concurrently unlinked.
--

commit 3df8f536b0380acddfcc85e22e361b03863b2a7d
Author: Tianchu Chen <flynnnchen@tencent.com>

scsi: target: fix use-after-free on self-preempt in PROUT PREEMPT

This commit fixes a use-after-free bug in core_scsi3_pro_preempt() that
occurs when a remote initiator preempts its own registration. It tracks if
the free path consumed the locate reference and conditionally skips the
final put.

> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> index e7933115087ab..e4160ed3c1191 100644
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -2926,6 +2927,13 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_key,
>  				sa_res_key_unmatched = false;
>  
>  				calling_it_nexus = (pr_reg_n == pr_reg) ? 1 : 0;
> +				/*
> +				 * dec_holders=1 makes the free path drop the
> +				 * reference core_scsi3_locate_pr_reg() took on
> +				 * pr_reg_n, so the tail must not put it again.
> +				 */
> +				if (calling_it_nexus)
> +					reg_n_released = true;
>  				pr_reg_nacl = pr_reg->pr_reg_nacl;
>  				pr_res_mapped_lun = pr_reg->pr_res_mapped_lun;
>  				__core_scsi3_free_registration(dev, pr_reg,

[Severity: High]
This is a pre-existing issue, but since we are modifying the preemption
logic here, could this list iteration be unsafe?

The loop enclosing this section uses list_for_each_entry_safe, which caches
the next element assuming the list remains safely locked. However, inside
__core_scsi3_free_registration():

__core_scsi3_free_registration() {
        ...
        spin_unlock(&pr_tmpl->registration_lock);
        while (atomic_read(&pr_reg->pr_res_holders) != 0) {
                cpu_relax();
        }
        ...
}

Since the list lock is dropped inside the loop body, couldn't a concurrent
thread acquire the lock and unlink or free the cached next element, causing
a use-after-free when iteration resumes?

[ ... ]

> @@ -2997,7 +3005,8 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_key,
>  		if (pr_tmpl->pr_aptpl_active)
>  			core_scsi3_update_and_write_aptpl(cmd->se_dev, true);
>  
> -		core_scsi3_put_pr_reg(pr_reg_n);
> +		if (!reg_n_released)
> +			core_scsi3_put_pr_reg(pr_reg_n);
>  		core_scsi3_pr_generation(cmd->se_dev);
>  		return 0;

[Severity: High]
This is a pre-existing issue, but I noticed something unusual about how
preempt_and_abort_list is handled before we return here.

When processing PREEMPT_AND_ABORT, preempted registrations are added to the
local preempt_and_abort_list. However, earlier in this function,
core_scsi3_release_preempt_and_abort() is only called if a very specific
condition is met:

if (pr_res_holder && all_reg && !(sa_res_key)) {
        __core_scsi3_complete_pro_preempt(...)
        if (preempt_type == PREEMPT_AND_ABORT)
                core_scsi3_release_preempt_and_abort(...)
}

If that condition evaluates to false, we eventually reach this return 0
statement without cleaning up the list.

Does this permanently leak the local preempt_and_abort_list and skip the
required task aborts if a remote initiator sends a PREEMPT_AND_ABORT command
with all_reg=0 or a non-zero sa_res_key?

[Severity: High]
This is another pre-existing issue, but looking at a similar pattern in
core_scsi3_emulate_pro_clear():

core_scsi3_emulate_pro_clear() {
        pr_reg_n = core_scsi3_locate_pr_reg(...);

        list_for_each_entry_safe(...) {
                calling_it_nexus = (pr_reg_n == pr_reg) ? 1 : 0;
                __core_scsi3_free_registration(..., calling_it_nexus);
        }
        ...
        return 0;
}

The function relies exclusively on finding pr_reg_n during its list loop to
drop the reference via __core_scsi3_free_registration. If another thread
concurrently unlinks pr_reg_n before the list lock is acquired, the loop
never encounters it.

Since the function eventually returns 0 without a fallback
core_scsi3_put_pr_reg() call, could this leaked reference force the
unlinking thread to spin infinitely in its atomic_read cpu_relax loop,
leading to a soft lockup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/ddf3c216cf486a1939784731bd1fb50e58092256@linux.dev?part=1