linux-kernel - Re: [PATCH] kdb: Replace deprecated strcpy() with strscpy() in vkdb

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=UrEv+=KmjZTDt7F5ftuw5xrpOyEim0OrGAkoiJ5Wi2cg@mail.gmail.com>
Date: Mon, 11 Aug 2025 10:48:46 -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>, 
	Justin Stitt <justinstitt@...gle.com>, linux-hardening@...r.kernel.org, 
	Daniel Thompson <daniel@...cstar.com>, kgdb-bugreport@...ts.sourceforge.net, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kdb: Replace deprecated strcpy() with strscpy() in vkdb_printf()

Hi,

On Mon, Aug 11, 2025 at 10:04 AM Thorsten Blum <thorsten.blum@...ux.dev> wrote:
>
> strcpy() is deprecated; use strscpy() instead.
>
> Use the return value of strscpy() instead of calling strlen() again.
>
> Link: https://github.com/KSPP/linux/issues/88
> Signed-off-by: Thorsten Blum <thorsten.blum@...ux.dev>
> ---
>  kernel/debug/kdb/kdb_io.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 9b11b10b120c..2062494c413b 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -732,8 +732,7 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
>                          * Shift the buffer left.
>                          */
>                         *cphold = replaced_byte;
> -                       strcpy(kdb_buffer, cphold);
> -                       len = strlen(kdb_buffer);
> +                       len = strscpy(kdb_buffer, cphold);
>                         next_avail = kdb_buffer + len;
>                         size_avail = sizeof(kdb_buffer) - len;
>                         goto kdb_print_out;

It made me a little nervous that you're not checking for the fact that
strscpy() could return an error code. Without the check you're just
replacing one type of problem (buffer overflow) with another type (the
code running with a negative length). IMO in cases like this either
leave the strlen() in there or check the return value for errors.

...so I looked a little deeper here to see if the buffer overflow was
actually possible to begin with. Looking, I _think_ this is true:

* `cp` is a pointer into `kdb_buffer` (location of first '\n')
* `cphold` and `cp` are equal at this point.

...so you're guaranteed not to overflow because the destination and
source overlap. ...but that means we shouldn't have been using
strcpy() either way. Both strcpy() and strscpy() say that their
behaviors are undefined if the src/dest overlap. This means that
really the right fix is to use memmove().

The above is based solely on code inspection w/ no testing. If I got
it wrong, let me know.

> @@ -872,8 +871,7 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
>          */
>         if (kdb_grepping_flag && !suspend_grep) {
>                 *cphold = replaced_byte;
> -               strcpy(kdb_buffer, cphold);
> -               len = strlen(kdb_buffer);
> +               len = strscpy(kdb_buffer, cphold);
>                 next_avail = kdb_buffer + len;
>                 size_avail = sizeof(kdb_buffer) - len;

I believe the above case is similar.

-Doug