[PATCH v1 1/1] seq_file: Replace strncpy()+nul by strscpy()

[PATCH v1 1/1] seq_file: Replace strncpy()+nul by strscpy()

[Andy Shevchenko]

Privided seq_show_option_n() macro breaks build with -Werror
and W=1, e.g.:

In function ‘strncpy’,
    inlined from ‘ocfs2_show_options’ at fs/ocfs2/super.c:1520:3:
include/linux/fortify-string.h:68:33: error: ‘__builtin_strncpy’ output may be truncated copying 4 bytes from a string of length 4 [-Werror=stringop-truncation]
   68 | #define __underlying_strncpy    __builtin_strncpy
      |                                 ^
include/linux/fortify-string.h:151:16: note: in expansion of macro ‘__underlying_strncpy’
  151 |         return __underlying_strncpy(p, q, size);
      |                ^~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

While -Werror wasn't enabled by default at the time of the original code
landed into mainline, strscpy() was already there and preferred over strncpy().
Due to above mentioned issues, use the latter in seq_show_option_n().

Fixes: a068acf2ee77 ("fs: create and use seq_show_option for escaping")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/seq_file.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index bd023dd38ae6..e87d635ca24f 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -260,8 +260,7 @@ static inline void seq_show_option(struct seq_file *m, const char *name,
  */
 #define seq_show_option_n(m, name, value, length) {	\
 	char val_buf[length + 1];			\
-	strncpy(val_buf, value, length);		\
-	val_buf[length] = '\0';				\
+	strscpy(val_buf, value, sizeof(val_buf));	\
 	seq_show_option(m, name, val_buf);		\
 }
 
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 1/1] seq_file: Replace strncpy()+nul by strscpy()

[Kees Cook]

On Mon, Jul 17, 2023 at 12:33:32PM +0300, Andy Shevchenko wrote:
> Privided seq_show_option_n() macro breaks build with -Werror
> and W=1, e.g.:
> 
> In function ‘strncpy’,
>     inlined from ‘ocfs2_show_options’ at fs/ocfs2/super.c:1520:3:
> include/linux/fortify-string.h:68:33: error: ‘__builtin_strncpy’ output may be truncated copying 4 bytes from a string of length 4 [-Werror=stringop-truncation]
>    68 | #define __underlying_strncpy    __builtin_strncpy
>       |                                 ^

While I totally agree with the removal of strncpy(), I'm confused about
how this warning is being produced:

                seq_show_option_n(s, "cluster_stack", osb->osb_cluster_stack,
                                  OCFS2_STACK_LABEL_LEN);

fs/ocfs2/ocfs2.h:389:   char osb_cluster_stack[OCFS2_STACK_LABEL_LEN + 1];

fs/ocfs2/ocfs2_fs.h:#define OCFS2_STACK_LABEL_LEN               4

#define seq_show_option_n(m, name, value, length) {  \
     char val_buf[length + 1];                       \
     strncpy(val_buf, value, length);                \
...

the source buffer is OCFS2_STACK_LABEL_LEN + 1 long, and the dest buffer
is OCFS2_STACK_LABEL_LEN + 1 long. ??

I think this doesn't need to use seq_show_option_n() at all.

> include/linux/fortify-string.h:151:16: note: in expansion of macro ‘__underlying_strncpy’
>   151 |         return __underlying_strncpy(p, q, size);
>       |                ^~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> While -Werror wasn't enabled by default at the time of the original code
> landed into mainline, strscpy() was already there and preferred over strncpy().
> Due to above mentioned issues, use the latter in seq_show_option_n().
> 
> Fixes: a068acf2ee77 ("fs: create and use seq_show_option for escaping")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/seq_file.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
> index bd023dd38ae6..e87d635ca24f 100644
> --- a/include/linux/seq_file.h
> +++ b/include/linux/seq_file.h
> @@ -260,8 +260,7 @@ static inline void seq_show_option(struct seq_file *m, const char *name,
>   */
>  #define seq_show_option_n(m, name, value, length) {	\
>  	char val_buf[length + 1];			\
> -	strncpy(val_buf, value, length);		\
> -	val_buf[length] = '\0';				\
> +	strscpy(val_buf, value, sizeof(val_buf));	\
>  	seq_show_option(m, name, val_buf);		\
>  }

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

