[PATCH] firmware: tegra: fix strncpy()/strncat() confusion

[PATCH] firmware: tegra: fix strncpy()/strncat() confusion

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The way that bpmp_populate_debugfs_inband() uses strncpy()
and strncat() makes no sense since the size argument for
the first is insufficient to contain the trailing '/'
and the second passes the length of the input rather than
the output, which triggers a warning:

In function 'strncat',
    inlined from 'bpmp_populate_debugfs_inband' at ../drivers/firmware/tegra/bpmp-debugfs.c:422:4:
include/linux/string.h:289:30: warning: '__builtin_strncat' specified bound depends on the length of the source argument [-Wstringop-overflow=]
  289 | #define __underlying_strncat __builtin_strncat
      |                              ^
include/linux/string.h:367:10: note: in expansion of macro '__underlying_strncat'
  367 |   return __underlying_strncat(p, q, count);
      |          ^~~~~~~~~~~~~~~~~~~~
drivers/firmware/tegra/bpmp-debugfs.c: In function 'bpmp_populate_debugfs_inband':
include/linux/string.h:288:29: note: length computed here
  288 | #define __underlying_strlen __builtin_strlen
      |                             ^
include/linux/string.h:321:10: note: in expansion of macro '__underlying_strlen'
  321 |   return __underlying_strlen(p);

Simplify this to use an snprintf() instead.

Fixes: 5e37b9c137ee ("firmware: tegra: Add support for in-band debug")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/firmware/tegra/bpmp-debugfs.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/firmware/tegra/bpmp-debugfs.c b/drivers/firmware/tegra/bpmp-debugfs.c
index c1bbba9ee93a..9ec20ddc9a6b 100644
--- a/drivers/firmware/tegra/bpmp-debugfs.c
+++ b/drivers/firmware/tegra/bpmp-debugfs.c
@@ -412,16 +412,12 @@ static int bpmp_populate_debugfs_inband(struct tegra_bpmp *bpmp,
 				goto out;
 			}
 
-			len = strlen(ppath) + strlen(name) + 1;
+			len = snprintf("%s%s/", pathlen, ppath, name);
 			if (len >= pathlen) {
 				err = -EINVAL;
 				goto out;
 			}
 
-			strncpy(pathbuf, ppath, pathlen);
-			strncat(pathbuf, name, strlen(name));
-			strcat(pathbuf, "/");
-
 			err = bpmp_populate_debugfs_inband(bpmp, dentry,
 							   pathbuf);
 			if (err < 0)
-- 
2.27.0

Re: [PATCH] firmware: tegra: fix strncpy()/strncat() confusion

[Arvind Sankar]

On Mon, Oct 26, 2020 at 05:10:19PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The way that bpmp_populate_debugfs_inband() uses strncpy()
> and strncat() makes no sense since the size argument for
> the first is insufficient to contain the trailing '/'
> and the second passes the length of the input rather than
> the output, which triggers a warning:
> 
> In function 'strncat',
>     inlined from 'bpmp_populate_debugfs_inband' at ../drivers/firmware/tegra/bpmp-debugfs.c:422:4:
> include/linux/string.h:289:30: warning: '__builtin_strncat' specified bound depends on the length of the source argument [-Wstringop-overflow=]
>   289 | #define __underlying_strncat __builtin_strncat
>       |                              ^
> include/linux/string.h:367:10: note: in expansion of macro '__underlying_strncat'
>   367 |   return __underlying_strncat(p, q, count);
>       |          ^~~~~~~~~~~~~~~~~~~~
> drivers/firmware/tegra/bpmp-debugfs.c: In function 'bpmp_populate_debugfs_inband':
> include/linux/string.h:288:29: note: length computed here
>   288 | #define __underlying_strlen __builtin_strlen
>       |                             ^
> include/linux/string.h:321:10: note: in expansion of macro '__underlying_strlen'
>   321 |   return __underlying_strlen(p);
> 
> Simplify this to use an snprintf() instead.
> 
> Fixes: 5e37b9c137ee ("firmware: tegra: Add support for in-band debug")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/firmware/tegra/bpmp-debugfs.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/firmware/tegra/bpmp-debugfs.c b/drivers/firmware/tegra/bpmp-debugfs.c
> index c1bbba9ee93a..9ec20ddc9a6b 100644
> --- a/drivers/firmware/tegra/bpmp-debugfs.c
> +++ b/drivers/firmware/tegra/bpmp-debugfs.c
> @@ -412,16 +412,12 @@ static int bpmp_populate_debugfs_inband(struct tegra_bpmp *bpmp,
>  				goto out;
>  			}
>  
> -			len = strlen(ppath) + strlen(name) + 1;
> +			len = snprintf("%s%s/", pathlen, ppath, name);

