Re: [PATCH] scsi: target: iscsi: Replace strlcat() with snprintf()

[Kees Cook <kees@kernel.org>]

On Tue, Nov 10, 2026 at 01:31:37AM -0800, Mariia Nikitash wrote:
> In preparation for removing the deprecated strlcat() API[1], replace
> the strlcat() call with snprintf() to construct the AuthMethod value
> directly from param->value.
> 
> Since buf1 contains unmodified copy of param->value, formatting
> original value and suffix together preserves the NUL-terminated
> output, including partial or omitted ",None" suffixes when the
> 256-byte buffer is too small.
> 
> Link: https://github.com/KSPP/linux/issues/370 [1]
> Signed-off-by: Mariia Nikitash <nikitash.mariiaw@gmail.com>
> ---
>  drivers/target/iscsi/iscsi_target_tpg.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/target/iscsi/iscsi_target_tpg.c b/drivers/target/iscsi/iscsi_target_tpg.c
> index 99772364c88e..aad82a37538f 100644
> --- a/drivers/target/iscsi/iscsi_target_tpg.c
> +++ b/drivers/target/iscsi/iscsi_target_tpg.c
> @@ -624,7 +624,7 @@ int iscsit_ta_authentication(struct iscsi_portal_group *tpg, u32 authentication)

OMG this whole function makes me weep. Also, I think it has an out of
bounds write in it too:

  echo None > .../tpgt_1/param/AuthMethod
  echo 1    > .../tpgt_1/attrib/authentication

and I don't think it handles the truncation case. This is a good candidate
for a full seq_buf rewrite, IMO.

>  		none = strstr(buf1, NONE);
>  		if (none)
>  			goto out;
> -		strlcat(buf1, "," NONE, sizeof(buf1));
> +		snprintf(buf1, sizeof(buf1), "%s," NONE, param->value);
>  		if (iscsi_update_param_value(param, buf1) < 0)
>  			return -EINVAL;
>  	}

And doesn't need memset either. e.g.:

diff --git a/drivers/target/iscsi/iscsi_target_parameters.h b/drivers/target/iscsi/iscsi_target_parameters.h
index 38d2238dfe08..da720f6be5c1 100644
--- a/drivers/target/iscsi/iscsi_target_parameters.h
+++ b/drivers/target/iscsi/iscsi_target_parameters.h
@@ -41,7 +41,7 @@ extern int iscsi_change_param_value(char *, struct iscsi_param_list *, int);
 extern void iscsi_release_param_list(struct iscsi_param_list *);
 extern struct iscsi_param *iscsi_find_param_from_key(char *, struct iscsi_param_list *);
 extern int iscsi_extract_key_value(char *, char **, char **);
-extern int iscsi_update_param_value(struct iscsi_param *, char *);
+extern int iscsi_update_param_value(struct iscsi_param *, const char *);
 extern int iscsi_decode_text_input(u8, u8, char *, u32, struct iscsit_conn *);
 extern int iscsi_encode_text_output(u8, u8, char *, u32 *, u32,
 			struct iscsi_param_list *, bool);
diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c
index 2b318b13268e..34846f5b641c 100644
--- a/drivers/target/iscsi/iscsi_target_parameters.c
+++ b/drivers/target/iscsi/iscsi_target_parameters.c
@@ -642,7 +642,7 @@ int iscsi_extract_key_value(char *textbuf, char **key, char **value)
 	return 0;
 }
 
-int iscsi_update_param_value(struct iscsi_param *param, char *value)
+int iscsi_update_param_value(struct iscsi_param *param, const char *value)
 {
 	kfree(param->value);
 
diff --git a/drivers/target/iscsi/iscsi_target_tpg.c b/drivers/target/iscsi/iscsi_target_tpg.c
index 99772364c88e..6d2cdfa89be5 100644
--- a/drivers/target/iscsi/iscsi_target_tpg.c
+++ b/drivers/target/iscsi/iscsi_target_tpg.c
@@ -8,6 +8,7 @@
  *
  ******************************************************************************/
 
+#include <linux/seq_buf.h>
 #include <linux/slab.h>
 #include <target/target_core_base.h>
 #include <target/target_core_fabric.h>
@@ -579,8 +580,9 @@ int iscsit_tpg_del_network_portal(
 
 int iscsit_ta_authentication(struct iscsi_portal_group *tpg, u32 authentication)
 {
-	unsigned char buf1[256], buf2[256], *none = NULL;
-	int len;
+	DECLARE_SEQ_BUF(s, 256);
+	char old[256], *p = old, *tok;
+	const char *str;
 	struct iscsi_param *param;
 	struct iscsi_tpg_attrib *a = &tpg->tpg_attrib;
 
@@ -590,44 +592,33 @@ int iscsit_ta_authentication(struct iscsi_portal_group *tpg, u32 authentication)
 		return -EINVAL;
 	}
 
-	memset(buf1, 0, sizeof(buf1));
-	memset(buf2, 0, sizeof(buf2));
-
 	param = iscsi_find_param_from_key(AUTHMETHOD, tpg->param_list);
 	if (!param)
 		return -EINVAL;
 
 	if (authentication) {
-		snprintf(buf1, sizeof(buf1), "%s", param->value);
-		none = strstr(buf1, NONE);
-		if (!none)
-			goto out;
-		if (!strncmp(none + 4, ",", 1)) {
-			if (!strcmp(buf1, none))
-				sprintf(buf2, "%s", none+5);
-			else {
-				none--;
-				*none = '\0';
-				len = sprintf(buf2, "%s", buf1);
-				none += 5;
-				sprintf(buf2 + len, "%s", none);
-			}
-		} else {
-			none--;
-			*none = '\0';
-			sprintf(buf2, "%s", buf1);
-		}
-		if (iscsi_update_param_value(param, buf2) < 0)
+		/* Remove "None" from the list, unless it is the only entry. */
+		if (strscpy(old, param->value) < 0)
 			return -EINVAL;
+		while ((tok = strsep(&p, ","))) {
+			if (!strcmp(tok, NONE))
+				continue;
+			if (seq_buf_used(&s))
+				seq_buf_putc(&s, ',');
+			seq_buf_puts(&s, tok);
+		}
+		if (!seq_buf_used(&s))
+			goto out;
 	} else {
-		snprintf(buf1, sizeof(buf1), "%s", param->value);
-		none = strstr(buf1, NONE);
-		if (none)
+		if (strstr(param->value, NONE))
 			goto out;
-		strlcat(buf1, "," NONE, sizeof(buf1));
-		if (iscsi_update_param_value(param, buf1) < 0)
-			return -EINVAL;
+		seq_buf_printf(&s, "%s,%s", param->value, NONE);
 	}
+	if (seq_buf_has_overflowed(&s))
+		return -EINVAL;
+	str = seq_buf_str(&s);
+	if (strcmp(str, param->value) && iscsi_update_param_value(param, str) < 0)
+		return -EINVAL;
 
 out:
 	a->authentication = authentication;

-- 
Kees Cook