All the other cases in this big switch statement use memcpy or other
methods for copying string data. Since the lengths are handled manually
and carefully, using strncpy() is may be misleading. It doesn't
guarantee any sort of NUL-termination on its destination buffer. At any
rate, it's deprecated [1] and we want to remove all its uses [2].
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]
Cc: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
Note: build-tested only.
Found with: $ rg "strncpy\("
---
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..8bba77b4a39c 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);
+ memcpy(p_tmp+len_tmp, cp, lastchar-cp+1);
len_tmp = strlen(p_tmp);
- strncpy(cp, p_tmp+len, len_tmp-len + 1);
+ memcpy(cp, p_tmp+len, len_tmp-len + 1);
len = len_tmp - len;
kdb_printf("%s", cp);
cp += len;
---
base-commit: 026e680b0a08a62b1d948e5a8ca78700bfac0e6e
change-id: 20240402-strncpy-kernel-debug-kdb-kdb_io-c-53e5ed26da3d
Best regards,
--
Justin Stitt <[email protected]>
On Wed, Apr 03, 2024 at 12:52:36AM +0000, Justin Stitt wrote:
> All the other cases in this big switch statement use memcpy or other
> methods for copying string data. Since the lengths are handled manually
> and carefully, using strncpy() is may be misleading. It doesn't
> guarantee any sort of NUL-termination on its destination buffer. At any
> rate, it's deprecated [1] and we want to remove all its uses [2].
>
> 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]
> Cc: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> Note: build-tested only.
>
> Found with: $ rg "strncpy\("
> ---
> 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..8bba77b4a39c 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);
> + memcpy(p_tmp+len_tmp, cp, lastchar-cp+1);
The strncpy() here is obviously wrong because it passes the size of the
source not the destination.
For that reason I'm not clear that memcpy() is the correct approach
here. It's probably not more wrong than what was there before but,
as mentioned, what was there before is already obviously wrong that
should provoke a bit of code review ;-) .
In particular are you sure lastchar-cp+1 can never larger than
buf_size-len_tmp (which is what I think is the remaining space
at p_tmp+len_tmp)?
> len_tmp = strlen(p_tmp);
> - strncpy(cp, p_tmp+len, len_tmp-len + 1);
> + memcpy(cp, p_tmp+len, len_tmp-len + 1);
Roughly the same question here. The original coded is obviously wrong
so trusting it did the boundary checks properly seems unwise.
Are you sure it is OK to make this copy with checking against bufend?
Daniel.
On Wed, Apr 3, 2024 at 4:23 AM Daniel Thompson
<[email protected]> wrote:
> > - strncpy(cp, p_tmp+len, len_tmp-len + 1);
> > + memcpy(cp, p_tmp+len, len_tmp-len + 1);
>
> Roughly the same question here. The original coded is obviously wrong
> so trusting it did the boundary checks properly seems unwise.
>
> Are you sure it is OK to make this copy with checking against bufend?
>
I am going to revisit this and find a better solution. Thanks Daniel.
>
> Daniel.