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: <aJ72XZ0VkrCkKFNy@aspen.lan>
Date: Fri, 15 Aug 2025 09:57:01 +0100
From: Daniel Thompson <daniel@...cstar.com>
To: Thorsten Blum <thorsten.blum@...ux.dev>
Cc: Jason Wessel <jason.wessel@...driver.com>,
	Daniel Thompson <danielt@...nel.org>,
	Douglas Anderson <dianders@...omium.org>,
	Nir Lichtman <nir@...htman.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Yuran Pereira <yuran.pereira@...mail.com>,
	linux-hardening@...r.kernel.org,
	kgdb-bugreport@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] kdb: Replace deprecated strcpy() with strscpy() and
 memcpy()

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@...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);

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.