NFS: nfsroot: replace strlcat() with snprintf()

[PATCH] NFS: nfsroot: replace strlcat() with snprintf()

Posted by Mariia Nikitash 1 month, 2 weeks ago

In preparation for removing the deprecated strlcat() API[1], replace its
uses in root_nfs_cat() with snprintf().

Build the separator and source string in a single call using the
remaining space in the destination buffer. snprintf() returns the length
it would have written excluding the terminating NUL, so comparing the
return value against the remaining buffer space preserves the existing
truncation check.

Link: https://github.com/KSPP/linux/issues/370 [1]
Signed-off-by: Mariia Nikitash <mariianikitash@google.com>
---
 fs/nfs/nfsroot.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/nfsroot.c b/fs/nfs/nfsroot.c
index 432612d22437..e951fe731679 100644
--- a/fs/nfs/nfsroot.c
+++ b/fs/nfs/nfsroot.c
@@ -173,12 +173,12 @@ static int __init root_nfs_cat(char *dest, const char *src,
 			       const size_t destlen)
 {
 	size_t len = strlen(dest);
+	size_t remaining = destlen - len;
+	const char *sep = "";
 
 	if (len && dest[len - 1] != ',')
-		if (strlcat(dest, ",", destlen) >= destlen)
-			return -1;
-
-	if (strlcat(dest, src, destlen) >= destlen)
+		sep = ",";
+	if (snprintf(dest + len, remaining, "%s%s", sep, src) >= remaining)
 		return -1;
 	return 0;
 }
-- 
2.55.0.679.g6767b8d81c-goog

Re: [PATCH] NFS: nfsroot: replace strlcat() with snprintf()

Posted by David Laight 1 month, 1 week ago

On Fri, 14 Aug 2026 00:28:45 -0700
Mariia Nikitash <mariianikitash@google.com> wrote:

> In preparation for removing the deprecated strlcat() API[1], replace its
> uses in root_nfs_cat() with snprintf().
> 
> Build the separator and source string in a single call using the
> remaining space in the destination buffer. snprintf() returns the length
> it would have written excluding the terminating NUL, so comparing the
> return value against the remaining buffer space preserves the existing
> truncation check.
> 
> Link: https://github.com/KSPP/linux/issues/370 [1]
> Signed-off-by: Mariia Nikitash <mariianikitash@google.com>
> ---
>  fs/nfs/nfsroot.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfs/nfsroot.c b/fs/nfs/nfsroot.c
> index 432612d22437..e951fe731679 100644
> --- a/fs/nfs/nfsroot.c
> +++ b/fs/nfs/nfsroot.c
> @@ -173,12 +173,12 @@ static int __init root_nfs_cat(char *dest, const char *src,
>  			       const size_t destlen)
>  {
>  	size_t len = strlen(dest);
> +	size_t remaining = destlen - len;
> +	const char *sep = "";
>  
>  	if (len && dest[len - 1] != ',')
> -		if (strlcat(dest, ",", destlen) >= destlen)
> -			return -1;
> -
> -	if (strlcat(dest, src, destlen) >= destlen)
> +		sep = ",";
> +	if (snprintf(dest + len, remaining, "%s%s", sep, src) >= remaining)
>  		return -1;

I think I'd have gone for:
	size_t len = strlen(dest);
	if (len && dest[len - 1] != ',' && ++len < destlen)
		dest[len - 1] = ',';
	if (strscpy(dest + len, src, destlen - len) < 0)
		return -1;

Although it would be better as an 'add_option()' function.
I suspect the it used to be just strcat().
(similarly for root_nfs_copy() which is a pointless wrapper on strscpy()).

Much more worth while would be fixing the sprintf() for NFS_ROOT.

David

>  	return 0;
>  }

Re: [PATCH] NFS: nfsroot: replace strlcat() with snprintf()

Posted by Justin Stitt 1 month, 1 week ago

Hi,

