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

In parse_grep(), we can safely use memcpy() because we already know the
length of the source string 'cp' and that it is guaranteed to be
NUL-terminated within the first KDB_GREP_STRLEN bytes.

No functional changes intended.

Link: https://github.com/KSPP/linux/issues/88
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
Changes in v3:
- Extract the strscpy() changes into a separate patch and focus on
  replacing the deprecated strcpy() calls as suggested by Greg
- Link to v2: https://lore.kernel.org/lkml/20250814163237.229544-2-thorsten.blum@linux.dev/

Changes in v2:
- Use memcpy() instead of strscpy() in parse_grep() as suggested by Greg
- Compile-tested only so far
- Link to v1: https://lore.kernel.org/lkml/20250814120338.219585-2-thorsten.blum@linux.dev/
---
 kernel/debug/kdb/kdb_main.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 7a4d2d4689a5..40de0ece724b 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);
+	memcpy(kdb_grep_string, cp, len + 1);
 	kdb_grepping_flag++;
 	return;
 }
-- 
2.50.1

On Fri, Aug 15, 2025 at 12:01:28AM +0200, Thorsten Blum wrote:
> strcpy() is deprecated; use strscpy() and memcpy() instead and remove
> several manual NUL-terminations.
>
> In parse_grep(), we can safely use memcpy() because we already know the
> length of the source string 'cp' and that it is guaranteed to be
> NUL-terminated within the first KDB_GREP_STRLEN bytes.
>
> No functional changes intended.
>
> Link: https://github.com/KSPP/linux/issues/88
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
> Changes in v3:
> - Extract the strscpy() changes into a separate patch and focus on
>   replacing the deprecated strcpy() calls as suggested by Greg
> - Link to v2: https://lore.kernel.org/lkml/20250814163237.229544-2-thorsten.blum@linux.dev/
>
> Changes in v2:
> - Use memcpy() instead of strscpy() in parse_grep() as suggested by Greg
> - Compile-tested only so far
> - Link to v1: https://lore.kernel.org/lkml/20250814120338.219585-2-thorsten.blum@linux.dev/
> ---
>  kernel/debug/kdb/kdb_main.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index 7a4d2d4689a5..40de0ece724b 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);

Sorry but a strscpy() where the length of the destination buffer has
been calculated from the source string is way too much of a red flag
for me.

Put another way if there are "no functional changes intended" then there
cannot possibly be any security benefit from replacing the "unsafe"
strcpy() with the "safe" strscpy(). Likewise abusing the destination
length argument to truncate a string makes the code shorter but *not*
clearer because it's too easy to misread.

In this case even adding a comment to explain the abuse is pointless:
if you want to get rid of the strcpy() then do it by eliminating the
need to copy the string in the first place (e.g. make
kdb_strdup() duplicate the part of argv[2] that you actually want to
keep).

> +	if (mp->help[0] == '"')
> +		strscpy(mp->help, argv[3] + 1, strlen(argv[3]) - 1);

Better yet add a kdb_strdup_dequote() helper so the same bit of code can
be used in both these cases!


Daniel.

Hi Daniel,

> On 15. Aug 2025, at 10:57, Daniel Thompson wrote:
> Sorry but a strscpy() where the length of the destination buffer has
> been calculated from the source string is way too much of a red flag
> for me.
> 
> Put another way if there are "no functional changes intended" then there
> cannot possibly be any security benefit from replacing the "unsafe"
> strcpy() with the "safe" strscpy(). Likewise abusing the destination
> length argument to truncate a string makes the code shorter but *not*
> clearer because it's too easy to misread.

Deliberately truncating the source using strscpy() is a valid use case.
strscpy() allows the size argument to be smaller than the destination
buffer, so this is an intended use of the size argument, not an abuse.

From the strscpy() function comment in linux/string.h:

* The size argument @... is only required when @dst is not an array, or
* when the copy needs to be smaller than sizeof(@dst).

Thanks,
Thorsten

