linux-kernel - Re: [PATCH v3] kdb: Replace deprecated strcpy() with strscpy() and memcpy()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD=FV=Udf3pZjhpPxEuHsFynP7GoHnZ7RG=NYZ2gLzm=E_4V4A@mail.gmail.com>
Date: Thu, 14 Aug 2025 19:05:02 -0700
From: Doug Anderson <dianders@...omium.org>
To: Thorsten Blum <thorsten.blum@...ux.dev>
Cc: Jason Wessel <jason.wessel@...driver.com>, Daniel Thompson <danielt@...nel.org>, 
	Nir Lichtman <nir@...htman.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Yuran Pereira <yuran.pereira@...mail.com>, linux-hardening@...r.kernel.org, 
	Daniel Thompson <daniel@...cstar.com>, kgdb-bugreport@...ts.sourceforge.net, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] kdb: Replace deprecated strcpy() with strscpy() and memcpy()

Hi,

On Thu, Aug 14, 2025 at 3:02 PM Thorsten Blum <thorsten.blum@...ux.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@...ux.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