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

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]

Date:   Wed, 23 Jan 2019 21:09:56 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     Andreas Ziegler <andreas.ziegler@....de>,
        Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] tracing: uprobes: Re-enable $comm support for
 uprobe events

On Thu, 24 Jan 2019 10:43:22 +0900
Masami Hiramatsu <mhiramat@...nel.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