Re: [PATCH 2/2] tracing: Use trace_seq_used() and seq_buf_used() instead of len

[Petr Mladek]

On Tue 2014-11-18 12:37:32, Steven Rostedt wrote:
> On Tue, 18 Nov 2014 17:33:54 +0100
> Petr Mladek <pmladek@xxxxxxx> 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.09ba3d38@xxxxxxxxxxxxxxxxxx
> > > 
> > > Reported-by: Petr Mladek <pmladek@xxxxxxx>
> > > Signed-off-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
> > > ---
> > >  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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/