On Fri, Aug 15, 2025 at 01:28:01PM +0200, Thorsten Blum wrote:
> Hi Daniel,
>
> > On 15. Aug 2025, at 10:57, Daniel Thompson wrote:
> > Sorry but a strscpy() where the length of the destination buffer has
> > been calculated from the source string is way too much of a red flag
> > for me.
> >
> > Put another way if there are "no functional changes intended" then there
> > cannot possibly be any security benefit from replacing the "unsafe"
> > strcpy() with the "safe" strscpy(). Likewise abusing the destination
> > length argument to truncate a string makes the code shorter but *not*
> > clearer because it's too easy to misread.
>
> Deliberately truncating the source using strscpy() is a valid use case.
> strscpy() allows the size argument to be smaller than the destination
> buffer, so this is an intended use of the size argument, not an abuse.

Sorry, I didn't phrase that especially well. I regard the abuse to be
deriving the length of the destination buffer exclusively from the
state of the source buffer.

As mentioned, it would be much cleaner to eliminate the string copy entirely
than to translate it into something so similar to the original strcpy().


Daniel.

On 15. Aug 2025, at 13:40, Daniel Thompson wrote:
> On Fri, Aug 15, 2025 at 01:28:01PM +0200, Thorsten Blum wrote:
>> Hi Daniel,
>> 
>>> On 15. Aug 2025, at 10:57, Daniel Thompson wrote:
>>> Sorry but a strscpy() where the length of the destination buffer has
>>> been calculated from the source string is way too much of a red flag
>>> for me.
>>> 
>>> Put another way if there are "no functional changes intended" then there
>>> cannot possibly be any security benefit from replacing the "unsafe"
>>> strcpy() with the "safe" strscpy(). Likewise abusing the destination
>>> length argument to truncate a string makes the code shorter but *not*
>>> clearer because it's too easy to misread.
>> 
>> Deliberately truncating the source using strscpy() is a valid use case.
>> strscpy() allows the size argument to be smaller than the destination
>> buffer, so this is an intended use of the size argument, not an abuse.
> 
> Sorry, I didn't phrase that especially well. I regard the abuse to be
> deriving the length of the destination buffer exclusively from the
> state of the source buffer.
> 
> As mentioned, it would be much cleaner to eliminate the string copy entirely
> than to translate it into something so similar to the original strcpy().

Something like this?

char *kdb_strdup_dequote(const char *str, gfp_t type)
{
	size_t len = strlen(str);
	char *s;

	if (str[0] == '"') {
		/* skip leading quote */
		len--;
		str++;

		if (len > 0 && str[len - 1] == '"')
			len--; /* skip trailing quote */
       }

	len++; /* add space for NUL terminator */

	s = kmalloc(len, type);
	if (!s)
		return NULL;
	strscpy(s, str, len);
	return s;
}

This should probably be a separate patch, right?

Thanks,
Thorsten

On Fri, Aug 15, 2025 at 03:16:27PM +0200, Thorsten Blum wrote:
> On 15. Aug 2025, at 13:40, Daniel Thompson wrote:
> > On Fri, Aug 15, 2025 at 01:28:01PM +0200, Thorsten Blum wrote:
> >> Hi Daniel,
> >>
> >>> On 15. Aug 2025, at 10:57, Daniel Thompson wrote:
> >>> Sorry but a strscpy() where the length of the destination buffer has
> >>> been calculated from the source string is way too much of a red flag
> >>> for me.
> >>>
> >>> Put another way if there are "no functional changes intended" then there
> >>> cannot possibly be any security benefit from replacing the "unsafe"
> >>> strcpy() with the "safe" strscpy(). Likewise abusing the destination
> >>> length argument to truncate a string makes the code shorter but *not*
> >>> clearer because it's too easy to misread.
> >>
> >> Deliberately truncating the source using strscpy() is a valid use case.
> >> strscpy() allows the size argument to be smaller than the destination
> >> buffer, so this is an intended use of the size argument, not an abuse.
> >
> > Sorry, I didn't phrase that especially well. I regard the abuse to be
> > deriving the length of the destination buffer exclusively from the
> > state of the source buffer.
> >
> > As mentioned, it would be much cleaner to eliminate the string copy entirely
> > than to translate it into something so similar to the original strcpy().
>
> Something like this?

It would feels disingenuous of me say "exactly like this" because I
think your code is nicer than mine would have been for this (I suspect
I would have been lazy and kdb_strdup()'ed str+1 and injected a
terminator)!

Looks great. Only one comment:

