[PATCH 0/3] tracing: Fix the parser when processing strings w/ or w/o terminated '\0'

[PATCH 0/3] tracing: Fix the parser when processing strings w/ or w/o terminated '\0'

[changbin.du]

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

I found there are some problems in the tracing parser when I investiage the root
cause of issues mentioned in below patch.
https://patchwork.kernel.org/patch/10132953/

This serials can fix them.

Changbin Du (3):
  tracing: detect the string termination character when parsing user
    input string
  tracing: make sure the parsed string always terminates with '\0'
  tracing: don't set parser->cont if it has reached the end of input
    buffer

 kernel/trace/trace.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

-- 
2.7.4

[PATCH 1/3] tracing: detect the string termination character when parsing user input string

[changbin.du]

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

The usersapce can give a '\0' terminated C string or even has '\0' at the
middle of input buffer. We need handle both these two cases correctly.

Before this change, trace_get_user() will return a parsed string "\0" in
below case. It is not expected (expects it skip all inputs) and cause the
caller failed.

open("/sys/kernel/debug/tracing//set_ftrace_pid", O_WRONLY|O_TRUNC) = 3
write(3, " \0", 2)                      = -1 EINVAL (Invalid argument)

This patch try to make the parser '\0' aware to fix such issue.

Since the caller expects trace_get_user() to parse whole input buffer, so
this patch treat '\0' as a separator as whitespace.

Signed-off-by: Changbin Du <changbin.du@intel.com>
---
 kernel/trace/trace.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 2a8d8a2..18526a1 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1194,9 +1194,14 @@ void trace_parser_put(struct trace_parser *parser)
 	parser->buffer = NULL;
 }
 
