[v1] kdb: Replace deprecated strcpy() with strscpy()

[PATCH] kdb: Replace deprecated strcpy() with strscpy()

Posted by Thorsten Blum 1 month, 3 weeks ago

strcpy() is deprecated; use strscpy() instead and remove several manual
NUL-terminations.

Since the destination buffers 'cmd_cur' and 'cmd_hist[cmd_head]' have
the fixed length CMD_BUFLEN, strscpy() automatically determines their
size using sizeof() when the size argument is omitted. This makes the
explicit size arguments for the existing strscpy() calls unnecessary,
remove them.

No functional changes intended.

Link: https://github.com/KSPP/linux/issues/88
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 kernel/debug/kdb/kdb_main.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 7a4d2d4689a5..ea7dc2540e40 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -727,14 +727,10 @@ static int kdb_defcmd(int argc, const char **argv)
 	mp->help = kdb_strdup(argv[3], GFP_KDB);
 	if (!mp->help)
 		goto fail_help;
-	if (mp->usage[0] == '"') {
-		strcpy(mp->usage, argv[2]+1);
-		mp->usage[strlen(mp->usage)-1] = '\0';
-	}
-	if (mp->help[0] == '"') {
-		strcpy(mp->help, argv[3]+1);
-		mp->help[strlen(mp->help)-1] = '\0';
-	}
+	if (mp->usage[0] == '"')
+		strscpy(mp->usage, argv[2] + 1, strlen(argv[2]) - 1);
+	if (mp->help[0] == '"')
+		strscpy(mp->help, argv[3] + 1, strlen(argv[3]) - 1);
 
 	INIT_LIST_HEAD(&kdb_macro->statements);
 	defcmd_in_progress = true;
@@ -860,7 +856,7 @@ static void parse_grep(const char *str)
 		kdb_printf("search string too long\n");
 		return;
 	}
-	strcpy(kdb_grep_string, cp);
+	strscpy(kdb_grep_string, cp);
 	kdb_grepping_flag++;
 	return;
 }
@@ -1076,12 +1072,12 @@ static int handle_ctrl_cmd(char *cmd)
 		if (cmdptr != cmd_tail)
 			cmdptr = (cmdptr + KDB_CMD_HISTORY_COUNT - 1) %
 				 KDB_CMD_HISTORY_COUNT;
-		strscpy(cmd_cur, cmd_hist[cmdptr], CMD_BUFLEN);
+		strscpy(cmd_cur, cmd_hist[cmdptr]);
 		return 1;
 	case CTRL_N:
 		if (cmdptr != cmd_head)
 			cmdptr = (cmdptr+1) % KDB_CMD_HISTORY_COUNT;
-		strscpy(cmd_cur, cmd_hist[cmdptr], CMD_BUFLEN);
+		strscpy(cmd_cur, cmd_hist[cmdptr]);
 		return 1;
 	}
 	return 0;