> char *kdb_strdup_dequote(const char *str, gfp_t type)
> {
> 	size_t len = strlen(str);
> 	char *s;
>
> 	if (str[0] == '"') {
> 		/* skip leading quote */
> 		len--;
> 		str++;
>
> 		if (len > 0 && str[len - 1] == '"')
> 			len--; /* skip trailing quote */

I like the extra diligence of checking the trailing quote but perhaps
combine the two if statements (so we only chomp the quotes if there
are two).


>        }
>
> 	len++; /* add space for NUL terminator */
>
> 	s = kmalloc(len, type);
> 	if (!s)
> 		return NULL;
> 	strscpy(s, str, len);
> 	return s;
> }
>
> This should probably be a separate patch, right?

I think so.

I generally figure if you have to put two paragraphs into the patch
description but each paragraph makes sense in isolation that's a sign
there should be two patches... and I think that would be the case here
(the paragraph explaining the memcpy() piece and the paragraph
explaining kdb_strdup_dequote() would make sense in isolation.


Daniel.

On 15. Aug 2025, at 16:29, Daniel Thompson wrote:
> 
> I like the extra diligence of checking the trailing quote but perhaps
> combine the two if statements (so we only chomp the quotes if there
> are two).

Ok.

Should I replace kdb_strdup() with kdb_strdup_dequote() because it also
handles unquoted strings or do we want to keep both versions? It's only
used in a few places where kdb_strdup_dequote() would also work.

Thanks,
Thorsten

Hi,

On Thu, Aug 14, 2025 at 3:02 PM Thorsten Blum <thorsten.blum@linux.dev> wrote:
>
> strcpy() is deprecated; use strscpy() and memcpy() instead and remove
> several manual NUL-terminations.
>
> In parse_grep(), we can safely use memcpy() because we already know the
> length of the source string 'cp' and that it is guaranteed to be
> NUL-terminated within the first KDB_GREP_STRLEN bytes.
>
> No functional changes intended.
>
> Link: https://github.com/KSPP/linux/issues/88
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
> Changes in v3:
> - Extract the strscpy() changes into a separate patch and focus on
>   replacing the deprecated strcpy() calls as suggested by Greg
> - Link to v2: https://lore.kernel.org/lkml/20250814163237.229544-2-thorsten.blum@linux.dev/
>
> Changes in v2:
> - Use memcpy() instead of strscpy() in parse_grep() as suggested by Greg
> - Compile-tested only so far
> - Link to v1: https://lore.kernel.org/lkml/20250814120338.219585-2-thorsten.blum@linux.dev/
> ---
>  kernel/debug/kdb/kdb_main.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index 7a4d2d4689a5..40de0ece724b 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);

Let's think about some test cases...

Old code:
mp->usage = kdb_strdup(argv[2], GFP_KDB);
if (mp->usage[0] == '"') {
  strcpy(mp->usage, argv[2]+1);
  mp->usage[strlen(mp->usage)-1] = '\0';
}

New code:
mp->usage = kdb_strdup(argv[2], GFP_KDB);
if (mp->usage[0] == '"')
  strscpy(mp->usage, argv[2] + 1, strlen(argv[2]) - 1);

Example string: argv[2] = "\"xyz\""

Old:
  mp->usage = strdup("\"xyz\"")
  mp->usage becomes "xyz\""
  mp->usage becomes "xyz"

New:
  mp->usage = strdup("\"xyz\"")
  mp->usage becomes "xyz\""
  mp->usage doesn't change (!)

To match old behavior, I think you'd need "strlen(argv[2]) - 2", right?

I'll also note that with a different (malformed) example string, the
old code would have also been broken.


Example string: argv[2] = "\""

Old:
  mp->usage = strdup("\"")
  mp->usage becomes ""
  mp->usage[-1] = '\0';  // BAD!


That should probably be fixed too. Luckily this command can't be run
by a user in kdb and it just runs stuff at init time...

Maybe a right fix is something like this (untested). You could even
put it in a small helper so it doesn't need to be duplicated for both
help and usage:

len = strlen(to_copy);
if (to_copy[0] == '"') {
  to_copy++;
  len--;
  if (to_copy[len-1] == '"')
    len--;
}
dest = kstrndup(to_copy, len, GFP_KDB);