Didn't you get any warnings with this? It should be
			len = snprintf(pathbuf, pathlen, "%s%s/", ppath, name);
right?

>  			if (len >= pathlen) {
>  				err = -EINVAL;
>  				goto out;
>  			}
>  
> -			strncpy(pathbuf, ppath, pathlen);
> -			strncat(pathbuf, name, strlen(name));
> -			strcat(pathbuf, "/");
> -
>  			err = bpmp_populate_debugfs_inband(bpmp, dentry,
>  							   pathbuf);
>  			if (err < 0)
> -- 
> 2.27.0
> 

Re: [PATCH] firmware: tegra: fix strncpy()/strncat() confusion

[Arnd Bergmann]

On Mon, Oct 26, 2020 at 5:35 PM Arvind Sankar <nivedita@alum.mit.edu> wrote:

> > diff --git a/drivers/firmware/tegra/bpmp-debugfs.c b/drivers/firmware/tegra/bpmp-debugfs.c
> > index c1bbba9ee93a..9ec20ddc9a6b 100644
> > --- a/drivers/firmware/tegra/bpmp-debugfs.c
> > +++ b/drivers/firmware/tegra/bpmp-debugfs.c
> > @@ -412,16 +412,12 @@ static int bpmp_populate_debugfs_inband(struct tegra_bpmp *bpmp,
> >                               goto out;
> >                       }
> >
> > -                     len = strlen(ppath) + strlen(name) + 1;
> > +                     len = snprintf("%s%s/", pathlen, ppath, name);
>
> Didn't you get any warnings with this? It should be
>                         len = snprintf(pathbuf, pathlen, "%s%s/", ppath, name);
> right?
>

Eek, I did get a warning about a different issue in that one-line change and
fixed it up in the wrong way without testing again. Sorry about that.

I'll retest and resend.

      Arnd

Re: [PATCH] firmware: tegra: fix strncpy()/strncat() confusion

[Arnd Bergmann]

On Mon, Oct 26, 2020 at 5:40 PM Arnd Bergmann <arnd@kernel.org> wrote:
> On Mon, Oct 26, 2020 at 5:35 PM Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > diff --git a/drivers/firmware/tegra/bpmp-debugfs.c b/drivers/firmware/tegra/bpmp-debugfs.c
> > > index c1bbba9ee93a..9ec20ddc9a6b 100644
> > > --- a/drivers/firmware/tegra/bpmp-debugfs.c
> > > +++ b/drivers/firmware/tegra/bpmp-debugfs.c
> > > @@ -412,16 +412,12 @@ static int bpmp_populate_debugfs_inband(struct tegra_bpmp *bpmp,
> > >                               goto out;
> > >                       }
> > >
> > > -                     len = strlen(ppath) + strlen(name) + 1;
> > > +                     len = snprintf("%s%s/", pathlen, ppath, name);
> >
> > Didn't you get any warnings with this? It should be
> >                         len = snprintf(pathbuf, pathlen, "%s%s/", ppath, name);
> > right?
> >
>
> Eek, I did get a warning about a different issue in that one-line change and
> fixed it up in the wrong way without testing again. Sorry about that.

Actually it turns out that gcc did not warn about my broken version.
The argument types are (roughly) correct and as the third argument
is not a constant string it could not verify the format string. Maybe it
should have complained about the constant string used as an output,
but it doesn't seem to care about that.

       Arnd