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

[Kees Cook <keescook@chromium.org>]

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