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

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

[Mariia Nikitash]

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)
 		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;
 	}
-- 
2.43.0

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

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 2187 bytes --]

Hi Mariia,

> Date: 2026-11-10 01:31:37-0800
> From: Mariia Nikitash <nikitash.mariiaw@gmail.com>
>
> 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)
>  		none = strstr(buf1, NONE);
>  		if (none)
>  			goto out;
> -		strlcat(buf1, "," NONE, sizeof(buf1));
> +		snprintf(buf1, sizeof(buf1), "%s," NONE, param->value);

This rewrites the string written a few lines above.  It seems rather
confusing.  I very much prefer strlcat(3) here.  Of course, an
strscpy_at() would be better, but snprintf(3) seems wrong.

For other readers, here's the same diff with some more context:

	@@ -622,11 +622,11 @@ int iscsit_ta_authentication(struct iscsi_portal_group *tpg, u32 authentication)
		} else {
			snprintf(buf1, sizeof(buf1), "%s", param->value);
			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;
		}
	 
	 out:

So far, NAK by me.


Have a lovely day!
Alex

>  		if (iscsi_update_param_value(param, buf1) < 0)
>  			return -EINVAL;
>  	}
> -- 
> 2.43.0
> 
> 

-- 
<https://www.alejandro-colomar.es>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Kees Cook]

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

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

[David Laight]

On Thu, 12 Nov 2026 10:22:07 -0800
Kees Cook <kees@kernel.org> wrote:

> 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.

The rest of the file isn't much better.
I'm pretty sure all the:
	spin_lock(&tpg->tpg_state_lock);
	tpg->tpg_state	= TPG_STATE_INACTIVE;
	spin_unlock(&tpg->tpg_state_lock);
sequences aren't doing what the writer expected.

> 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.

Not really, it just needs rewriting to avoid all the string copies.
Some are just silly, eg:
				len = sprintf(buf2, "%s", buf1);
				none += 5;
				sprintf(buf2 + len, "%s", none);

> 
> >  		none = strstr(buf1, NONE);
> >  		if (none)
> >  			goto out;
> > -		strlcat(buf1, "," NONE, sizeof(buf1));
> > +		snprintf(buf1, sizeof(buf1), "%s," NONE, param->value);

I'm not sure, but I think it could look for the "None" in param->value
rather than its truncated copy.
If the strings start getting truncated I'm not at all sure anything
sensible happens.
Then there is this line:
	if (!strncmp(none + 4, ",", 1)) {

> >  		if (iscsi_update_param_value(param, buf1) < 0)
> >  			return -EINVAL;
> >  	}  
> 
> And doesn't need memset either. e.g.:

Oh, I bet it does.
There is a random 'none + 5' that needs to hit a 0 byte.

> 
> 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. */

I don't think that is what it is doing.
I think it is just trying to remove "None" and any associated ',' separators.
So:
	"None" => ""
	"Foo,None" => "Foo"
	"None,Bar" => "Bar"
	"Foo,None,Bar" => "Foo,Bar"

So you just want (ignoring the fact the "None" might be in the middle of something):

		none = strstr(param->value, NONE);
		if (!none)
			goto out;
		if (none == param->value)
			buf = kstrdup(none[4] ? none + 5 : "");
		else
			buf = kasprint("%.*s%s", (int)(none - param->value - 1),
				param->value, none + 4);
		if (!buf)
			return -EINVAL;
		free(param->value);
		param->value - buf;

Or even, if none[4] is 0, just splat a 0 into the existing buffer.

David

> +		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;
>