linux-kernel - Re: [PATCH] tracing: a few fields of struct trace

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJL_ektQ=q2T6CUR0bKig088DWaOSUd-yek2S_dT3fjRRfrf0Q@mail.gmail.com>
Date:	Fri, 2 Aug 2013 11:15:36 -0700
From:	David Sharp <dhsharp@...gle.com>
To:	Andrew Vagin <avagin@...nvz.org>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...hat.com>,
	Hiraku Toyooka <hiraku.toyooka.gu@...achi.com>,
	Arjan van de Ven <arjan@...ux.intel.com>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
Subject: Re: [PATCH] tracing: a few fields of struct trace_iterator are zeroed
 by mistake

On Fri, Aug 2, 2013 at 10:16 AM, Andrew Vagin <avagin@...nvz.org> wrote:
> tracing_read_pipe zeros all fields bellow "seq". The declaration contains
> a comment about that, but it doesn't help.
>
> The first field is "snapshot", it's true when current open file is
> snapshot. Looks obvious, that it should not be zeroed.
>
> The second field is "started". It was converted from cpumask_t to
> cpumask_var_t (v2.6.28-4983-g4462344) or by another words it was
> converted from cpumask to pointer on cpumask.
>
> Currently the reference on "started" memory is lost after the first read
> from tracing_read_pipe and a proper object will never be freed.
>
> The "started" is never dereferenced for trace_pipe, because trace_pipe
> can't have the TRACE_FILE_ANNOTATE options (why?).
>
> Cc: Steven Rostedt <rostedt@...dmis.org>
> Cc: Frederic Weisbecker <fweisbec@...il.com>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: David Sharp <dhsharp@...gle.com>
> Cc: Hiraku Toyooka <hiraku.toyooka.gu@...achi.com>
> Cc: Arjan van de Ven <arjan@...ux.intel.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
>
> Signed-off-by: Andrew Vagin <avagin@...nvz.org>
> ---
>  include/linux/ftrace_event.h |   10 ++++++----
>  kernel/trace/trace.c         |    1 +
>  2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> index 4372658..44cdc11 100644
> --- a/include/linux/ftrace_event.h
> +++ b/include/linux/ftrace_event.h
> @@ -78,6 +78,11 @@ struct trace_iterator {
>         /* trace_seq for __print_flags() and __print_symbolic() etc. */
>         struct trace_seq        tmp_seq;
>
> +       cpumask_var_t           started;
> +
> +       /* it's true when current open file is snapshot */
> +       bool                    snapshot;
> +
>         /* The below is zeroed out in pipe_read */
>         struct trace_seq        seq;
>         struct trace_entry      *ent;
> @@ -90,10 +95,7 @@ struct trace_iterator {
>         loff_t                  pos;
>         long                    idx;
>
> -       cpumask_var_t           started;
> -
> -       /* it's true when current open file is snapshot */
> -       bool                    snapshot;
> +       /* All new field here will be zeroed out in pipe_read */

Wow. That is a terrible hack. Thanks for noticing it.

If I may suggest a way better idea: put these zeroed-in-pipe_read
members in an embedded anonymous structure, and zero the structure in
pipe_read.

>  };
>
>  enum trace_iter_flags {
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 0cd500b..897f553 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -4166,6 +4166,7 @@ waitagain:
>         memset(&iter->seq, 0,
>                sizeof(struct trace_iterator) -
>                offsetof(struct trace_iterator, seq));
> +       cpumask_clear(iter->started);
>         iter->pos = -1;
>
>         trace_event_read_lock();
> --
> 1.7.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/