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@linux.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;
@@ -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;
 	}
-- 
2.50.1

Hi,

On Mon, Aug 11, 2025 at 10:04 AM Thorsten Blum <thorsten.blum@linux.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@linux.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

Hi Doug,

On 11. Aug 2025, at 19:48, Doug Anderson wrote:
> On Mon, Aug 11, 2025 at 10:04 AM Thorsten Blum 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@linux.dev>
>> ---
> 
> 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.

Yes, I should have checked the return value or left strlen() as is. This
was actually a last minute "improvement" I should have skipped.

> ...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().

Good catch. I read about the undefined behavior in the function comment,
but never encountered it and haven't really been looking out for it.

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

Yes, I just compile-tested it as I didn't expect src/dst to overlap. And
then my last-minute change to strlen() made it even worse. Sorry about
that.

Are you going to fix it using memmove() or should I submit a v2?

Thanks,
Thorsten

Hi,

On Mon, Aug 11, 2025 at 11:11 AM Thorsten Blum <thorsten.blum@linux.dev> wrote:
>
> > ...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().
>
> Good catch. I read about the undefined behavior in the function comment,
> but never encountered it and haven't really been looking out for it.
>
> > The above is based solely on code inspection w/ no testing. If I got
> > it wrong, let me know.
>
> Yes, I just compile-tested it as I didn't expect src/dst to overlap. And
> then my last-minute change to strlen() made it even worse. Sorry about
> that.
>
> Are you going to fix it using memmove() or should I submit a v2?

Do you want to send a v2 that switches it to memmove()?

-Doug

On 11. Aug 2025, at 20:13, Doug Anderson wrote:
> Do you want to send a v2 that switches it to memmove()?

Yes, I might just need a day or two.

Thanks,
Thorsten