strlcpy() reads the entire source buffer first.
This read may exceed the destination size limit.
This is both inefficient and can lead to linear read
overflows if a source string is not NUL-terminated [1].
Check for strscpy()'s return value of -E2BIG on truncate for safe
replacement with strlcpy().

This is part of a tree-wide cleanup to remove the strlcpy() function
entirely from the kernel [2].

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
[2] https://github.com/KSPP/linux/issues/89

Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
---
Note to reviewers: the one case where this patch would modify existing behavior
is when strlen(src)==destlen==0. Current behavior returns 0, with this patch it
would return -1.

Not sure what the implication of this updated behavior would be,
so bringing it to your attention.

 fs/nfs/nfsroot.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfs/nfsroot.c b/fs/nfs/nfsroot.c
index 620329b7e6ae..7600100ba26f 100644
--- a/fs/nfs/nfsroot.c
+++ b/fs/nfs/nfsroot.c
@@ -164,7 +164,7 @@ __setup("nfsroot=", nfs_root_setup);
 static int __init root_nfs_copy(char *dest, const char *src,
 				     const size_t destlen)
 {
-	if (strlcpy(dest, src, destlen) > destlen)
+	if (strscpy(dest, src, destlen) == -E2BIG)
 		return -1;
 	return 0;
 }
-- 
2.40.1.606.ga4b1b128d6-goog

On Fri, 12 May 2023 15:57:49 +0000, Azeem Shaikh wrote:
> strlcpy() reads the entire source buffer first.
> This read may exceed the destination size limit.
> This is both inefficient and can lead to linear read
> overflows if a source string is not NUL-terminated [1].
> Check for strscpy()'s return value of -E2BIG on truncate for safe
> replacement with strlcpy().
> 
> [...]

Applied to for-next/hardening, thanks!

[1/1] NFS: Prefer strscpy over strlcpy calls
      https://git.kernel.org/kees/c/8ca25e00cf81

-- 
Kees Cook

On Fri, May 12, 2023 at 03:57:49PM +0000, Azeem Shaikh wrote:
> strlcpy() reads the entire source buffer first.
> This read may exceed the destination size limit.
> This is both inefficient and can lead to linear read
> overflows if a source string is not NUL-terminated [1].
> Check for strscpy()'s return value of -E2BIG on truncate for safe
> replacement with strlcpy().
> 
> This is part of a tree-wide cleanup to remove the strlcpy() function
> entirely from the kernel [2].
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> [2] https://github.com/KSPP/linux/issues/89
> 
> Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
> ---
> Note to reviewers: the one case where this patch would modify existing behavior
> is when strlen(src)==destlen==0. Current behavior returns 0, with this patch it
> would return -1.
> 
> Not sure what the implication of this updated behavior would be,
> so bringing it to your attention.

I'm not sure either, but I would prefer non-terminated strings produce
an error, which this change does. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> 
>  fs/nfs/nfsroot.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/nfsroot.c b/fs/nfs/nfsroot.c
> index 620329b7e6ae..7600100ba26f 100644
> --- a/fs/nfs/nfsroot.c
> +++ b/fs/nfs/nfsroot.c
> @@ -164,7 +164,7 @@ __setup("nfsroot=", nfs_root_setup);
>  static int __init root_nfs_copy(char *dest, const char *src,
>  				     const size_t destlen)
>  {
> -	if (strlcpy(dest, src, destlen) > destlen)
> +	if (strscpy(dest, src, destlen) == -E2BIG)
>  		return -1;
>  	return 0;
>  }
> -- 
> 2.40.1.606.ga4b1b128d6-goog
> 
> 

-- 
Kees Cook

> > ---
> > Note to reviewers: the one case where this patch would modify existing behavior
> > is when strlen(src)==destlen==0. Current behavior returns 0, with this patch it
> > would return -1.
> >
> > Not sure what the implication of this updated behavior would be,
> > so bringing it to your attention.
>
> I'm not sure either, but I would prefer non-terminated strings produce
> an error, which this change does. :)
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>

Friendly ping.