@@ -1285,19 +1281,19 @@ static int kdb_local(kdb_reason_t reason, int error, struct pt_regs *regs,
 		cmdbuf = kdb_getstr(cmdbuf, CMD_BUFLEN, kdb_prompt_str);
 		if (*cmdbuf != '\n') {
 			if (*cmdbuf < 32) {
-				if (cmdptr == cmd_head) {
+				if (cmdptr == cmd_head)
+					/* Copy the current command to the
+					 * history and let strscpy() replace the
+					 * last character with a NUL terminator.
+					 */
 					strscpy(cmd_hist[cmd_head], cmd_cur,
-						CMD_BUFLEN);
-					*(cmd_hist[cmd_head] +
-					  strlen(cmd_hist[cmd_head])-1) = '\0';
-				}
+						strlen(cmd_cur));
 				if (!handle_ctrl_cmd(cmdbuf))
 					*(cmd_cur+strlen(cmd_cur)-1) = '\0';
 				cmdbuf = cmd_cur;
 				goto do_full_getstr;
 			} else {
-				strscpy(cmd_hist[cmd_head], cmd_cur,
-					CMD_BUFLEN);
+				strscpy(cmd_hist[cmd_head], cmd_cur);
 			}
 
 			cmd_head = (cmd_head+1) % KDB_CMD_HISTORY_COUNT;
-- 
2.50.1

Re: [PATCH] kdb: Replace deprecated strcpy() with strscpy()

Posted by Greg Kroah-Hartman 1 month, 3 weeks ago

On Thu, Aug 14, 2025 at 02:03:37PM +0200, Thorsten Blum wrote:
> strcpy() is deprecated; use strscpy() instead and remove several manual
> NUL-terminations.

Manual NULL terminations are good, why get rid of that?

> Since the destination buffers 'cmd_cur' and 'cmd_hist[cmd_head]' have
> the fixed length CMD_BUFLEN, strscpy() automatically determines their
> size using sizeof() when the size argument is omitted. This makes the
> explicit size arguments for the existing strscpy() calls unnecessary,
> remove them.

But now you are dynamically calculating this?

> No functional changes intended.

How did you test this?  Many of these types of changes are wrong, so you
really really need to prove it is correct.

> 
> Link: https://github.com/KSPP/linux/issues/88
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  kernel/debug/kdb/kdb_main.c | 32 ++++++++++++++------------------
>  1 file changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index 7a4d2d4689a5..ea7dc2540e40 100644
> --- a/kernel/debug/kdb/kdb_main.c
> +++ b/kernel/debug/kdb/kdb_main.c
> @@ -727,14 +727,10 @@ static int kdb_defcmd(int argc, const char **argv)
>  	mp->help = kdb_strdup(argv[3], GFP_KDB);
>  	if (!mp->help)
>  		goto fail_help;
> -	if (mp->usage[0] == '"') {
> -		strcpy(mp->usage, argv[2]+1);
> -		mp->usage[strlen(mp->usage)-1] = '\0';
> -	}
> -	if (mp->help[0] == '"') {
> -		strcpy(mp->help, argv[3]+1);
> -		mp->help[strlen(mp->help)-1] = '\0';
> -	}
> +	if (mp->usage[0] == '"')
> +		strscpy(mp->usage, argv[2] + 1, strlen(argv[2]) - 1);

Now you are manually testing the length of argv[2], are you sure that's
ok?

> +	if (mp->help[0] == '"')
> +		strscpy(mp->help, argv[3] + 1, strlen(argv[3]) - 1);
>  
>  	INIT_LIST_HEAD(&kdb_macro->statements);
>  	defcmd_in_progress = true;
> @@ -860,7 +856,7 @@ static void parse_grep(const char *str)
>  		kdb_printf("search string too long\n");
>  		return;
>  	}
> -	strcpy(kdb_grep_string, cp);
> +	strscpy(kdb_grep_string, cp);

If this was just a search/replace, it would have been done already, so
why is this ok?


>  	kdb_grepping_flag++;
>  	return;
>  }
> @@ -1076,12 +1072,12 @@ static int handle_ctrl_cmd(char *cmd)
>  		if (cmdptr != cmd_tail)
>  			cmdptr = (cmdptr + KDB_CMD_HISTORY_COUNT - 1) %
>  				 KDB_CMD_HISTORY_COUNT;
> -		strscpy(cmd_cur, cmd_hist[cmdptr], CMD_BUFLEN);
> +		strscpy(cmd_cur, cmd_hist[cmdptr]);

Same here.  And other places...

thanks,

greg k-h

Re: [PATCH] kdb: Replace deprecated strcpy() with strscpy()

Posted by Greg Kroah-Hartman 1 month, 2 weeks ago

