[PATCH] scsi: target: Fix NULL deref when clearing alua_lu_gp twice

[PATCH] scsi: target: Fix NULL deref when clearing alua_lu_gp twice

[kensanya]

From: TanZheng <tanzheng@kylinos.cn>

Writing "NULL" to the alua_lu_gp configuration file attribute will
clear the LU group association of the device. This early return
path is only executed when lu_gp has been set. If the device has
been unassociated (for example, after the second write of "NULL"),
the storage operation will jump to  __core_alua_attach_lu_gp_mem(),
at which point lu_gp_new is NULL and a kernel crash occurs when
obtaining spin_lock(&lu_gp->lu_gp_lock).

Crash information is as follows:
[  850.069209][ T2984] BUG: kernel NULL pointer dereference, address: 0000000000000010
[  850.070015][ T2984] #PF: supervisor write access in kernel mode
[  850.070654][ T2984] #PF: error_code(0x0002) - not-present page
[  850.071284][ T2984] PGD 14d4c2067 P4D 0
[  850.071715][ T2984] Oops: Oops: 0002 [#1] SMP KASAN NOPTI
...
[  850.077945][ T2984] RSP: 0018:ff1100010a0877f8 EFLAGS: 00010297
[  850.078578][ T2984] RAX: 0000000000000000 RBX: 0000000000000010 RCX: ffffffff86b17884
[  850.079402][ T2984] RDX: 0000000000000001 RSI: 0000000000000004 RDI: ff1100010a087818
[  850.080237][ T2984] RBP: 1fe2200021410eff R08: 0000000000000001 R09: ffe21c0021410f03
[  850.081065][ T2984] R10: 0000000000000003 R11: 6e696c6261736944 R12: ff11000113c16788
[  850.081897][ T2984] R13: ff11000113c16780 R14: 0000000000000010 R15: 0000000000000000
[  850.082721][ T2984] FS:  00007fc9a84d6740(0000) GS:ff11000388fe1000(0000) knlGS:0000000000000000
[  850.083649][ T2984] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  850.084342][ T2984] CR2: 0000000000000010 CR3: 0000000154a24004 CR4: 0000000000773ef0
[  850.085172][ T2984] PKRU: 55555554
[  850.085546][ T2984] Call Trace:
[  850.085897][ T2984]  <TASK>
[  850.086208][ T2984]  ? __pfx__raw_spin_lock+0x10/0x10
[  850.086755][ T2984]  ? configfs_write_iter+0x362/0x550
[  850.087309][ T2984]  ? _raw_spin_lock+0x84/0xe0
[  850.087807][ T2984]  ? __pfx__raw_spin_lock+0x10/0x10
[  850.088345][ T2984]  __core_alua_attach_lu_gp_mem+0x29/0x1f0 [target_core_mod]
[  850.089162][ T2984]  target_dev_alua_lu_gp_store+0x1a8/0x520 [target_core_mod]
[  850.089975][ T2984]  ? __pfx_target_dev_alua_lu_gp_store+0x10/0x10 [target_core_mod]
...

When both the current group and the requested group are NULL,
an early return occurs.

Fixes: c66ac9db8d4a ("[SCSI] target: Add LIO target core v4.0.0-rc6")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/patchset/20260730031917.72750-1-kensanya@163.com?part=1
Signed-off-by: TanZheng <tanzheng@kylinos.cn>
---
 drivers/target/target_core_configfs.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index 0e50987e6dbe..292730f98db4 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -2572,6 +2572,12 @@ static ssize_t target_dev_alua_lu_gp_store(struct config_item *item,
 		__core_alua_drop_lu_gp_mem(lu_gp_mem, lu_gp);
 		move = 1;
 	}
+
+	if (!lu_gp_new) {
+		spin_unlock(&lu_gp_mem->lu_gp_mem_lock);
+		return count;
+	}
+
 	/*
 	 * Associate lu_gp_mem with lu_gp_new.
 	 */
-- 
2.25.1

Re: [PATCH] scsi: target: Fix NULL deref when clearing alua_lu_gp twice

[sashiko-bot]

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

Pre-existing issues:
- [High] An off-by-one error in the input length validation allows a stack buffer overread and out-of-bounds write if `count` is exactly equal to the buffer size.
- [High] Dropping a spinlock inside a `list_for_each_entry_safe` loop in ALUA release functions exposes the loop to list poisoning and Use-After-Free crashes.
- [High] TOCTOU race in ALUA group ID assignment corrupts lists, and the error path frees an active configfs item, causing a double-free.
--

