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

Kees Cook
2023-07-17T15:44:01Z

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