| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2a534333-b5f6-4b1d-b4b8-a1a71f91c3ff@wanadoo.fr>
Date: Wed, 1 Nov 2023 08:34:30 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Kees Cook <keescook@...omium.org>,
Steven Rostedt <rostedt@...dmis.org>
Cc: "Matthew Wilcox (Oracle)" <willy@...radead.org>,
Christoph Hellwig <hch@....de>,
Justin Stitt <justinstitt@...gle.com>,
Kent Overstreet <kent.overstreet@...ux.dev>,
Petr Mladek <pmladek@...e.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Rasmus Villemoes <linux@...musvillemoes.dk>,
Sergey Senozhatsky <senozhatsky@...omium.org>,
Masami Hiramatsu <mhiramat@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Arnd Bergmann <arnd@...db.de>,
Jonathan Corbet <corbet@....net>,
Yun Zhou <yun.zhou@...driver.com>,
Jacob Keller <jacob.e.keller@...el.com>,
Zhen Lei <thunder.leizhen@...wei.com>,
linux-trace-kernel@...r.kernel.org,
Yosry Ahmed <yosryahmed@...gle.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] seq_buf: Introduce DECLARE_SEQ_BUF and seq_buf_str()
Le 27/10/2023 à 17:56, Kees Cook a écrit :
> Solve two ergonomic issues with struct seq_buf;
>
> 1) Too much boilerplate is required to initialize:
>
> struct seq_buf s;
> char buf[32];
>
> seq_buf_init(s, buf, sizeof(buf));
>
> Instead, we can build this directly on the stack. Provide
> DECLARE_SEQ_BUF() macro to do this:
>
> DECLARE_SEQ_BUF(s, 32);
>
> 2) %NUL termination is fragile and requires 2 steps to get a valid
> C String (and is a layering violation exposing the "internals" of
> seq_buf):
>
> seq_buf_terminate(s);
> do_something(s->buffer);
>
> Instead, we can just return s->buffer directly after terminating it in
> the refactored seq_buf_terminate(), now known as seq_buf_str():
>
> do_something(seq_buf_str(s));
>
> Cc: Steven Rostedt <rostedt@...dmis.org>
> Cc: "Matthew Wilcox (Oracle)" <willy@...radead.org>
> Cc: Christoph Hellwig <hch@....de>
> Cc: Justin Stitt <justinstitt@...gle.com>
> Cc: Kent Overstreet <kent.overstreet@...ux.dev>
> Cc: Petr Mladek <pmladek@...e.com>
> Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> Cc: Rasmus Villemoes <linux@...musvillemoes.dk>
> Cc: Sergey Senozhatsky <senozhatsky@...omium.org>
> Cc: Masami Hiramatsu <mhiramat@...nel.org>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: Arnd Bergmann <arnd@...db.de>
> Cc: Jonathan Corbet <corbet@....net>
> Cc: Yun Zhou <yun.zhou@...driver.com>
> Cc: Jacob Keller <jacob.e.keller@...el.com>
> Cc: Zhen Lei <thunder.leizhen@...wei.com>
> Cc: linux-trace-kernel@...r.kernel.org
> Link: https://lore.kernel.org/r/20231026194033.it.702-kees@kernel.org
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---
> v3
> - fix commit log typos
> - improve code style for DECLARE_SEQ_BUF (shevchenko)
> - const-ify seq_bug_str() return (rostedt)
> v2 - https://lore.kernel.org/lkml/20231026194033.it.702-kees@kernel.org
> v1 - https://lore.kernel.org/lkml/20231026170722.work.638-kees@kernel.org
> ---
> include/linux/seq_buf.h | 21 +++++++++++++++++----
> kernel/trace/trace.c | 11 +----------
> lib/seq_buf.c | 4 +---
> 3 files changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
> index 8483e4b2d0d2..5fb1f12c33f9 100644
> --- a/include/linux/seq_buf.h
> +++ b/include/linux/seq_buf.h
> @@ -21,9 +21,18 @@ struct seq_buf {
> size_t len;
> };
>
> +#define DECLARE_SEQ_BUF(NAME, SIZE) \
> + char __ ## NAME ## _buffer[SIZE] = ""; \
> + struct seq_buf NAME = { \
> + .buffer = &__ ## NAME ## _buffer, \
~~~
Is the & needed here?
CJ
> + .size = SIZE, \
> + }
> +
> static inline void seq_buf_clear(struct seq_buf *s)
> {
> s->len = 0;
> + if (s->size)
> + s->buffer[0] = '\0';
> }
>
> static inline void
> @@ -69,8 +78,8 @@ static inline unsigned int seq_buf_used(struct seq_buf *s)
> }
>
> /**
> - * seq_buf_terminate - Make sure buffer is nul terminated
> - * @s: the seq_buf descriptor to terminate.
> + * seq_buf_str - get %NUL-terminated C string from seq_buf
> + * @s: the seq_buf handle
> *
> * This makes sure that the buffer in @s is nul terminated and
> * safe to read as a string.
> @@ -81,16 +90,20 @@ static inline unsigned int seq_buf_used(struct seq_buf *s)
> *
> * After this function is called, s->buffer is safe to use
> * in string operations.
> + *
> + * Returns @s->buf after making sure it is terminated.
> */
> -static inline void seq_buf_terminate(struct seq_buf *s)
> +static inline const char *seq_buf_str(struct seq_buf *s)
> {
> if (WARN_ON(s->size == 0))
> - return;
> + return "";
>
> if (seq_buf_buffer_left(s))
> s->buffer[s->len] = 0;
> else
> s->buffer[s->size - 1] = 0;
> +
> + return s->buffer;
> }
>
> /**
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index d629065c2383..2539cfc20a97 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -3828,15 +3828,6 @@ static bool trace_safe_str(struct trace_iterator *iter, const char *str,
> return false;
> }
>
> -static const char *show_buffer(struct trace_seq *s)
> -{
> - struct seq_buf *seq = &s->seq;
> -
> - seq_buf_terminate(seq);
> -
> - return seq->buffer;
> -}
> -
> static DEFINE_STATIC_KEY_FALSE(trace_no_verify);
>
> static int test_can_verify_check(const char *fmt, ...)
> @@ -3976,7 +3967,7 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt,
> */
> if (WARN_ONCE(!trace_safe_str(iter, str, star, len),
> "fmt: '%s' current_buffer: '%s'",
> - fmt, show_buffer(&iter->seq))) {
> + fmt, seq_buf_str(&iter->seq.seq))) {
> int ret;
>
> /* Try to safely read the string */
> diff --git a/lib/seq_buf.c b/lib/seq_buf.c
> index b7477aefff53..23518f77ea9c 100644
> --- a/lib/seq_buf.c
> +++ b/lib/seq_buf.c
> @@ -109,9 +109,7 @@ void seq_buf_do_printk(struct seq_buf *s, const char *lvl)
> if (s->size == 0 || s->len == 0)
> return;
>
> - seq_buf_terminate(s);
> -
> - start = s->buffer;
> + start = seq_buf_str(s);
> while ((lf = strchr(start, '\n'))) {
> int len = lf - start + 1;
>
Powered by blists - more mailing lists