-Kees

-- 
Kees Cook

Re: [PATCH v1 1/1] seq_file: Replace strncpy()+nul by strscpy()

[Andy Shevchenko]

On Mon, Jul 17, 2023 at 08:43:55AM -0700, Kees Cook wrote:
> On Mon, Jul 17, 2023 at 12:33:32PM +0300, Andy Shevchenko wrote:

...

> I think this doesn't need to use seq_show_option_n() at all.

Quite likely. Nevertheless, it's one of the dozens (?) warnings like this.

...

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

Thank you for the review!

I think it's you who may take it as seq_file.h seems everybody's playground.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] seq_file: Replace strncpy()+nul by strscpy()

[Kees Cook]

On Mon, Jul 17, 2023 at 07:05:44PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 17, 2023 at 08:43:55AM -0700, Kees Cook wrote:
> > On Mon, Jul 17, 2023 at 12:33:32PM +0300, Andy Shevchenko wrote:
> 
> ...
> 
> > I think this doesn't need to use seq_show_option_n() at all.
> 
> Quite likely. Nevertheless, it's one of the dozens (?) warnings like this.
> 
> ...
> 
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> 
> Thank you for the review!
> 
> I think it's you who may take it as seq_file.h seems everybody's playground.

Ah, good point. :P

-- 
Kees Cook

Re: [PATCH v1 1/1] seq_file: Replace strncpy()+nul by strscpy()

[Kees Cook]

On Mon, 17 Jul 2023 12:33:32 +0300, Andy Shevchenko wrote:
> Privided seq_show_option_n() macro breaks build with -Werror
> and W=1, e.g.:
> 
> In function ‘strncpy’,
>     inlined from ‘ocfs2_show_options’ at fs/ocfs2/super.c:1520:3:
> include/linux/fortify-string.h:68:33: error: ‘__builtin_strncpy’ output may be truncated copying 4 bytes from a string of length 4 [-Werror=stringop-truncation]
>    68 | #define __underlying_strncpy    __builtin_strncpy
>       |                                 ^
> include/linux/fortify-string.h:151:16: note: in expansion of macro ‘__underlying_strncpy’
>   151 |         return __underlying_strncpy(p, q, size);
>       |                ^~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> [...]

Applied, thanks!

[1/1] seq_file: Replace strncpy()+nul by strscpy()
      https://git.kernel.org/kees/c/c30417b20f49

Best regards,
-- 
Kees Cook

RE: [PATCH v1 1/1] seq_file: Replace strncpy()+nul by strscpy()

[David Laight]

From: Andy Shevchenko
> Sent: 17 July 2023 10:34
...
>  #define seq_show_option_n(m, name, value, length) {	\
>  	char val_buf[length + 1];			\
> -	strncpy(val_buf, value, length);		\
> -	val_buf[length] = '\0';				\
> +	strscpy(val_buf, value, sizeof(val_buf));	\
>  	seq_show_option(m, name, val_buf);		\
>  }

Why the extra double-copy with (potentially) a VLA?

seq_show_option() calls seq_escape_str(),
seq_escape_str calls seq_escape_mem(...  strlen(src) ...).

Implement seq_show_option() as seq_show_option_n(... strlen(value))
and use seq_escape_mem() for the value.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v1 1/1] seq_file: Replace strncpy()+nul by strscpy()

[Kees Cook]

