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

[sashiko-bot@kernel.org]

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