--- Patch [1]: [PATCH] scsi: target: Fix NULL deref when clearing alua_lu_gp twice ---
commit 1601e8bb13acc929ed9af1ae24358c7f9e46e6aa
Author: TanZheng <tanzheng@kylinos.cn>

scsi: target: Fix NULL deref when clearing alua_lu_gp twice

This patch fixes a kernel panic caused by a NULL pointer dereference
when a device's ALUA LU group association is cleared multiple times. It
adds an early return to prevent passing a NULL lu_gp_new to
__core_alua_attach_lu_gp_mem.

Link: https://sashiko.dev/#/patchset/20260730031917.72750-1-kensanya@163.com?part=1

> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index 860e840461778..a9c3ab54022e1 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -2058,6 +2058,12 @@ static ssize_t target_core_store_alua_lu_gp(
>  		__core_alua_drop_lu_gp_mem(lu_gp_mem, lu_gp);
>  		move = 1;
>  	}
> +
> +	if (!lu_gp_new) {
> +		spin_unlock(&lu_gp_mem->lu_gp_mem_lock);
> +		return count;
> +	}
> +
>  	/*
>  	 * Associate lu_gp_mem with lu_gp_new.
>  	 */

While reviewing this change, I noticed a few pre-existing issues in this
function and related ALUA code:

drivers/target/target_core_configfs.c:target_core_store_alua_lu_gp() {
...
    if (count > LU_GROUP_NAME_BUF) {
        pr_err("ALUA LU Group Alias too large!\n");
        return -EINVAL;
    }
    memset(buf, 0, LU_GROUP_NAME_BUF);
    memcpy(buf, page, count);
...

[Severity: High]
This is a pre-existing issue, but does this off-by-one check allow a
stack buffer overflow?

If count is exactly LU_GROUP_NAME_BUF, memcpy will fill the entire
buffer, leaving no space for a null terminator. Later calls to
strstrip(buf) or strim(buf) would run off the end of the stack buffer.

Could this happen when an administrator writes exactly 256 bytes without
a null terminator? The same pattern appears in
drivers/target/target_core_alua.c:core_alua_store_tg_pt_gp_info() where
TG_PT_GROUP_NAME_BUF is used.

[ ... ]

drivers/target/target_core_alua.c:core_alua_free_lu_gp() {
...
    spin_lock(&lu_gp->lu_gp_lock);
    list_for_each_entry_safe(lu_gp_mem, lu_gp_mem_tmp,
                &lu_gp->lu_gp_mem_list, lu_gp_mem_list) {
...
        spin_unlock(&lu_gp->lu_gp_lock);
...

[Severity: High]
This is a pre-existing issue, but does dropping the spinlock inside
list_for_each_entry_safe expose the loop to list poisoning and a
use-after-free?

Since the _safe variant only protects against the deletion of the
current element, dropping the lock could allow a concurrent thread
handling device teardown to remove and free the next element. When the
lock is reacquired and the loop continues, wouldn't it dereference the
freed next pointer?

The exact same pattern seems to exist in core_alua_free_tg_pt_gp() for
LUN list iteration.

[ ... ]

drivers/target/target_core_alua.c:core_alua_set_lu_gp_id() {
...
    if (lu_gp->lu_gp_valid_id) {
        return -EINVAL;
    }

    spin_lock(&lu_gps_lock);
    if (alua_lu_gps_count == 0x0000ffff) {
        spin_unlock(&lu_gps_lock);
        kmem_cache_free(t10_alua_lu_gp_cache, lu_gp);
        return -ENOSPC;
    }
...

[Severity: High]
This is a pre-existing issue, but does this create a race condition that
corrupts the list, along with an illegal free?

The lu_gp_valid_id check happens outside the spinlock. Could concurrent
sysfs writes both pass this check, serialize on the lock, and then call
list_add_tail on the same node, corrupting the list?

Additionally, if the global count limit is reached, kmem_cache_free is
called directly on lu_gp. Since lu_gp embeds a configfs item that configfs
still references, does this bypass configfs lifecycle rules and result in
a double-free when the directory is removed?

The same pattern exists in core_alua_set_tg_pt_gp_id().

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