Re: [PATCH] NFS: Prefer strscpy over strlcpy calls

[Kees Cook <keescook@chromium.org>]

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