[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <202307170826.397635AD@keescook>
Date: Mon, 17 Jul 2023 08:43:55 -0700
From: Kees Cook <keescook@...omium.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/1] seq_file: Replace strncpy()+nul by strscpy()
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@...ux.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@...omium.org>
-Kees
--
Kees Cook
Powered by blists - more mailing lists