[PATCH] target: Fix an overwriting of the error status code

[PATCH] target: Fix an overwriting of the error status code

[Markov Gleb]

From: Gleb Markov <markov.gi@npc-ksb.ru>

If the maximum field length is exceeded, the error status code written to
ret will be overwritten without verification, and data with an incorrect
field length will be passed to core_scsi3_alloc_aptpl_registration(),
where it will be truncated by snprintf() to the maximum allowed length,
without "\0", resulting in the use of truncated data.

If pr_reg->pr_reg_isid, pr_reg->pr_iport or pr_reg->pr_tport are passed to
functions such as strlen(), strcmp(), which expect a "\0" at the end, it
will result in a memory access outside the buffer.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: c66ac9db8d4a ("[SCSI] target: Add LIO target core v4.0.0-rc6")
Signed-off-by: Gleb Markov <markov.gi@npc-ksb.ru>
---
 drivers/target/target_core_configfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index d93773b3227c..3385d542de62 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -2194,7 +2194,7 @@ static ssize_t target_pr_res_aptpl_metadata_store(struct config_item *item,
 					" exceeds PR_APTPL_MAX_IPORT_LEN: %d\n",
 					PR_APTPL_MAX_IPORT_LEN);
 				ret = -EINVAL;
-				break;
+				goto out;
 			}
 			break;
 		case Opt_initiator_sid:
@@ -2208,7 +2208,7 @@ static ssize_t target_pr_res_aptpl_metadata_store(struct config_item *item,
 					"= exceeds PR_REG_ISID_LEN: %d\n",
 					PR_REG_ISID_LEN);
 				ret = -EINVAL;
-				break;
+				goto out;
 			}
 			break;
 		case Opt_sa_res_key:
@@ -2272,7 +2272,7 @@ static ssize_t target_pr_res_aptpl_metadata_store(struct config_item *item,
 					" exceeds PR_APTPL_MAX_TPORT_LEN: %d\n",
 					PR_APTPL_MAX_TPORT_LEN);
 				ret = -EINVAL;
-				break;
+				goto out;
 			}
 			break;
 		case Opt_tpgt:
-- 
2.43.0

Re: [PATCH] target: Fix an overwriting of the error status code

[sashiko-bot]

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

Pre-existing issues:
- [High] Invalid format specifier (`%lld`) and parsing function (`match_int`) used for 64-bit LUN values, causing silent truncation and parsing failures.
- [High] Out-of-bounds heap read when parsing `initiator_sid` from APTPL metadata if the provided string is shorter than 8 bytes.
- [High] Missing lock acquisition when adding to `aptpl_reg_list`, leading to linked list corruption on concurrent writes.
- [Medium] A pre-existing memory leak occurs when duplicate string keys are provided in the APTPL metadata.
--

commit 351651d7e3ca454861c7698ee9547223c7b37c39
Author: Gleb Markov <markov.gi@npc-ksb.ru>

target: Fix an overwriting of the error status code

If the maximum field length is exceeded, the error status code written to
ret will be overwritten without verification, and data with an incorrect
field length will be passed to core_scsi3_alloc_aptpl_registration(). This
commit fixes the error handling by jumping to the "out" label instead of
breaking the switch statement.

> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index 860e840461778..4436111900318 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -1636,7 +1636,7 @@ static ssize_t target_core_dev_pr_store_attr_res_aptpl_metadata(
>  					" exceeds PR_APTPL_MAX_IPORT_LEN: %d\n",
>  					PR_APTPL_MAX_IPORT_LEN);
>  				ret = -EINVAL;
> -				break;
> +				goto out;

[Severity: Medium]
This is a pre-existing issue, but does match_strdup() leak memory if multiple
instances of the same key are supplied?

Just above these lines in target_core_dev_pr_store_attr_res_aptpl_metadata(),
when Opt_initiator_node is matched:

		case Opt_initiator_node:
			i_port = match_strdup(args);

If a user provides a string with duplicate keys, match_strdup() allocates new
memory and overwrites i_port without freeing the previous allocation. Could
this cause a memory leak?

>  			}
>  			break;
>  		case Opt_initiator_sid:

[ ... ]

[Severity: High]
This is a pre-existing issue, but is the 64-bit LUN value being silently
truncated and ignored?

Looking at the token definitions in drivers/target/target_core_configfs.c:

	{Opt_mapped_lun, "mapped_lun=%lld"},
	{Opt_target_lun, "target_lun=%lld"},

The match_one() function in lib/parser.c does not support the %lld format
specifier, which causes match_token() to fail and silently ignore these
options.

Furthermore, even if it did match, the parsed value is truncated because
match_int() only handles 32-bit integers:

		case Opt_mapped_lun:
			ret = match_int(args, &arg);
			if (ret)
				goto out;
			mapped_lun = (u64)arg;

Does this completely break PR APTPL state restoration for non-zero LUNs?

[Severity: High]
This is a pre-existing issue, but can we read out-of-bounds from the isid
pointer when parsing initiator_sid?

The isid string is dynamically allocated based on its length in
target_core_dev_pr_store_attr_res_aptpl_metadata().

When it is passed to core_scsi3_alloc_aptpl_registration() in
drivers/target/target_core_pr.c, it is read using get_unaligned_be64():

	if (isid != NULL) {
		pr_reg->pr_reg_bin_isid = get_unaligned_be64(isid);
		snprintf(pr_reg->pr_reg_isid, PR_REG_ISID_LEN, "%s", isid);

If the initiator_sid provided by the user is shorter than 8 bytes,
get_unaligned_be64() will read past the end of the allocated string. Can this
leak uninitialized memory or cause a crash?

[Severity: High]
This is a pre-existing issue, but is there a missing lock when adding to
aptpl_reg_list?

In core_scsi3_alloc_aptpl_registration() in drivers/target/target_core_pr.c,
the new registration is added to the list:

	pr_reg->pr_res_holder = res_holder;

	list_add_tail(&pr_reg->pr_reg_aptpl_list, &pr_tmpl->aptpl_reg_list);

Since configfs store attributes don't provide mutual exclusion, concurrent
writes to the res_aptpl_metadata attribute could invoke
core_scsi3_alloc_aptpl_registration() simultaneously.

Does pr_tmpl->aptpl_reg_list need to be protected by aptpl_reg_lock to
prevent linked list corruption during concurrent writes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629130932.684-1-markov.gi@npc-ksb.ru?part=1