On Mon, Jul 17, 2023 at 04:09:23PM -0700, Kees Cook wrote:
> 
> On Mon, 17 Jul 2023 12:33:32 +0300, Andy Shevchenko wrote:
> > Privided seq_show_option_n() macro breaks build with -Werror
> > and W=1, e.g.:
> > 
> > In function ‘strncpy’,
> >     inlined from ‘ocfs2_show_options’ at fs/ocfs2/super.c:1520:3:
> > include/linux/fortify-string.h:68:33: error: ‘__builtin_strncpy’ output may be truncated copying 4 bytes from a string of length 4 [-Werror=stringop-truncation]
> >    68 | #define __underlying_strncpy    __builtin_strncpy
> >       |                                 ^
> > include/linux/fortify-string.h:151:16: note: in expansion of macro ‘__underlying_strncpy’
> >   151 |         return __underlying_strncpy(p, q, size);
> >       |                ^~~~~~~~~~~~~~~~~~~~
> > cc1: all warnings being treated as errors
> > 
> > [...]
> 
> Applied, thanks!
> 
> [1/1] seq_file: Replace strncpy()+nul by strscpy()
>       https://git.kernel.org/kees/c/c30417b20f49

Gah, I dropped this from my tree since it was actually wrong[1]. This is an
ugly corner case with strscpy vs strncpy: the cast be32 from hfs/hfsplus[2]
looks unterminated to strscpy, so it would return -E2BIG, but really
FORTIFY noticed the over-read (strscpy is correctly checking the 5th
byte for NUL).

So... I think we need to fix seq_show_option_n() using memcpy+NUL, drop
the ocfs2 usage, and clarify that the seq_show_option_n() docs mean
"n means _exactly_ n bytes"...

-Kees

[1] https://lore.kernel.org/lkml/0000000000000a88cb0600ccef54@google.com/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/hfsplus/options.c?h=v6.4#n221

-- 
Kees Cook

Re: [PATCH v1 1/1] seq_file: Replace strncpy()+nul by strscpy()

[Andy Shevchenko]

On Tue, Jul 18, 2023 at 10:00:24PM -0700, Kees Cook wrote:
> On Mon, Jul 17, 2023 at 04:09:23PM -0700, Kees Cook wrote:
> > On Mon, 17 Jul 2023 12:33:32 +0300, Andy Shevchenko wrote:
> > > Privided seq_show_option_n() macro breaks build with -Werror
> > > and W=1, e.g.:
> > > 
> > > In function ‘strncpy’,
> > >     inlined from ‘ocfs2_show_options’ at fs/ocfs2/super.c:1520:3:
> > > include/linux/fortify-string.h:68:33: error: ‘__builtin_strncpy’ output may be truncated copying 4 bytes from a string of length 4 [-Werror=stringop-truncation]
> > >    68 | #define __underlying_strncpy    __builtin_strncpy
> > >       |                                 ^
> > > include/linux/fortify-string.h:151:16: note: in expansion of macro ‘__underlying_strncpy’
> > >   151 |         return __underlying_strncpy(p, q, size);
> > >       |                ^~~~~~~~~~~~~~~~~~~~
> > > cc1: all warnings being treated as errors
> > > 
> > > [...]
> > 
> > Applied, thanks!
> > 
> > [1/1] seq_file: Replace strncpy()+nul by strscpy()
> >       https://git.kernel.org/kees/c/c30417b20f49
> 
> Gah, I dropped this from my tree since it was actually wrong[1]. This is an
> ugly corner case with strscpy vs strncpy: the cast be32 from hfs/hfsplus[2]
> looks unterminated to strscpy, so it would return -E2BIG, but really
> FORTIFY noticed the over-read (strscpy is correctly checking the 5th
> byte for NUL).
> 
> So... I think we need to fix seq_show_option_n() using memcpy+NUL, drop
> the ocfs2 usage, and clarify that the seq_show_option_n() docs mean
> "n means _exactly_ n bytes"...

Sounds like a plan!

Please go ahead with that. My intention is to make eventually build kernel with
`make W=1` when CONFIG_WERROR=y.

> [1] https://lore.kernel.org/lkml/0000000000000a88cb0600ccef54@google.com/
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/hfsplus/options.c?h=v6.4#n221

-- 
With Best Regards,
Andy Shevchenko