[PATCH] scsi: target: fix PRIN READ_FULL_STATUS buffer bounds check

[PATCH] scsi: target: fix PRIN READ_FULL_STATUS buffer bounds check

[Tianchu Chen]

From: Tianchu Chen <flynnnchen@tencent.com>

core_scsi3_pri_read_full_status() writes one Full Status descriptor
per registered I_T nexus into the PRIN response buffer: a fixed
24-byte descriptor header followed by the variable-length
TransportID.  The overflow check, however, compares

	exp_desc_len + add_len > cmd->data_length

where exp_desc_len covers only the TransportID and add_len
accumulates only after completed descriptors, so it undershoots the actual
write extent by a constant 32 bytes (the 8-byte PRIN header, since
off = add_len + 8, plus the 24-byte descriptor header), leading a
remotely-triggerable heap OOB-write.

The allocation length of the PERSISTENT RESERVE IN CDB is fully
controlled by the (possibly remote) initiator and truncates
cmd->data_length, so an initiator that registered itself can pick a
data_length in [desc_len, desc_len + 31] and make the first
descriptor write up to 32 bytes past the end of the command's data
buffer.

Base the check on the actual write offset instead, so the test
covers exactly what the loop is about to write (24-byte descriptor
header plus TransportID, matching add_desc_len = 24 + desc_len
below), as core_scsi3_pri_read_keys() already does. Same principle
as commit c58a252beb04c ("target: fix buffer offset in
core_scsi3_pri_read_full_status").

Discovered by Atuin - Automated Vulnerability Discovery Engine.

Fixes: c66ac9db8d4ad ("[SCSI] target: Add LIO target core v4.0.0-rc6")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Tianchu Chen <flynnnchen@tencent.com>
---
 drivers/target/target_core_pr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 25b1bcacc0c8f..1d34887e9e845 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -3990,7 +3990,7 @@ core_scsi3_pri_read_full_status(struct se_cmd *cmd)
 		exp_desc_len = target_get_pr_transport_id_len(se_nacl, pr_reg,
 					&format_code);
 		if (exp_desc_len < 0 ||
-		    exp_desc_len + add_len > cmd->data_length) {
+		    off + 24 + exp_desc_len > cmd->data_length) {
 			pr_warn("SPC-3 PRIN READ_FULL_STATUS ran"
 				" out of buffer: %d\n", cmd->data_length);
 			spin_lock(&pr_tmpl->registration_lock);
-- 
2.51.0

Re: [PATCH] scsi: target: fix PRIN READ_FULL_STATUS buffer bounds check

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Preexisting out-of-bounds heap write in `iscsi_get_pr_transport_id` caused by double-incrementing buffer offset and length, bypassing the bounds check.
- [High] Use-After-Free in `core_scsi3_pri_read_full_status` due to dropping the lock while iterating with `list_for_each_entry_safe`.
--

commit 5d1c2d868ed5fae3f80b9cbcf44f8a5d0a03c061
Author: Tianchu Chen <flynnnchen@tencent.com>
scsi: target: fix PRIN READ_FULL_STATUS buffer bounds check

This commit fixes a heap out-of-bounds write in the PR IN READ FULL STATUS
command processing. It corrects the buffer overflow check to account for the
8-byte PRIN header and the 24-byte descriptor header, which were previously
omitted.

> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> index e7933115087ab..53ffb89f196b9 100644
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -3988,7 +3988,7 @@ core_scsi3_pri_read_full_status(struct se_cmd *cmd)
>  		exp_desc_len = target_get_pr_transport_id_len(se_nacl, pr_reg,
>  					&format_code);

[Severity: High]
This is a pre-existing issue, but I have a question about the list iteration
happening just above this bounds check:

drivers/target/target_core_pr.c:core_scsi3_pri_read_full_status() {
    ...
    list_for_each_entry_safe(pr_reg, pr_reg_tmp,
			&pr_tmpl->registration_list, pr_reg_list) {
        ...
        atomic_inc_mb(&pr_reg->pr_res_holders);
        spin_unlock(&pr_tmpl->registration_lock);
    ...
}

While the lock is dropped to process the descriptor, isn't the cached next
pointer (pr_reg_tmp) completely unprotected?

If another initiator concurrently issues a PR OUT command that unregisters
and frees the next registration in the list, could this lead to a
use-after-free when the lock is reacquired and the loop evaluates the next
condition?

>  		if (exp_desc_len < 0 ||
> -		    exp_desc_len + add_len > cmd->data_length) {
> +		    off + 24 + exp_desc_len > cmd->data_length) {

[Severity: Critical]
This is also a pre-existing issue, but does this bounds check still leave us
vulnerable to an out-of-bounds write if the transport ID generation overruns
the expected length?

Looking at the iSCSI specific implementation:

drivers/target/target_core_fabric_lib.c:iscsi_get_pr_transport_id() {
    ...
    buf[off+len] = 0x2c; off++; /* ASCII Character: "," */
    buf[off+len] = 0x69; off++; /* ASCII Character: "i" */
    buf[off+len] = 0x2c; off++; /* ASCII Character: "," */
    buf[off+len] = 0x30; off++; /* ASCII Character: "0" */
    buf[off+len] = 0x78; off++; /* ASCII Character: "x" */
    len += 5;
    buf[off+len] = pr_reg->pr_reg_isid[0]; off++;
    ...
}

When writing the separator, both off is incremented per byte and len += 5 is
executed at the end. Since the array index used is off+len, won't this cause
the index to jump by 10 instead of 5?

If an initiator logs in over iSCSI providing a custom ISID, could this write
the 7-byte ISID up to 5 bytes past the correctly calculated descriptor length?

Since this bounds check trusts the expected length returned by
target_get_pr_transport_id_len(), wouldn't this bypass the check and
corrupt memory past the allocated cmd->data_length buffer?

>  			pr_warn("SPC-3 PRIN READ_FULL_STATUS ran"
>  				" out of buffer: %d\n", cmd->data_length);
>  			spin_lock(&pr_tmpl->registration_lock);

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

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

[Tianchu Chen]

From: Tianchu Chen <flynnnchen@tencent.com>

In core_scsi3_emulate_pro_preempt(), the !all_reg first path removes
every registration whose key matches the SA RESERVATION KEY field -
including the caller's own.  Unlike the two sibling loops, this one
does not skip the calling I_T nexus; instead it passes
dec_holders = calling_it_nexus, so __core_scsi3_free_registration()
drops the reference taken by core_scsi3_locate_pr_reg() and, for
plain PREEMPT, kmem_cache_free()s the registration right away.

The function tail then unconditionally calls
core_scsi3_put_pr_reg(pr_reg_n), decrementing pr_res_holders of the
already freed object.

A remote initiator can trigger this with two commands: REGISTER with
key K, then PREEMPT with SA RESERVATION KEY K while no reservation is
held (or it is held with a different key).  The dangling decrement
silently corrupts the freed slot, or underflows pr_res_holders of
whatever registration reuses the t10_pr_reg_cache slot, which can
later turn into an early free / double free of a live registration.

Track that the free path consumed the locate reference and skip the
final put in that case.

Discovered by Atuin - Automated Vulnerability Discovery Engine.

Fixes: c66ac9db8d4ad ("[SCSI] target: Add LIO target core v4.0.0-rc6")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Tianchu Chen <flynnnchen@tencent.com>
---
 drivers/target/target_core_pr.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 1d34887e9e845..80e112efd6ead 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -2823,6 +2823,7 @@ core_scsi3_emulate_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_
 	u64 pr_res_mapped_lun = 0;
 	int all_reg = 0, calling_it_nexus = 0;
 	bool sa_res_key_unmatched = sa_res_key != 0;
+	bool reg_n_released = false;
 	int prh_type = 0, prh_scope = 0;
 
 	if (!se_sess)
@@ -2926,6 +2927,13 @@ core_scsi3_emulate_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_
 				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,
@@ -3012,7 +3020,8 @@ core_scsi3_emulate_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_
 		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;
 	}
-- 
2.51.0

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

[sashiko-bot]

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