On Tue 2014-11-18 12:37:32, Steven Rostedt wrote: > On Tue, 18 Nov 2014 17:33:54 +0100 > Petr Mladek <pmla...@suse.cz> wrote: > > > On Mon 2014-11-17 14:12:15, Steven Rostedt wrote: > > > > > > > I don't like the fact that I did a code structure change with this > > > > patch. This patch should be just a simple conversion of len to > > > > seq_buf_used(). I'm going to strip this change out and put it before > > > > this patch. > > > > > > > > > As the seq_buf->len will soon be +1 size when there's an overflow, we > > > must use trace_seq_used() or seq_buf_used() methods to get the real > > > length. This will prevent buffer overflow issues if just the len > > > of the seq_buf descriptor is used to copy memory. > > > > > > Link: http://lkml.kernel.org/r/20141114121911.09ba3...@gandalf.local.home > > > > > > Reported-by: Petr Mladek <pmla...@suse.cz> > > > Signed-off-by: Steven Rostedt <rost...@goodmis.org> > > > --- > > > include/linux/trace_seq.h | 20 +++++++++++++++++++- > > > kernel/trace/seq_buf.c | 2 +- > > > kernel/trace/trace.c | 22 +++++++++++----------- > > > kernel/trace/trace_events.c | 9 ++++++--- > > > kernel/trace/trace_functions_graph.c | 5 ++++- > > > kernel/trace/trace_seq.c | 2 +- > > > 6 files changed, 42 insertions(+), 18 deletions(-) > > > > [...] > > > > > > > --- a/kernel/trace/trace.c > > > +++ b/kernel/trace/trace.c > > > @@ -944,10 +944,10 @@ static ssize_t trace_seq_to_buffer(struct trace_seq > > > *s, void *buf, size_t cnt) > > > { > > > int len; > > > > > > - if (s->seq.len <= s->seq.readpos) > > > + if (trace_seq_used(s) <= s->seq.readpos) > > > return -EBUSY; > > > > > > - len = s->seq.len - s->seq.readpos; > > > + len = trace_seq_used(s) - s->seq.readpos; > > > if (cnt > len) > > > cnt = len; > > > memcpy(buf, s->buffer + s->seq.readpos, cnt); > > > > > > There is one more dangerous usage in trace_printk_seq(). It is on > > three lines there. > > You totally confused me. What usage in trace_printk_seq(), and what > three lines? > > In this patch, trace_printk_seq() looks like this: > > int trace_print_seq(struct seq_file *m, struct trace_seq *s) > { > int ret; > > __trace_seq_init(s); > > ret = seq_buf_print_seq(m, &s->seq); > > /* > * Only reset this buffer if we successfully wrote to the > * seq_file buffer. This lets the caller try again or > * do something else with the contents. > */ > if (!ret) > trace_seq_init(s); > > return ret; > }
The confusion is caused by the 'k' ("print" vs. "printk") in the function name. I was talking about the following function from kernel/trace/trace.c: void trace_printk_seq(struct trace_seq *s) { /* Probably should print a warning here. */ if (s->seq.len >= TRACE_MAX_PRINT) s->seq.len = TRACE_MAX_PRINT; /* should be zero ended, but we are paranoid. */ s->buffer[s->seq.len] = 0; printk(KERN_TRACE "%s", s->buffer); trace_seq_init(s); } I found it when checking the applied patches in origin/rfc/seq-buf branch. I hope that it was the correct place. Best Regards, Petr -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/