Received-SPF: pass (google.com: domain of linux-kernel+bounces-132763-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 5 Apr 2024 10:51:44 +0100
From: Daniel Thompson <daniel.thompson@linaro.org>
To: Justin Stitt <justinstitt@google.com>
Cc: Jason Wessel <jason.wessel@windriver.com>,
	Douglas Anderson <dianders@chromium.org>,
	kgdb-bugreport@lists.sourceforge.net, linux-kernel@vger.kernel.org,
	linux-hardening@vger.kernel.org
Subject: Re: [PATCH v2] kdb: replace deprecated strncpy
Message-ID: <20240405095144.GB2890893@aspen.lan>
References: <20240405-strncpy-kernel-debug-kdb-kdb_io-c-v2-1-d0bf595ab301@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240405-strncpy-kernel-debug-kdb-kdb_io-c-v2-1-d0bf595ab301@google.com>

On Fri, Apr 05, 2024 at 02:33:58AM +0000, Justin Stitt wrote:
> We should move away from using strncpy because it is deprecated [1].
>
> Since these buffers want to be NUL-terminated, let's use strscpy() which
> guarantees this behavior.
>
> The code in question enables the visual autocomplete when using kdb tab
> completion. After pressing tab a couple times when sitting on a partial
> symbol it will attempt to fill it in.

FWIW the code that this patch changes is only executed when tab is
pressed once.


> In my testing, strscpy() provides
> the exact same autocomplete behavior that strncpy() provides here (i.e:
> it fills in the same number of characters for the user).
>
> You can confirm this by enabling kdb [3] and booting up the kernel. I
> performed my tests with qemu with this incantation (wow these get long):
>
> $ /usr/bin/qemu-system-x86_64 -display none -nodefaults -cpu Nehalem
> -append 'console=ttyS0,115200 earlycon=uart8250,io,0x3f8 rdinit=/bin/sh
> kgdboc=ttyS0,115200 nokaslr' -kernel $BUILD_DIR/arch/x86/boot/bzImage
> -initrd $REPOS/boot-utils/images/x86_64/rootfs.cpio -m 512m -serial
> mon:stdio
>
> ... then you can type some symbols and see that autocomplete still kicks
> in and performs exactly the same.
>
> For example:
> tes <tab><tab> gives you "test",
> then "test_ap" <tab><tab> gives you "test_aperfmperf"
>
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://github.com/KSPP/linux/issues/90 [2]
> Link: https://www.kernel.org/doc/html/v5.0/dev-tools/kgdb.html#using-kdb [3]
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> Changes in v2:
> - use strscpy over memcpy (thanks Daniel T.)
> - Link to v1: https://lore.kernel.org/r/20240403-strncpy-kernel-debug-kdb-kdb_io-c-v1-1-7f78a08e9ff4@google.com
> ---
> ---
>  kernel/debug/kdb/kdb_io.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 9443bc63c5a2..60be22132020 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -368,9 +368,9 @@ static char *kdb_read(char *buffer, size_t bufsize)
>  			kdb_printf("%s", buffer);
>  		} else if (tab != 2 && count > 0) {
>  			len_tmp = strlen(p_tmp);
> -			strncpy(p_tmp+len_tmp, cp, lastchar-cp+1);
> +			strscpy(p_tmp+len_tmp, cp, lastchar-cp+1);

This still looks like it is reproducing the obvious[1] error in
the original strncpy() call. The third argument does *not* provide the
number of characters in the destination buffer.

Just to be really clear, I think this patch and your original memcpy()
conversion is mechanically correct, in that the new code is equivalent
to the original strncpy(). The problem is that neither patch acts
on the warning signs that the original code is broken.


[1] I know that this code is hard to read so "obvious" is a relative
    term. However just looking at one line tells us that the source
    pointer is part of a two pointer calculation that purports to
    give the length of the destination string! Such code is almost
    always going to be wrong.


>  			len_tmp = strlen(p_tmp);
> -			strncpy(cp, p_tmp+len, len_tmp-len + 1);
> +			strscpy(cp, p_tmp+len, len_tmp-len + 1);

Again, I really don't think the third argument provides the number of
characters in the destination buffer.


Daniel.