On Tue, Aug 18, 2026 at 1:55 AM David Laight
<david.laight.linux@gmail.com> wrote:
>
> On Fri, 14 Aug 2026 00:28:45 -0700
> Mariia Nikitash <mariianikitash@google.com> wrote:
>
> > In preparation for removing the deprecated strlcat() API[1], replace its
> > uses in root_nfs_cat() with snprintf().
> >
> > Build the separator and source string in a single call using the
> > remaining space in the destination buffer. snprintf() returns the length
> > it would have written excluding the terminating NUL, so comparing the
> > return value against the remaining buffer space preserves the existing
> > truncation check.
> >
> > Link: https://github.com/KSPP/linux/issues/370 [1]
> > Signed-off-by: Mariia Nikitash <mariianikitash@google.com>
> > ---
> >  fs/nfs/nfsroot.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/nfs/nfsroot.c b/fs/nfs/nfsroot.c
> > index 432612d22437..e951fe731679 100644
> > --- a/fs/nfs/nfsroot.c
> > +++ b/fs/nfs/nfsroot.c
> > @@ -173,12 +173,12 @@ static int __init root_nfs_cat(char *dest, const char *src,
> >                              const size_t destlen)
> >  {
> >       size_t len = strlen(dest);
> > +     size_t remaining = destlen - len;
> > +     const char *sep = "";
> >
> >       if (len && dest[len - 1] != ',')
> > -             if (strlcat(dest, ",", destlen) >= destlen)
> > -                     return -1;
> > -
> > -     if (strlcat(dest, src, destlen) >= destlen)
> > +             sep = ",";
> > +     if (snprintf(dest + len, remaining, "%s%s", sep, src) >= remaining)
> >               return -1;
>
> I think I'd have gone for:
>         size_t len = strlen(dest);
>         if (len && dest[len - 1] != ',' && ++len < destlen)
>                 dest[len - 1] = ',';
>         if (strscpy(dest + len, src, destlen - len) < 0)
>                 return -1;

Can we run into underflow issues if `len == destlen` during the
increment. imagine @len is 10 and @destlen is also 10. We end up
incrementing len and our strscpy receives `10 - 11` wrapping to
SIZE_MAX.

Maybe this isn't reachable as @destlen may always be larger than what
`strlen()` can give us but that increment looks suspicious.

>
> Although it would be better as an 'add_option()' function.
> I suspect the it used to be just strcat().
> (similarly for root_nfs_copy() which is a pointless wrapper on strscpy()).
>
> Much more worth while would be fixing the sprintf() for NFS_ROOT.
>
> David
>
> >       return 0;
> >  }
>

Justin

Re: [PATCH] NFS: nfsroot: replace strlcat() with snprintf()

Posted by David Laight 1 month, 1 week ago

On Tue, 18 Aug 2026 14:05:41 -0700
Justin Stitt <justinstitt@google.com> wrote:

> Hi,
> 
> On Tue, Aug 18, 2026 at 1:55 AM David Laight
> <david.laight.linux@gmail.com> wrote:
> >
> > On Fri, 14 Aug 2026 00:28:45 -0700
> > Mariia Nikitash <mariianikitash@google.com> wrote:
> >  
> > > In preparation for removing the deprecated strlcat() API[1], replace its
> > > uses in root_nfs_cat() with snprintf().
> > >
> > > Build the separator and source string in a single call using the
> > > remaining space in the destination buffer. snprintf() returns the length
> > > it would have written excluding the terminating NUL, so comparing the
> > > return value against the remaining buffer space preserves the existing
> > > truncation check.
> > >
> > > Link: https://github.com/KSPP/linux/issues/370 [1]
> > > Signed-off-by: Mariia Nikitash <mariianikitash@google.com>
> > > ---
> > >  fs/nfs/nfsroot.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/nfs/nfsroot.c b/fs/nfs/nfsroot.c
> > > index 432612d22437..e951fe731679 100644
> > > --- a/fs/nfs/nfsroot.c
> > > +++ b/fs/nfs/nfsroot.c
> > > @@ -173,12 +173,12 @@ static int __init root_nfs_cat(char *dest, const char *src,
> > >                              const size_t destlen)
> > >  {
> > >       size_t len = strlen(dest);
> > > +     size_t remaining = destlen - len;
> > > +     const char *sep = "";
> > >
> > >       if (len && dest[len - 1] != ',')
> > > -             if (strlcat(dest, ",", destlen) >= destlen)
> > > -                     return -1;
> > > -
> > > -     if (strlcat(dest, src, destlen) >= destlen)
> > > +             sep = ",";
> > > +     if (snprintf(dest + len, remaining, "%s%s", sep, src) >= remaining)
> > >               return -1;  
> >
> > I think I'd have gone for:
> >         size_t len = strlen(dest);
> >         if (len && dest[len - 1] != ',' && ++len < destlen)
> >                 dest[len - 1] = ',';
> >         if (strscpy(dest + len, src, destlen - len) < 0)
> >                 return -1;  
> 
> Can we run into underflow issues if `len == destlen` during the
> increment. imagine @len is 10 and @destlen is also 10. We end up
> incrementing len and our strscpy receives `10 - 11` wrapping to
> SIZE_MAX.

len can only be less than destlen, so strscpy() can only see a zero
length - which is fine,

This whole code could be rewritten to avoid the strlen().
The function is only used twice to append data to a single static string.
Tweaking the initialisation would allow sizeof() be used to set
the initial length.
The initialisation also means that the ',' is always needed.

The second call appends data from an snprintf() into a temporary buffer.
It wouldn't be hard to directly add the data to the actual buffer.

David

> 
> Maybe this isn't reachable as @destlen may always be larger than what
> `strlen()` can give us but that increment looks suspicious.
> 
> >
> > Although it would be better as an 'add_option()' function.
> > I suspect the it used to be just strcat().
> > (similarly for root_nfs_copy() which is a pointless wrapper on strscpy()).
> >
> > Much more worth while would be fixing the sprintf() for NFS_ROOT.
> >
> > David
> >  
> > >       return 0;
> > >  }  
> >  
> 
> Justin

Re: [PATCH] NFS: nfsroot: replace strlcat() with snprintf()

Posted by Kees Cook 1 month, 1 week ago

