[PATCH] scsi: target: Fix stack out-of-bounds in ALUA lu_gp and tg_pt_gp store

[PATCH] scsi: target: Fix stack out-of-bounds in ALUA lu_gp and tg_pt_gp store

[kensanya]

From: TanZheng <tanzheng@kylinos.cn>

Both alua_lu_gp and alua_tg_pt_gp allow a write of exactly
LU/TG_PT_GROUP_NAME_BUF bytes into a same-sized stack buffer,
then pass it to strstrip() without a trailing NUL. A full-sized
write causes strlen() to read past the end of the array (visible
under KASAN as stack-out-of-bounds).

Enlarge the local buffers by one byte so a full-sized write
remains NUL-terminated.

Signed-off-by: TanZheng <tanzheng@kylinos.cn>
---
Reproduction (before the fix)

modules + configfs
modprobe configfs target_core_mod target_core_rd iscsi_target_mod
mount -t configfs none /sys/kernel/config 2>/dev/null || true
CORE=/sys/kernel/config/target/core
ISCSI=/sys/kernel/config/target/iscsi

# --- case A: alua_lu_gp (core device) ---
mkdir -p $CORE/rd_mcp_0/ram1
echo "rd_pages=16" > $CORE/rd_mcp_0/ram1/control
echo 1 > $CORE/rd_mcp_0/ram1/enable
python3 -c 'open("'"$CORE"'/rd_mcp_0/ram1/alua_lu_gp","wb").write(b"\xff"*256)'

# --- case B: alua_tg_pt_gp (fabric LUN) ---
IQN=iqn.2024-01.local:kasan-alua
mkdir -p $ISCSI/$IQN/tpgt_1/lun/lun_0
ln -sf $CORE/rd_mcp_0/ram1 $ISCSI/$IQN/tpgt_1/lun/lun_0/ram1
echo 1 > $ISCSI/$IQN/tpgt_1/enable
python3 -c 'open("'"$ISCSI"'/'"$IQN"'/tpgt_1/lun/lun_0/alua_tg_pt_gp","wb").write(b"\xff"*256)'

 drivers/target/target_core_alua.c     | 3 +--
 drivers/target/target_core_configfs.c | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index 10250aca5a81..60259417013b 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -1904,7 +1904,7 @@ ssize_t core_alua_store_tg_pt_gp_info(
 	 */
 	struct se_device *dev = rcu_dereference_raw(lun->lun_se_dev);
 	struct t10_alua_tg_pt_gp *tg_pt_gp = NULL, *tg_pt_gp_new = NULL;
-	unsigned char buf[TG_PT_GROUP_NAME_BUF];
+	unsigned char buf[TG_PT_GROUP_NAME_BUF + 1] = { };
 	int move = 0;
 
 	if (dev->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_ALUA ||
@@ -1915,7 +1915,6 @@ ssize_t core_alua_store_tg_pt_gp_info(
 		pr_err("ALUA Target Port Group alias too large!\n");
 		return -EINVAL;
 	}
-	memset(buf, 0, TG_PT_GROUP_NAME_BUF);
 	memcpy(buf, page, count);
 	/*
 	 * Any ALUA target port group alias besides "NULL" means we will be
diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index 2b19a956007b..0e50987e6dbe 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -2518,7 +2518,7 @@ static ssize_t target_dev_alua_lu_gp_store(struct config_item *item,
 	struct se_hba *hba = dev->se_hba;
 	struct t10_alua_lu_gp *lu_gp = NULL, *lu_gp_new = NULL;
 	struct t10_alua_lu_gp_member *lu_gp_mem;
-	unsigned char buf[LU_GROUP_NAME_BUF] = { };
+	unsigned char buf[LU_GROUP_NAME_BUF + 1] = { };
 	int move = 0;
 
 	lu_gp_mem = dev->dev_alua_lu_gp_mem;
-- 
2.25.1

Re: [PATCH] scsi: target: Fix stack out-of-bounds in ALUA lu_gp and tg_pt_gp store

[sashiko-bot]

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

Pre-existing issues:
- [High] Writing "NULL" to a device's alua_lu_gp configfs file when the current group is NULL triggers a guaranteed NULL pointer dereference.
--

--- Patch [1]: [PATCH] scsi: target: Fix stack out-of-bounds in ALUA lu_gp and tg_pt_gp store ---
commit 57ed25faa0822b84299ce2ade4007a8dc2229f88
Author: TanZheng <tanzheng@kylinos.cn>

scsi: target: Fix stack out-of-bounds in ALUA lu_gp and tg_pt_gp store

Both alua_lu_gp and alua_tg_pt_gp allow a write of exactly
LU/TG_PT_GROUP_NAME_BUF bytes into a same-sized stack buffer. This commit
enlarges the local buffers by one byte so a full-sized write remains
NUL-terminated when passed to strstrip().

> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index 2b19a956007b7..0e50987e6dbee 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -2518,7 +2518,7 @@ static ssize_t target_dev_alua_lu_gp_store(struct config_item *item,
>  	struct se_hba *hba = dev->se_hba;
>  	struct t10_alua_lu_gp *lu_gp = NULL, *lu_gp_new = NULL;
>  	struct t10_alua_lu_gp_member *lu_gp_mem;
> -	unsigned char buf[LU_GROUP_NAME_BUF] = { };
> +	unsigned char buf[LU_GROUP_NAME_BUF + 1] = { };
>  	int move = 0;
>  
>  	lu_gp_mem = dev->dev_alua_lu_gp_mem;

[Severity: High]
This is a pre-existing issue, but I noticed a potential NULL pointer
dereference further down in target_dev_alua_lu_gp_store().

If a user writes "NULL" to the alua_lu_gp attribute when the current LU group
is already NULL (e.g., by writing "NULL" twice), the early return block inside
if (lu_gp) is bypassed:

drivers/target/target_core_configfs.c:target_dev_alua_lu_gp_store() {
    ...
	lu_gp = lu_gp_mem->lu_gp;
	if (lu_gp) {
        ...
		if (!lu_gp_new) {
            ...
			return count;
		}
        ...
	}
	/*
	 * Associate lu_gp_mem with lu_gp_new.
	 */
	__core_alua_attach_lu_gp_mem(lu_gp_mem, lu_gp_new);
    ...
}

This causes the function to fall through to __core_alua_attach_lu_gp_mem()
with lu_gp_new == NULL, where it is unconditionally dereferenced during lock
acquisition:

drivers/target/target_core_alua.c:__core_alua_attach_lu_gp_mem() {
    ...
	spin_lock(&lu_gp->lu_gp_lock);
    ...
}

Should there be a check to return early if both lu_gp and lu_gp_new are NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730031917.72750-1-kensanya@163.com?part=1