+static inline bool is_space_or_zero(char ch)
+{
+	return isspace(ch) || !ch;
+}
+
 /*
- * trace_get_user - reads the user input string separated by  space
- * (matched by isspace(ch))
+ * trace_get_user - reads the user input string separated by space or '\0'
+ * (matched by is_space_or_zero(ch))
  *
  * For each string found the 'struct trace_parser' is updated,
  * and the function returns.
@@ -1228,7 +1233,7 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
 	 */
 	if (!parser->cont) {
 		/* skip white space */
-		while (cnt && isspace(ch)) {
+		while (cnt && is_space_or_zero(ch)) {
 			ret = get_user(ch, ubuf++);
 			if (ret)
 				goto out;
@@ -1237,7 +1242,7 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
 		}
 
 		/* only spaces were written */
-		if (isspace(ch)) {
+		if (is_space_or_zero(ch)) {
 			*ppos += read;
 			ret = read;
 			goto out;
@@ -1247,7 +1252,7 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
 	}
 
 	/* read the non-space input */
-	while (cnt && !isspace(ch)) {
+	while (cnt && !is_space_or_zero(ch)) {
 		if (parser->idx < parser->size - 1)
 			parser->buffer[parser->idx++] = ch;
 		else {
@@ -1262,7 +1267,7 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
 	}
 
 	/* We either got finished input or we have to wait for another call. */
-	if (isspace(ch)) {
+	if (is_space_or_zero(ch)) {
 		parser->buffer[parser->idx] = 0;
 		parser->cont = false;
 	} else if (parser->idx < parser->size - 1) {
-- 
2.7.4

[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

[PATCH 3/3] tracing: don't set parser->cont if it has reached the end of input buffer

[changbin.du]

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

We should not set parser->cont if it has reached the end of input buffer.
And since some callers (like ftrace_graph_write()) treat it as an error
condition if trace_parser_cont() returns true.

For example, if userspace set 'set_ftrace_filter' by writing:
write(3, "abcdefg", 7)

Then in the kernel function ftrace_regex_write(), ftrace_process_regex()
will not be executed. The result is that the given filter will not be
applied at all.

ftrace_regex_write() {
	...
	read = trace_get_user(parser, ubuf, cnt, ppos);
	if (read >= 0 && trace_parser_loaded(parser) &&
	    !trace_parser_cont(parser)) {
		ret = ftrace_process_regex(iter, parser->buffer,
					   parser->idx, enable);
		...
	}
	...
}

Signed-off-by: Changbin Du <changbin.du@intel.com>
---
 kernel/trace/trace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index e1baca0..79fdce2 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1269,7 +1269,8 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
 		parser->buffer[parser->idx] = 0;
 		parser->cont = false;
 	} else if (parser->idx < parser->size - 2) {
-		parser->cont = true;
+		parser->cont = !!cnt;
+
 		parser->buffer[parser->idx++] = ch;
 		/* Make sure the parsed string always terminates with '\0'. */
 		parser->buffer[parser->idx] = 0;
-- 
2.7.4

Re: [PATCH 1/3] tracing: detect the string termination character when parsing user input string

[Steven Rostedt]

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

> From: Changbin Du <changbin.du@intel.com>
> 
> The usersapce can give a '\0' terminated C string or even has '\0' at the
> middle of input buffer. We need handle both these two cases correctly.

What do you define as correctly. Because I'm not seeing it.

> 
> Before this change, trace_get_user() will return a parsed string "\0" in
> below case. It is not expected (expects it skip all inputs) and cause the
> caller failed.
> 
> open("/sys/kernel/debug/tracing//set_ftrace_pid", O_WRONLY|O_TRUNC) = 3
> write(3, " \0", 2)                      = -1 EINVAL (Invalid argument)

That looks more like a feature and not a bug.

> 
> This patch try to make the parser '\0' aware to fix such issue.

Why?

> 
> Since the caller expects trace_get_user() to parse whole input buffer, so
> this patch treat '\0' as a separator as whitespace.

It looks more like we are trying to fix a userspace bug via the kernel.

I'm not liking this. So NACK.

-- Steve

> 
> Signed-off-by: Changbin Du <changbin.du@intel.com>
> ---
>  kernel/trace/trace.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 2a8d8a2..18526a1 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -1194,9 +1194,14 @@ void trace_parser_put(struct trace_parser *parser)
>  	parser->buffer = NULL;
>  }
>  
> +static inline bool is_space_or_zero(char ch)
> +{
> +	return isspace(ch) || !ch;
> +}
> +
>  /*
> - * trace_get_user - reads the user input string separated by  space
> - * (matched by isspace(ch))
> + * trace_get_user - reads the user input string separated by space or '\0'
> + * (matched by is_space_or_zero(ch))
>   *
>   * For each string found the 'struct trace_parser' is updated,
>   * and the function returns.
> @@ -1228,7 +1233,7 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
>  	 */
>  	if (!parser->cont) {
>  		/* skip white space */
> -		while (cnt && isspace(ch)) {
> +		while (cnt && is_space_or_zero(ch)) {
>  			ret = get_user(ch, ubuf++);
>  			if (ret)
>  				goto out;
> @@ -1237,7 +1242,7 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
>  		}
>  
>  		/* only spaces were written */
> -		if (isspace(ch)) {
> +		if (is_space_or_zero(ch)) {
>  			*ppos += read;
>  			ret = read;
>  			goto out;
> @@ -1247,7 +1252,7 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
>  	}
>  
>  	/* read the non-space input */
> -	while (cnt && !isspace(ch)) {
> +	while (cnt && !is_space_or_zero(ch)) {
>  		if (parser->idx < parser->size - 1)
>  			parser->buffer[parser->idx++] = ch;
>  		else {
> @@ -1262,7 +1267,7 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
>  	}
>  
>  	/* We either got finished input or we have to wait for another call. */
> -	if (isspace(ch)) {
> +	if (is_space_or_zero(ch)) {
>  		parser->buffer[parser->idx] = 0;
>  		parser->cont = false;
>  	} else if (parser->idx < parser->size - 1) {

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 3/3] tracing: don't set parser->cont if it has reached the end of input buffer

[Steven Rostedt]

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

> From: Changbin Du <changbin.du@intel.com>
> 
> We should not set parser->cont if it has reached the end of input buffer.
> And since some callers (like ftrace_graph_write()) treat it as an error
> condition if trace_parser_cont() returns true.

This will break existing use cases. In fact you are removing the entire
point of this code. It NEEDS to continue if it reached the end of the
input buffer.

I do things like:

 # cat file > set_ftrace_filter

where the file has a list of function names. It writes in blocks, and
it could very well have a function name split between two writes where
the write is at the end of the buffer but not finished writing the
function name.

> 
> For example, if userspace set 'set_ftrace_filter' by writing:
> write(3, "abcdefg", 7)

>From my point of view, the above isn't done writing the function name
yet and we SHOULD continue waiting for more input.

BIG NACK on this patch. Sorry.

I'm guessing you have some program that writes only the strlen() of
these strings. That's wrong, you need to write "strlen()+1". Write some
real white space between calls, it will work. Add a "write(fd, " ", 1)"
between calls if you need to. Please don't change the kernel to fix
some bad use case. Especially when your fix will break existing use
cases.

-- Steve

> 
> Then in the kernel function ftrace_regex_write(), ftrace_process_regex()
> will not be executed. The result is that the given filter will not be
> applied at all.
> 
> ftrace_regex_write() {
> 	...
> 	read = trace_get_user(parser, ubuf, cnt, ppos);
> 	if (read >= 0 && trace_parser_loaded(parser) &&
> 	    !trace_parser_cont(parser)) {
> 		ret = ftrace_process_regex(iter, parser->buffer,
> 					   parser->idx, enable);
> 		...
> 	}
> 	...
> }
> 
> Signed-off-by: Changbin Du <changbin.du@intel.com>

Re: [PATCH 1/3] tracing: detect the string termination character when parsing user input string

[Du, Changbin]

hi Rostedt,

On Tue, Jan 09, 2018 at 05:54:34PM -0500, Steven Rostedt wrote:
> On Tue,  9 Jan 2018 17:55:46 +0800
> changbin.du@intel.com wrote:
> 
> > From: Changbin Du <changbin.du@intel.com>
> > 
> > The usersapce can give a '\0' terminated C string or even has '\0' at the
> > middle of input buffer. We need handle both these two cases correctly.
> 
> What do you define as correctly. Because I'm not seeing it.
>
Soory I don't fully understand your question. What I meant is want to get clear that
how will tracing parser below strings.
  "", "  ",  "\0", " \0 ", "aa\0bb"
The parser may only recognize certain formats, but whatever the behaviour should
be clear and coherent for all tracing interfaces.

> > 
> > Before this change, trace_get_user() will return a parsed string "\0" in
> > below case. It is not expected (expects it skip all inputs) and cause the
> > caller failed.
> > 
> > open("/sys/kernel/debug/tracing//set_ftrace_pid", O_WRONLY|O_TRUNC) = 3
> > write(3, " \0", 2)                      = -1 EINVAL (Invalid argument)
> 
> That looks more like a feature and not a bug.
> 
I point this out because I think the parser should take this as an emptry string
per the comments of trace_get_user().
/*
 * trace_get_user - reads the user input string separated by  space
 * (matched by isspace(ch))
 *
 * For each string found the 'struct trace_parser' is updated,
 * and the function returns.
 *
 * Returns number of bytes read.
 *
 * See kernel/trace/trace.h for 'struct trace_parser' details.
 */

> > 
> > This patch try to make the parser '\0' aware to fix such issue.
> 
> Why?
> 

> > 
> > Since the caller expects trace_get_user() to parse whole input buffer, so
> > this patch treat '\0' as a separator as whitespace.
> 
> It looks more like we are trying to fix a userspace bug via the kernel.
> 

> I'm not liking this. So NACK.
> 
> -- Steve
> 
> > 
> > Signed-off-by: Changbin Du <changbin.du@intel.com>
> > ---
> >  kernel/trace/trace.c | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 2a8d8a2..18526a1 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -1194,9 +1194,14 @@ void trace_parser_put(struct trace_parser *parser)
> >  	parser->buffer = NULL;
> >  }
> >  
> > +static inline bool is_space_or_zero(char ch)
> > +{
> > +	return isspace(ch) || !ch;
> > +}
> > +
> >  /*
> > - * trace_get_user - reads the user input string separated by  space
> > - * (matched by isspace(ch))
> > + * trace_get_user - reads the user input string separated by space or '\0'
> > + * (matched by is_space_or_zero(ch))
> >   *
> >   * For each string found the 'struct trace_parser' is updated,
> >   * and the function returns.
> > @@ -1228,7 +1233,7 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
> >  	 */
> >  	if (!parser->cont) {
> >  		/* skip white space */
> > -		while (cnt && isspace(ch)) {
> > +		while (cnt && is_space_or_zero(ch)) {
> >  			ret = get_user(ch, ubuf++);
> >  			if (ret)
> >  				goto out;
> > @@ -1237,7 +1242,7 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
> >  		}
> >  
> >  		/* only spaces were written */
> > -		if (isspace(ch)) {
> > +		if (is_space_or_zero(ch)) {
> >  			*ppos += read;
> >  			ret = read;
> >  			goto out;
> > @@ -1247,7 +1252,7 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
> >  	}
> >  
> >  	/* read the non-space input */
> > -	while (cnt && !isspace(ch)) {
> > +	while (cnt && !is_space_or_zero(ch)) {
> >  		if (parser->idx < parser->size - 1)
> >  			parser->buffer[parser->idx++] = ch;
> >  		else {
> > @@ -1262,7 +1267,7 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
> >  	}
> >  
> >  	/* We either got finished input or we have to wait for another call. */
> > -	if (isspace(ch)) {
> > +	if (is_space_or_zero(ch)) {
> >  		parser->buffer[parser->idx] = 0;
> >  		parser->cont = false;
> >  	} else if (parser->idx < parser->size - 1) {
> 

-- 
Thanks,
Changbin Du

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 3/3] tracing: don't set parser->cont if it has reached the end of input buffer

[Du, Changbin]

On Tue, Jan 09, 2018 at 06:12:41PM -0500, Steven Rostedt wrote:
> On Tue,  9 Jan 2018 17:55:48 +0800
> changbin.du@intel.com wrote:
> 
> > From: Changbin Du <changbin.du@intel.com>
> > 
> > We should not set parser->cont if it has reached the end of input buffer.
> > And since some callers (like ftrace_graph_write()) treat it as an error
> > condition if trace_parser_cont() returns true.
> 
> This will break existing use cases. In fact you are removing the entire
> point of this code. It NEEDS to continue if it reached the end of the
> input buffer.
> 
> I do things like:
> 
>  # cat file > set_ftrace_filter
> 
> where the file has a list of function names. It writes in blocks, and
> it could very well have a function name split between two writes where
> the write is at the end of the buffer but not finished writing the
> function name.
>
> > 
> > For example, if userspace set 'set_ftrace_filter' by writing:
> > write(3, "abcdefg", 7)
> 
> From my point of view, the above isn't done writing the function name
> yet and we SHOULD continue waiting for more input.
> 
hmm, thanks for the background. Your above case is a postive use case. So by
this design, instead of write(3, "abcdefg", 7), it should be
write(3, "abcdefg\0", 8), right?

If true, it means kernel expect userspace write every string terminated with
'\0'. So to fix this issue:
open("/sys/kernel/debug/tracing//set_ftrace_pid", O_WRONLY|O_TRUNC) = 3
write(3, " \0", 2)                      = -1 EINVAL (Invalid argument)

Fix would be:
write(3, "\0", 1)?

So far, I am still confused. Some of the tracing debugfs entry accept '\0'
while some not. AFIK, 'echo xxx > <path to tracing file>' always has a '\0'
terminated.

> BIG NACK on this patch. Sorry.
> 
> I'm guessing you have some program that writes only the strlen() of
> these strings. That's wrong, you need to write "strlen()+1". Write some
> real white space between calls, it will work. Add a "write(fd, " ", 1)"
> between calls if you need to. Please don't change the kernel to fix
> some bad use case. Especially when your fix will break existing use
> cases.
> 
> -- Steve
> 
> > 
> > Then in the kernel function ftrace_regex_write(), ftrace_process_regex()
> > will not be executed. The result is that the given filter will not be
> > applied at all.
> > 
> > ftrace_regex_write() {
> > 	...
> > 	read = trace_get_user(parser, ubuf, cnt, ppos);
> > 	if (read >= 0 && trace_parser_loaded(parser) &&
> > 	    !trace_parser_cont(parser)) {
> > 		ret = ftrace_process_regex(iter, parser->buffer,
> > 					   parser->idx, enable);
> > 		...
> > 	}
> > 	...
> > }
> > 
> > Signed-off-by: Changbin Du <changbin.du@intel.com>

-- 
Thanks,
Changbin Du

Re: [PATCH 1/3] tracing: detect the string termination character when parsing user input string

[Steven Rostedt]

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

> hi Rostedt,
> 
> On Tue, Jan 09, 2018 at 05:54:34PM -0500, Steven Rostedt wrote:
> > On Tue,  9 Jan 2018 17:55:46 +0800
> > changbin.du@intel.com wrote:
> >   
> > > From: Changbin Du <changbin.du@intel.com>
> > > 
> > > The usersapce can give a '\0' terminated C string or even has '\0' at the
> > > middle of input buffer. We need handle both these two cases correctly.  
> > 
> > What do you define as correctly. Because I'm not seeing it.
> >  
> Soory I don't fully understand your question. What I meant is want to get clear that
> how will tracing parser below strings.
>   "", "  ",  "\0", " \0 ", "aa\0bb"
> The parser may only recognize certain formats, but whatever the behaviour should
> be clear and coherent for all tracing interfaces.

We have a lack of communication here, because now I'm confused by what
exactly you are asking. ;-)

> 
> > > 
> > > Before this change, trace_get_user() will return a parsed string "\0" in
> > > below case. It is not expected (expects it skip all inputs) and cause the
> > > caller failed.
> > > 
> > > open("/sys/kernel/debug/tracing//set_ftrace_pid", O_WRONLY|O_TRUNC) = 3
> > > write(3, " \0", 2)                      = -1 EINVAL (Invalid argument)  
> > 
> > That looks more like a feature and not a bug.
> >   
> I point this out because I think the parser should take this as an emptry string
> per the comments of trace_get_user().
> /*
>  * trace_get_user - reads the user input string separated by  space
>  * (matched by isspace(ch))
>  *
>  * For each string found the 'struct trace_parser' is updated,
>  * and the function returns.
>  *
>  * Returns number of bytes read.
>  *
>  * See kernel/trace/trace.h for 'struct trace_parser' details.
>  */

I'm also confused about the inconsistency you see here.

> 
> > > 
> > > This patch try to make the parser '\0' aware to fix such issue.  
> > 
> > Why?
> >   
> 
> > > 
> > > Since the caller expects trace_get_user() to parse whole input buffer, so
> > > this patch treat '\0' as a separator as whitespace.  
> > 
> > It looks more like we are trying to fix a userspace bug via the kernel.
> >   
> 
> > I'm not liking this. So NACK.

Thinking about this more, I may not be so against it. I'm guessing you
want to do something like:

	write(fd, "func1", 6);
	write(fd, "func2", 6);

And have it add both func1 and func2, where the '\0' is the separator.

If so, I can see that as a legitimate use case.

-- Steve

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 3/3] tracing: don't set parser->cont if it has reached the end of input buffer

[Steven Rostedt]

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

> write(3, "abcdefg", 7)  
> > 
> > From my point of view, the above isn't done writing the function name
> > yet and we SHOULD continue waiting for more input.
> >   
> hmm, thanks for the background. Your above case is a postive use case. So by
> this design, instead of write(3, "abcdefg", 7), it should be
> write(3, "abcdefg\0", 8), right?

BTW, gcc would translate the above string to 'abcdefg\0\0'. When
defining strings with "", gcc (and all C compilers) append a '\0' to
the end.

But I replied to the first patch, saying that allowing \0 as whitespace
may be OK, given the usecase I showed.

> 
> If true, it means kernel expect userspace write every string terminated with
> '\0'. So to fix this issue:
> open("/sys/kernel/debug/tracing//set_ftrace_pid", O_WRONLY|O_TRUNC) = 3
> write(3, " \0", 2)                      = -1 EINVAL (Invalid argument)
> 
> Fix would be:
> write(3, "\0", 1)?
> 
> So far, I am still confused. Some of the tracing debugfs entry accept '\0'
> while some not. AFIK, 'echo xxx > <path to tracing file>' always has a '\0'
> terminated.

I don't believe that's true.

 $ echo -n abc > /tmp/abc
 $ wc /tmp/abc
 0 1 3 /tmp/abc

Echo writes only the characters you put on the line, nothing more.

Note, when the file descriptor is closed, the code also executes on
what was written but not terminated. That is:

	write(fd, "abc", 3);
	close(fd);

Will keep the "abc" in the continue buffer, but the closing of the file
descriptor will flush it, and execute it.

-- Steve

Re: [PATCH 3/3] tracing: don't set parser->cont if it has reached the end of input buffer

[Du, Changbin]

Hi Rostedt,
On Tue, Jan 09, 2018 at 11:19:36PM -0500, Steven Rostedt wrote:
> On Wed, 10 Jan 2018 11:18:23 +0800
> "Du, Changbin" <changbin.du@intel.com> wrote:
> 
> > write(3, "abcdefg", 7)  
> > > 
> > > From my point of view, the above isn't done writing the function name
> > > yet and we SHOULD continue waiting for more input.
> > >   
> > hmm, thanks for the background. Your above case is a postive use case. So by
> > this design, instead of write(3, "abcdefg", 7), it should be
> > write(3, "abcdefg\0", 8), right?
> 
> BTW, gcc would translate the above string to 'abcdefg\0\0'. When
> defining strings with "", gcc (and all C compilers) append a '\0' to
> the end.
> 
I should clarify the expression here first. :) All the strings here is to express
all the content of a string buffer, including the compiler appended '\0'. (Just like
the output of 'strace').
If this description is still not clear, please let me know!

> But I replied to the first patch, saying that allowing \0 as whitespace
> may be OK, given the usecase I showed.
> 
> > 
> > If true, it means kernel expect userspace write every string terminated with
> > '\0'. So to fix this issue:
> > open("/sys/kernel/debug/tracing//set_ftrace_pid", O_WRONLY|O_TRUNC) = 3
> > write(3, " \0", 2)                      = -1 EINVAL (Invalid argument)
> > 
> > Fix would be:
> > write(3, "\0", 1)?
> > 
> > So far, I am still confused. Some of the tracing debugfs entry accept '\0'
> > while some not. AFIK, 'echo xxx > <path to tracing file>' always has a '\0'
> > terminated.
> 
> I don't believe that's true.
> 
>  $ echo -n abc > /tmp/abc
>  $ wc /tmp/abc
>  0 1 3 /tmp/abc
> 
> Echo writes only the characters you put on the line, nothing more.
> 
Sorry, I misundertood it. The extra character is '\n'.
  $ echo abc > set_ftrace_filter
    0.000 probe:ftrace_filter_write_line0:(ffffffffa7b8db80) ubuf=0xc77408 cnt=0x4)
  $ echo -n abc > set_ftrace_filter
    8889.832 probe:ftrace_filter_write_line0:(ffffffffa7b8db80) ubuf=0xc77408 cnt=0x3)

> Note, when the file descriptor is closed, the code also executes on
> what was written but not terminated. That is:
> 
> 	write(fd, "abc", 3);
> 	close(fd);
> 
> Will keep the "abc" in the continue buffer, but the closing of the file
> descriptor will flush it, and execute it.
> 
Thanks, so now I unstand why below corner case. The userspace try to set the
filter with a unrecognized symbole name (e.g "abcdefg").
open("/sys/kernel/debug/tracing/set_ftrace_filter", O_WRONLY|O_TRUNC) = 3
write(3, "abcdefg", 7)

Since "abcdefg" is not in the symbole list, so we would expect the write return
-EINVAL, right? As below:
# echo abcdefg > set_ftrace_filter
bash: echo: write error: Invalid argument

But the above mechanism hide the error. It return success actually no filter is
apllied at all.
# echo -n abcdefg > set_ftrace_filter

I think in this case kernel may request the userspace append a '\0' or space to the
string buffer so everything can work.

Also there is another corner case. Below write dosn't work.
open("/sys/kernel/debug/tracing//set_ftrace_pid", O_WRONLY|O_TRUNC) = 3
write(3, " \0", 2)                      = -1 EINVAL (Invalid argument)

While these works:
# echo "" > set_ftrace_pid
# echo " " > set_ftrace_pid
# echo -n " " > set_ftrace_pid

These is the reason why I think '\0' should be recognized by the parser.

> -- Steve

-- 
Thanks,
Changbin Du

Re: [PATCH 3/3] tracing: don't set parser->cont if it has reached the end of input buffer

[Steven Rostedt]

On Fri, 12 Jan 2018 12:05:13 +0800
"Du, Changbin" <changbin.du@intel.com> wrote:

> Hi Rostedt,
> On Tue, Jan 09, 2018 at 11:19:36PM -0500, Steven Rostedt wrote:
> > On Wed, 10 Jan 2018 11:18:23 +0800
> > "Du, Changbin" <changbin.du@intel.com> wrote:
> >   
> > > write(3, "abcdefg", 7)    
> > > > 
> > > > From my point of view, the above isn't done writing the function name
> > > > yet and we SHOULD continue waiting for more input.
> > > >     
> > > hmm, thanks for the background. Your above case is a postive use case. So by
> > > this design, instead of write(3, "abcdefg", 7), it should be
> > > write(3, "abcdefg\0", 8), right?  
> > 
> > BTW, gcc would translate the above string to 'abcdefg\0\0'. When
> > defining strings with "", gcc (and all C compilers) append a '\0' to
> > the end.
> >   
> I should clarify the expression here first. :) All the strings here is to express
> all the content of a string buffer, including the compiler appended '\0'. (Just like
> the output of 'strace').
> If this description is still not clear, please let me know!

I was just saying that:

	write(3, "abcdefg\0", 8);

is exactly the same as;

	write(3, "abcdefg", 8);

The '\0' is redundant, as the quoted string adds it. In other words

	sizeof("abcdefg") returns 8

> 
> > But I replied to the first patch, saying that allowing \0 as whitespace
> > may be OK, given the usecase I showed.
> >   
> > > 
> > > If true, it means kernel expect userspace write every string terminated with
> > > '\0'. So to fix this issue:
> > > open("/sys/kernel/debug/tracing//set_ftrace_pid", O_WRONLY|O_TRUNC) = 3
> > > write(3, " \0", 2)                      = -1 EINVAL (Invalid argument)
> > > 
> > > Fix would be:
> > > write(3, "\0", 1)?
> > > 
> > > So far, I am still confused. Some of the tracing debugfs entr0y accept '\0'
> > > while some not. AFIK, 'echo xxx > <path to tracing file>' always has a '\0'
> > > terminated.  
> > 
> > I don't believe that's true.
> > 
> >  $ echo -n abc > /tmp/abc
> >  $ wc /tmp/abc
> >  0 1 3 /tmp/abc
> > 
> > Echo writes only the characters you put on the line, nothing more.
> >   
> Sorry, I misundertood it. The extra character is '\n'.
>   $ echo abc > set_ftrace_filter
>     0.000 probe:ftrace_filter_write_line0:(ffffffffa7b8db80) ubuf=0xc77408 cnt=0x4)
>   $ echo -n abc > set_ftrace_filter
>     8889.832 probe:ftrace_filter_write_line0:(ffffffffa7b8db80) ubuf=0xc77408 cnt=0x3)
> 
> > Note, when the file descriptor is closed, the code also executes on
> > what was written but not terminated. That is:
> > 
> > 	write(fd, "abc", 3);
> > 	close(fd);
> > 
> > Will keep the "abc" in the continue buffer, but the closing of the file
> > descriptor will flush it, and execute it.
> >   
> Thanks, so now I unstand why below corner case. The userspace try to set the
> filter with a unrecognized symbole name (e.g "abcdefg").
> open("/sys/kernel/debug/tracing/set_ftrace_filter", O_WRONLY|O_TRUNC) = 3
> write(3, "abcdefg", 7)
> 
> Since "abcdefg" is not in the symbole list, so we would expect the write return
> -EINVAL, right? As below:
> # echo abcdefg > set_ftrace_filter
> bash: echo: write error: Invalid argument

The write itself doesn't finish the operation. There may be another
write. In other words:

	write(3, "do_", 3);
	write(3, "IRQ\n", 4);

Should both return success, even though it only enabled do_IRQ.

> 
> But the above mechanism hide the error. It return success actually no filter is
> apllied at all.
> # echo -n abcdefg > set_ftrace_filter
> 
> I think in this case kernel may request the userspace append a '\0' or space to the
> string buffer so everything can work.
> 
> Also there is another corner case. Below write dosn't work.
> open("/sys/kernel/debug/tracing//set_ftrace_pid", O_WRONLY|O_TRUNC) = 3
> write(3, " \0", 2)                      = -1 EINVAL (Invalid argument)
> 
> While these works:
> # echo "" > set_ftrace_pid
> # echo " " > set_ftrace_pid
> # echo -n " " > set_ftrace_pid
> 
> These is the reason why I think '\0' should be recognized by the parser.

Hmm, thinking about this more, I do partially agree with you. We should
accept '\0' but I disagree that it should be treated as a space. I
don't want hidden code.

It should be treated as a terminator. And carefully as well.

	write(3, "do_IRQ", 7);

Which will send to the kernel 'd' 'o' '_' 'I' 'R' 'Q' '\0' when the
kernel sees the '\0', and the write has not sent anything else, it
should go ahead and execute 'do_IRQ'

This will allow for this to work:

	char *funcs[] = { "do_IRQ", "schedule", NULL };

	for (i = 0; funcs[i]; i++) {
		ret = write(3, funcs[i], strlen(funcs[i]) + 1);
		if (ret < 0)
			exit(-1);
	}


Now if someone were to write:

	write(3, "do_IRQ\0schedule", 16);

That should return an error.

Why?

Because these are strings, and most tools treat '\0' as a nul
terminator to a string. If we allow for tools to send data after that
nul terminator, we are opening up a way for those interacting with
these tools to sneak in strings that are not visible.

Say we have some admin tools that is doing tracing, and takes input.
And all the input is logged. And say the tool does something like:


	r = read(0, buf, sizeof(buf));
	if (r < 0 || r > sizeof(buf) - 1)
		return -1;
	log("Adding to output %s\n", buf);
	write(3, buf, r);

The "Adding to output" would only show up to the '\0', but if we allow
that write to process after the '\0' then we just allowed the user to
circumvent the log.

-- Steve

Re: [PATCH 3/3] tracing: don't set parser->cont if it has reached the end of input buffer

[Du, Changbin]

On Fri, Jan 12, 2018 at 10:31:08AM -0500, Steven Rostedt wrote:
[...]
> > Thanks, so now I unstand why below corner case. The userspace try to set the
> > filter with a unrecognized symbole name (e.g "abcdefg").
> > open("/sys/kernel/debug/tracing/set_ftrace_filter", O_WRONLY|O_TRUNC) = 3
> > write(3, "abcdefg", 7)
> > 
> > Since "abcdefg" is not in the symbole list, so we would expect the write return
> > -EINVAL, right? As below:
> > # echo abcdefg > set_ftrace_filter
> > bash: echo: write error: Invalid argument
> 
> The write itself doesn't finish the operation. There may be another
> write. In other words:
> 
> 	write(3, "do_", 3);
> 	write(3, "IRQ\n", 4);
> 
> Should both return success, even though it only enabled do_IRQ.
> 
> > 
> > But the above mechanism hide the error. It return success actually no filter is
> > apllied at all.
> > # echo -n abcdefg > set_ftrace_filter
> > 
> > I think in this case kernel may request the userspace append a '\0' or space to the
> > string buffer so everything can work.
> > 
> > Also there is another corner case. Below write dosn't work.
> > open("/sys/kernel/debug/tracing//set_ftrace_pid", O_WRONLY|O_TRUNC) = 3
> > write(3, " \0", 2)                      = -1 EINVAL (Invalid argument)
> > 
> > While these works:
> > # echo "" > set_ftrace_pid
> > # echo " " > set_ftrace_pid
> > # echo -n " " > set_ftrace_pid
> > 
> > These is the reason why I think '\0' should be recognized by the parser.
> 
> Hmm, thinking about this more, I do partially agree with you. We should
> accept '\0' but I disagree that it should be treated as a space. I
> don't want hidden code.
> 
> It should be treated as a terminator. And carefully as well.
> 
> 	write(3, "do_IRQ", 7);
> 
> Which will send to the kernel 'd' 'o' '_' 'I' 'R' 'Q' '\0' when the
> kernel sees the '\0', and the write has not sent anything else, it
> should go ahead and execute 'do_IRQ'
> 
> This will allow for this to work:
> 
> 	char *funcs[] = { "do_IRQ", "schedule", NULL };
> 
> 	for (i = 0; funcs[i]; i++) {
> 		ret = write(3, funcs[i], strlen(funcs[i]) + 1);
> 		if (ret < 0)
> 			exit(-1);
> 	}
> 
> 
> Now if someone were to write:
> 
> 	write(3, "do_IRQ\0schedule", 16);
> 
> That should return an error.
> 
> Why?
> 
> Because these are strings, and most tools treat '\0' as a nul
> terminator to a string. If we allow for tools to send data after that
> nul terminator, we are opening up a way for those interacting with
> these tools to sneak in strings that are not visible.
> 
> Say we have some admin tools that is doing tracing, and takes input.
> And all the input is logged. And say the tool does something like:
> 
> 
> 	r = read(0, buf, sizeof(buf));
> 	if (r < 0 || r > sizeof(buf) - 1)
> 		return -1;
> 	log("Adding to output %s\n", buf);
> 	write(3, buf, r);
> 
> The "Adding to output" would only show up to the '\0', but if we allow
> that write to process after the '\0' then we just allowed the user to
> circumvent the log.
> 
> -- Steve
I agree on your concern. So I will revise this serias and drop the last patch.

-- 
Thanks,
Changbin Du

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