On Tue, Aug 18, 2026 at 09:55:48AM +0100, David Laight wrote:
> On Fri, 14 Aug 2026 00:28:45 -0700
> Mariia Nikitash <mariianikitash@google.com> wrote:
> 
> > In preparation for removing the deprecated strlcat() API[1], replace its
> > uses in root_nfs_cat() with snprintf().
> > 
> > Build the separator and source string in a single call using the
> > remaining space in the destination buffer. snprintf() returns the length
> > it would have written excluding the terminating NUL, so comparing the
> > return value against the remaining buffer space preserves the existing
> > truncation check.
> > 
> > Link: https://github.com/KSPP/linux/issues/370 [1]
> > Signed-off-by: Mariia Nikitash <mariianikitash@google.com>
> > ---
> >  fs/nfs/nfsroot.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/nfs/nfsroot.c b/fs/nfs/nfsroot.c
> > index 432612d22437..e951fe731679 100644
> > --- a/fs/nfs/nfsroot.c
> > +++ b/fs/nfs/nfsroot.c
> > @@ -173,12 +173,12 @@ static int __init root_nfs_cat(char *dest, const char *src,
> >  			       const size_t destlen)
> >  {
> >  	size_t len = strlen(dest);
> > +	size_t remaining = destlen - len;
> > +	const char *sep = "";
> >  
> >  	if (len && dest[len - 1] != ',')
> > -		if (strlcat(dest, ",", destlen) >= destlen)
> > -			return -1;
> > -
> > -	if (strlcat(dest, src, destlen) >= destlen)
> > +		sep = ",";
> > +	if (snprintf(dest + len, remaining, "%s%s", sep, src) >= remaining)
> >  		return -1;
> 
> I think I'd have gone for:
> 	size_t len = strlen(dest);
> 	if (len && dest[len - 1] != ',' && ++len < destlen)
> 		dest[len - 1] = ',';
> 	if (strscpy(dest + len, src, destlen - len) < 0)
> 		return -1;

This is valid, but I just feel like %s%s is more readable for what it
does. You've open-coded the first %s, and the ++len's interaction
between the logic and the argument to strscpy is subtle. Since this
isn't fast-path, let's use what Mariia has proposed.

> 
> Although it would be better as an 'add_option()' function.

If this were done in more places, I'd agree, but as-is it's pretty
limited.

Reviewed-by: Kees Cook <kees@kernel.org>


-- 
Kees Cook

Re: [PATCH] NFS: nfsroot: replace strlcat() with snprintf()

Posted by Mariia Nikitash 1 month, 1 week ago

Hi,



On Tue, Aug 18, 2026 at 10:42 AM Kees Cook <kees@kernel.org> wrote:
>
> On Tue, Aug 18, 2026 at 09:55:48AM +0100, David Laight wrote:
> > On Fri, 14 Aug 2026 00:28:45 -0700
> > Mariia Nikitash <mariianikitash@google.com> wrote:
> >
> > > In preparation for removing the deprecated strlcat() API[1], replace its
> > > uses in root_nfs_cat() with snprintf().
> > >
> > > Build the separator and source string in a single call using the
> > > remaining space in the destination buffer. snprintf() returns the length
> > > it would have written excluding the terminating NUL, so comparing the
> > > return value against the remaining buffer space preserves the existing
> > > truncation check.
> > >
> > > Link: https://github.com/KSPP/linux/issues/370 [1]
> > > Signed-off-by: Mariia Nikitash <mariianikitash@google.com>
> > > ---
> > >  fs/nfs/nfsroot.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/nfs/nfsroot.c b/fs/nfs/nfsroot.c
> > > index 432612d22437..e951fe731679 100644
> > > --- a/fs/nfs/nfsroot.c
> > > +++ b/fs/nfs/nfsroot.c
> > > @@ -173,12 +173,12 @@ static int __init root_nfs_cat(char *dest, const char *src,
> > >                            const size_t destlen)
> > >  {
> > >     size_t len = strlen(dest);
> > > +   size_t remaining = destlen - len;
> > > +   const char *sep = "";
> > >
> > >     if (len && dest[len - 1] != ',')
> > > -           if (strlcat(dest, ",", destlen) >= destlen)
> > > -                   return -1;
> > > -
> > > -   if (strlcat(dest, src, destlen) >= destlen)
> > > +           sep = ",";
> > > +   if (snprintf(dest + len, remaining, "%s%s", sep, src) >= remaining)
> > >             return -1;
> >
> > I think I'd have gone for:
> >       size_t len = strlen(dest);
> >       if (len && dest[len - 1] != ',' && ++len < destlen)
> >               dest[len - 1] = ',';
> >       if (strscpy(dest + len, src, destlen - len) < 0)
> >               return -1;
>
> This is valid, but I just feel like %s%s is more readable for what it
> does. You've open-coded the first %s, and the ++len's interaction
> between the logic and the argument to strscpy is subtle. Since this
> isn't fast-path, let's use what Mariia has proposed.

I had also considered similar approach to what David suggested, I am
open to any path forward based on what maintainers want.

>
> >
> > Although it would be better as an 'add_option()' function.
>
> If this were done in more places, I'd agree, but as-is it's pretty
> limited.
>
> Reviewed-by: Kees Cook <kees@kernel.org>

Thanks for the review!

>
>
> --
> Kees Cook


Mariia

Re: [PATCH] NFS: nfsroot: replace strlcat() with snprintf()

Posted by Justin Stitt 1 month, 1 week ago

Hi,

On Fri, Aug 14, 2026 at 12:28:45AM -0700, Mariia Nikitash wrote:
> In preparation for removing the deprecated strlcat() API[1], replace its
> uses in root_nfs_cat() with snprintf().
> 
> Build the separator and source string in a single call using the
> remaining space in the destination buffer. snprintf() returns the length
> it would have written excluding the terminating NUL, so comparing the
> return value against the remaining buffer space preserves the existing
> truncation check.
> 
> Link: https://github.com/KSPP/linux/issues/370 [1]
> Signed-off-by: Mariia Nikitash <mariianikitash@google.com>


Reviewed-by: Justin Stitt <justinstitt@google.com>

> ---
>  fs/nfs/nfsroot.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfs/nfsroot.c b/fs/nfs/nfsroot.c
> index 432612d22437..e951fe731679 100644
> --- a/fs/nfs/nfsroot.c
> +++ b/fs/nfs/nfsroot.c
> @@ -173,12 +173,12 @@ static int __init root_nfs_cat(char *dest, const char *src,
>  			       const size_t destlen)
>  {
>  	size_t len = strlen(dest);
> +	size_t remaining = destlen - len;
> +	const char *sep = "";
>  
>  	if (len && dest[len - 1] != ',')
> -		if (strlcat(dest, ",", destlen) >= destlen)
> -			return -1;
> -
> -	if (strlcat(dest, src, destlen) >= destlen)
> +		sep = ",";
> +	if (snprintf(dest + len, remaining, "%s%s", sep, src) >= remaining)
>  		return -1;
>  	return 0;
>  }
> -- 
> 2.55.0.679.g6767b8d81c-goog
> 
>

Thanks
Justin

Re: [PATCH] NFS: nfsroot: replace strlcat() with snprintf()

Posted by Bill Wendling 1 month, 1 week ago

On Mon, Aug 17, 2026 at 1:33 PM Justin Stitt <justinstitt@google.com> wrote:
>
> Hi,
>
> On Fri, Aug 14, 2026 at 12:28:45AM -0700, Mariia Nikitash wrote:
> > In preparation for removing the deprecated strlcat() API[1], replace its
> > uses in root_nfs_cat() with snprintf().
> >
> > Build the separator and source string in a single call using the
> > remaining space in the destination buffer. snprintf() returns the length
> > it would have written excluding the terminating NUL, so comparing the
> > return value against the remaining buffer space preserves the existing
> > truncation check.
> >
> > Link: https://github.com/KSPP/linux/issues/370 [1]
> > Signed-off-by: Mariia Nikitash <mariianikitash@google.com>
>
>
> Reviewed-by: Justin Stitt <justinstitt@google.com>
>
Acked-by: Bill Wendling <morbo@google.com>

> > ---
> >  fs/nfs/nfsroot.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/nfs/nfsroot.c b/fs/nfs/nfsroot.c
> > index 432612d22437..e951fe731679 100644
> > --- a/fs/nfs/nfsroot.c
> > +++ b/fs/nfs/nfsroot.c
> > @@ -173,12 +173,12 @@ static int __init root_nfs_cat(char *dest, const char *src,
> >                              const size_t destlen)
> >  {
> >       size_t len = strlen(dest);
> > +     size_t remaining = destlen - len;
> > +     const char *sep = "";
> >
> >       if (len && dest[len - 1] != ',')
> > -             if (strlcat(dest, ",", destlen) >= destlen)
> > -                     return -1;
> > -
> > -     if (strlcat(dest, src, destlen) >= destlen)
> > +             sep = ",";
> > +     if (snprintf(dest + len, remaining, "%s%s", sep, src) >= remaining)
> >               return -1;
> >       return 0;
> >  }
> > --
> > 2.55.0.679.g6767b8d81c-goog
> >
> >
>
> Thanks
> Justin
>