On Thu, Aug 14, 2025 at 02:35:56PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 14, 2025 at 02:03:37PM +0200, Thorsten Blum wrote:
> > strcpy() is deprecated; use strscpy() instead and remove several manual
> > NUL-terminations.
> 
> Manual NULL terminations are good, why get rid of that?
> 
> > Since the destination buffers 'cmd_cur' and 'cmd_hist[cmd_head]' have
> > the fixed length CMD_BUFLEN, strscpy() automatically determines their
> > size using sizeof() when the size argument is omitted. This makes the
> > explicit size arguments for the existing strscpy() calls unnecessary,
> > remove them.
> 
> But now you are dynamically calculating this?
> 
> > No functional changes intended.
> 
> How did you test this?  Many of these types of changes are wrong, so you
> really really need to prove it is correct.
> 
> > 
> > Link: https://github.com/KSPP/linux/issues/88
> > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> > ---
> >  kernel/debug/kdb/kdb_main.c | 32 ++++++++++++++------------------
> >  1 file changed, 14 insertions(+), 18 deletions(-)
> > 
> > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> > index 7a4d2d4689a5..ea7dc2540e40 100644
> > --- a/kernel/debug/kdb/kdb_main.c
> > +++ b/kernel/debug/kdb/kdb_main.c
> > @@ -727,14 +727,10 @@ static int kdb_defcmd(int argc, const char **argv)
> >  	mp->help = kdb_strdup(argv[3], GFP_KDB);
> >  	if (!mp->help)
> >  		goto fail_help;
> > -	if (mp->usage[0] == '"') {
> > -		strcpy(mp->usage, argv[2]+1);
> > -		mp->usage[strlen(mp->usage)-1] = '\0';
> > -	}
> > -	if (mp->help[0] == '"') {
> > -		strcpy(mp->help, argv[3]+1);
> > -		mp->help[strlen(mp->help)-1] = '\0';
> > -	}
> > +	if (mp->usage[0] == '"')
> > +		strscpy(mp->usage, argv[2] + 1, strlen(argv[2]) - 1);
> 
> Now you are manually testing the length of argv[2], are you sure that's
> ok?
> 
> > +	if (mp->help[0] == '"')
> > +		strscpy(mp->help, argv[3] + 1, strlen(argv[3]) - 1);
> >  
> >  	INIT_LIST_HEAD(&kdb_macro->statements);
> >  	defcmd_in_progress = true;
> > @@ -860,7 +856,7 @@ static void parse_grep(const char *str)
> >  		kdb_printf("search string too long\n");
> >  		return;
> >  	}
> > -	strcpy(kdb_grep_string, cp);
> > +	strscpy(kdb_grep_string, cp);
> 
> If this was just a search/replace, it would have been done already, so
> why is this ok?

I missed that strscpy() can now handle 2 arguments like this, so yes,
this should be ok.

BUT, you just checked the length above this line, which now isn't
needed, right?  So this function can get simpler?


> 
> 
> >  	kdb_grepping_flag++;
> >  	return;
> >  }
> > @@ -1076,12 +1072,12 @@ static int handle_ctrl_cmd(char *cmd)
> >  		if (cmdptr != cmd_tail)
> >  			cmdptr = (cmdptr + KDB_CMD_HISTORY_COUNT - 1) %
> >  				 KDB_CMD_HISTORY_COUNT;
> > -		strscpy(cmd_cur, cmd_hist[cmdptr], CMD_BUFLEN);
> > +		strscpy(cmd_cur, cmd_hist[cmdptr]);
> 
> Same here.  And other places...

Sorry, this should also be ok, BUT it's really just doing the same exact
thing, right?  And, it's a different thing, so it should be a different
patch (i.e. do not mix different logical things in the same patch, it
confuses everyone.  Well, me at least...)

thanks,

greg k-h

Re: [PATCH] kdb: Replace deprecated strcpy() with strscpy()

Posted by Thorsten Blum 1 month, 2 weeks ago

Hi Greg,

On 14. Aug 2025, at 16:35, Greg Kroah-Hartman wrote:
> On Thu, Aug 14, 2025 at 02:35:56PM +0200, Greg Kroah-Hartman wrote:
>> On Thu, Aug 14, 2025 at 02:03:37PM +0200, Thorsten Blum wrote:
>>> [...]
>>> -	strcpy(kdb_grep_string, cp);
>>> +	strscpy(kdb_grep_string, cp);
>> 
>> If this was just a search/replace, it would have been done already, so
>> why is this ok?
> 
> I missed that strscpy() can now handle 2 arguments like this, so yes,
> this should be ok.
> 
> BUT, you just checked the length above this line, which now isn't
> needed, right?  So this function can get simpler?

Yes, this could just be

	memcpy(kdb_grep_string, cp, len + 1);

because we already know the length which strscpy() would just recompute
before calling memcpy() internally. I'll submit a v2.

>>> -		strscpy(cmd_cur, cmd_hist[cmdptr], CMD_BUFLEN);
>>> +		strscpy(cmd_cur, cmd_hist[cmdptr]);
>> 
>> Same here.  And other places...
> 
> Sorry, this should also be ok, BUT it's really just doing the same exact
> thing, right?

Yes, it's the same because sizeof(cmd_cur) equals CMD_BUFLEN.

Thanks,
Thorsten