[PATCH 2/3] tracing: make sure the parsed string always terminates with '\0'

[PATCH 2/3] tracing: make sure the parsed string always terminates with '\0'

[changbin.du]

From: Changbin Du <changbin.du@intel.com>

The parser parse every string into parser.buffer. And some of the callers
assume that parser.buffer contains a C string. So it is dangerous that the
parser returns a unterminated string. The userspace can leverage this to
attack the kernel.

Signed-off-by: Changbin Du <changbin.du@intel.com>
Cc: stable@vger.kernel.org
---
 kernel/trace/trace.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 18526a1..e1baca0 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -530,8 +530,6 @@ int trace_pid_write(struct trace_pid_list *filtered_pids,
 		ubuf += ret;
 		cnt -= ret;
 
-		parser.buffer[parser.idx] = 0;
-
 		ret = -EINVAL;
 		if (kstrtoul(parser.buffer, 0, &val))
 			break;
@@ -1253,7 +1251,7 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
 
 	/* read the non-space input */
 	while (cnt && !is_space_or_zero(ch)) {
-		if (parser->idx < parser->size - 1)
+		if (parser->idx < parser->size - 2)
 			parser->buffer[parser->idx++] = ch;
 		else {
 			ret = -EINVAL;
@@ -1270,9 +1268,11 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
 	if (is_space_or_zero(ch)) {
 		parser->buffer[parser->idx] = 0;
 		parser->cont = false;
-	} else if (parser->idx < parser->size - 1) {
+	} else if (parser->idx < parser->size - 2) {
 		parser->cont = true;
 		parser->buffer[parser->idx++] = ch;
+		/* Make sure the parsed string always terminates with '\0'. */
+		parser->buffer[parser->idx] = 0;
 	} else {
 		ret = -EINVAL;
 		goto out;
-- 
2.7.4

Re: [PATCH 2/3] tracing: make sure the parsed string always terminates with '\0'

[Steven Rostedt]

On Tue,  9 Jan 2018 17:55:47 +0800
changbin.du@intel.com wrote:

> From: Changbin Du <changbin.du@intel.com>
> 
> The parser parse every string into parser.buffer. And some of the callers
> assume that parser.buffer contains a C string. So it is dangerous that the
> parser returns a unterminated string. The userspace can leverage this to
> attack the kernel.

Is this only a bug if we apply your first patch?

-- Steve

Re: [PATCH 2/3] tracing: make sure the parsed string always terminates with '\0'

[Du, Changbin]

On Tue, Jan 09, 2018 at 06:02:58PM -0500, Steven Rostedt wrote:
> On Tue,  9 Jan 2018 17:55:47 +0800
> changbin.du@intel.com wrote:
> 
> > From: Changbin Du <changbin.du@intel.com>
> > 
> > The parser parse every string into parser.buffer. And some of the callers
> > assume that parser.buffer contains a C string. So it is dangerous that the
> > parser returns a unterminated string. The userspace can leverage this to
> > attack the kernel.
> 
> Is this only a bug if we apply your first patch?
>
I don't think so. Seems it is there already.
 
> -- Steve
> 

-- 
Thanks,
Changbin Du

Re: [PATCH 2/3] tracing: make sure the parsed string always terminates with '\0'

[Steven Rostedt]

On Wed, 10 Jan 2018 11:02:06 +0800
"Du, Changbin" <changbin.du@intel.com> wrote:

> On Tue, Jan 09, 2018 at 06:02:58PM -0500, Steven Rostedt wrote:
> > On Tue,  9 Jan 2018 17:55:47 +0800
> > changbin.du@intel.com wrote:
> >   
> > > From: Changbin Du <changbin.du@intel.com>
> > > 
> > > The parser parse every string into parser.buffer. And some of the callers
> > > assume that parser.buffer contains a C string. So it is dangerous that the
> > > parser returns a unterminated string. The userspace can leverage this to
> > > attack the kernel.  
> > 
> > Is this only a bug if we apply your first patch?
> >  
> I don't think so. Seems it is there already.
>  

OK. I'll have to take a deeper look into this so that I completely
understand the problem and your solution. I'm currently traveling and
may not get to do that this week. Please ping me next week if you don't
hear back from me on this issue.

Thanks!

-- Steve

Re: [PATCH 2/3] tracing: make sure the parsed string always terminates with '\0'

[Du, Changbin]

On Tue, Jan 09, 2018 at 11:10:22PM -0500, Steven Rostedt wrote:
> On Wed, 10 Jan 2018 11:02:06 +0800
> "Du, Changbin" <changbin.du@intel.com> wrote:
> 
> > On Tue, Jan 09, 2018 at 06:02:58PM -0500, Steven Rostedt wrote:
> > > On Tue,  9 Jan 2018 17:55:47 +0800
> > > changbin.du@intel.com wrote:
> > >   
> > > > From: Changbin Du <changbin.du@intel.com>
> > > > 
> > > > The parser parse every string into parser.buffer. And some of the callers
> > > > assume that parser.buffer contains a C string. So it is dangerous that the
> > > > parser returns a unterminated string. The userspace can leverage this to
> > > > attack the kernel.  
> > > 
> > > Is this only a bug if we apply your first patch?
> > >  
> > I don't think so. Seems it is there already.
> >  
> 
> OK. I'll have to take a deeper look into this so that I completely
> understand the problem and your solution. I'm currently traveling and
> may not get to do that this week. Please ping me next week if you don't
> hear back from me on this issue.
> 
> Thanks!
> 
> -- Steve

I checked every trace_get_user() clients again and found it is not an issue in
current kernel. The client has checked trace_parser_cont() before using parsed
string or append '\0'.

I still want to make the parser returns a '\0' terminated string. Then we don't
require the clients append it. I think this would be better since we are dealing
with strings.

-- 
Thanks,
Changbin Du