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

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 <[email protected]>

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/[email protected]?part=1