...of course, that stops using kdb_strdup(). I don't really see why
that exists? The comments make no sense...



-Doug

Hi Doug,

On 15. Aug 2025, at 04:05, Doug Anderson wrote:
> Let's think about some test cases...
> 
> Old code:
> mp->usage = kdb_strdup(argv[2], GFP_KDB);
> if (mp->usage[0] == '"') {
>  strcpy(mp->usage, argv[2]+1);
>  mp->usage[strlen(mp->usage)-1] = '\0';
> }
> 
> New code:
> mp->usage = kdb_strdup(argv[2], GFP_KDB);
> if (mp->usage[0] == '"')
>  strscpy(mp->usage, argv[2] + 1, strlen(argv[2]) - 1);
> 
> Example string: argv[2] = "\"xyz\""
> 
> Old:
>  mp->usage = strdup("\"xyz\"")
>  mp->usage becomes "xyz\""
>  mp->usage becomes "xyz"
> 
> New:
>  mp->usage = strdup("\"xyz\"")
>  mp->usage becomes "xyz\""
>  mp->usage doesn't change (!)
> 
> To match old behavior, I think you'd need "strlen(argv[2]) - 2", right?

No, it should be "strlen(argv[2]) - 1" to match the old behavior.

In the new code, there are only two steps instead of three.

With your example source string "\"xyz\"" in argv[2]:

	strscpy(mp->usage, argv[2] + 1, strlen(argv[2]) - 1)

evaluates to:

	strscpy(mp->usage, "xyz\"", strlen("\"xyz\"") - 1)

strlen("\"xyz\"") is 5, so this becomes:

	strscpy(mp->usage, "xyz\"", 4)

Unlike strcpy(), strscpy() copies at most 'size - 1' characters and then
appends a NUL terminator. In the example, it copies only the first three
bytes (xyz) and then appends a NUL terminator, effectively replacing the
trailing quote. The result is "xyz", the same as before.

Thanks,
Thorsten

Hi,

On Fri, Aug 15, 2025 at 3:48 AM Thorsten Blum <thorsten.blum@linux.dev> wrote:
>
> Hi Doug,
>
> On 15. Aug 2025, at 04:05, Doug Anderson wrote:
> > Let's think about some test cases...
> >
> > Old code:
> > mp->usage = kdb_strdup(argv[2], GFP_KDB);
> > if (mp->usage[0] == '"') {
> >  strcpy(mp->usage, argv[2]+1);
> >  mp->usage[strlen(mp->usage)-1] = '\0';
> > }
> >
> > New code:
> > mp->usage = kdb_strdup(argv[2], GFP_KDB);
> > if (mp->usage[0] == '"')
> >  strscpy(mp->usage, argv[2] + 1, strlen(argv[2]) - 1);
> >
> > Example string: argv[2] = "\"xyz\""
> >
> > Old:
> >  mp->usage = strdup("\"xyz\"")
> >  mp->usage becomes "xyz\""
> >  mp->usage becomes "xyz"
> >
> > New:
> >  mp->usage = strdup("\"xyz\"")
> >  mp->usage becomes "xyz\""
> >  mp->usage doesn't change (!)
> >
> > To match old behavior, I think you'd need "strlen(argv[2]) - 2", right?
>
> No, it should be "strlen(argv[2]) - 1" to match the old behavior.
>
> In the new code, there are only two steps instead of three.
>
> With your example source string "\"xyz\"" in argv[2]:
>
>         strscpy(mp->usage, argv[2] + 1, strlen(argv[2]) - 1)
>
> evaluates to:
>
>         strscpy(mp->usage, "xyz\"", strlen("\"xyz\"") - 1)
>
> strlen("\"xyz\"") is 5, so this becomes:
>
>         strscpy(mp->usage, "xyz\"", 4)
>
> Unlike strcpy(), strscpy() copies at most 'size - 1' characters and then
> appends a NUL terminator. In the example, it copies only the first three
> bytes (xyz) and then appends a NUL terminator, effectively replacing the
> trailing quote. The result is "xyz", the same as before.

Ugh, I missed that strscpy() implicitly takes away an extra character.
So, yes, your version does appear correct in that sense. It's
definitely non-obvious and I agree with Daniel that it doesn't feel
like the right way to use strscpy().

-Doug