Re: [PATCH v2 1/3] tracing: uprobes: Re-enable $comm support for uprobe events

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 24 Jan 2019 10:43:22 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > >  kernel/trace/trace_uprobe.c |   15 +++++++++++++--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> > > index 3a1d5ab6b4ba..b07e498ccbc6 100644
> > > --- a/kernel/trace/trace_uprobe.c
> > > +++ b/kernel/trace/trace_uprobe.c
> > > @@ -156,7 +156,10 @@ fetch_store_string(unsigned long addr, void *dest, void *base)
> > >  	if (unlikely(!maxlen))
> > >  		return -ENOMEM;
> > >  
> > > -	ret = strncpy_from_user(dst, src, maxlen);
> > > +	if (addr == (unsigned long)current->comm)
> > > +		ret = strlcpy(dst, current->comm, maxlen);  
> > 
> > As user space (although only root) defines the size of the event being
> > stored, and we could trick addr to be current->comm (although
> > difficult), we could possibly leak data if maxlen is > TASK_COMM_LEN. I
> > would feel better if we tested maxlen against TASK_COMM_LEN in this
> > case.
> > 
> > 	if (maxlen > TASK_COMM_LEN)
> > 		maxlen = TASK_COMM_LEN;
> > 
> > Or if we don't think it can happen, add a WARN_ON(maxlen >
> > TASK_COMM_LEN).  
> 
> Hmm, I thought current->comm is null terminated, isn't it?

Yes it is. I was thinking it was a memcpy (I blame conference fatigue ;-)

> Anyway, if user can specify current->comm, he must be able to specify 
> current->comm + TASK_COMM_LEN too by kprobe_events.
> Moreover, it can leak any data in kernel...

But this is for uprobes, which I why I was concerned.

> 
> And also, maxlen is calculated by fetch_store_strlen, right before
> this has been called.
> 
> I rather concern the case that if we have shorter size of maxlen than
> current->comm. Would we better show "(fault)" or tail-cut name ?
> (of course this is very difficult to happen, since the length is
> already checked.)

Actually, it would still be OK, as strlcpy does guarantee to be nul
terminated as long as it's greater than zero.

Hmm, strlcpy doesn't pad the rest if what is written is shorter than
what is allocated. Could that leak data?

-- Steve