1. Patchew
  2. linux
  3. View series

[PATCH v2] tracing: Limit access to parser->buffer when trace_get_user failed

Pu Lehui posted 1 patch 2 months ago 
kernel/trace/trace.c | 20 ++++++++++++++------
kernel/trace/trace.h |  3 ++-
2 files changed, 16 insertions(+), 7 deletions(-)
[PATCH v2] tracing: Limit access to parser->buffer when trace_get_user failed
Posted by Pu Lehui 2 months ago 
From: Pu Lehui <pulehui@huawei.com>

When the length of the string written to set_ftrace_filter exceeds
FTRACE_BUFF_MAX, the following KASAN alarm will be triggered:

BUG: KASAN: slab-out-of-bounds in strsep+0x18c/0x1b0
Read of size 1 at addr ffff0000d00bd5ba by task ash/165

CPU: 1 UID: 0 PID: 165 Comm: ash Not tainted 6.16.0-g6bcdbd62bd56-dirty
Hardware name: linux,dummy-virt (DT)
Call trace:
 show_stack+0x34/0x50 (C)
 dump_stack_lvl+0xa0/0x158
 print_address_description.constprop.0+0x88/0x398
 print_report+0xb0/0x280
 kasan_report+0xa4/0xf0
 __asan_report_load1_noabort+0x20/0x30
 strsep+0x18c/0x1b0
 ftrace_process_regex.isra.0+0x100/0x2d8
 ftrace_regex_release+0x484/0x618
 __fput+0x364/0xa58
 ____fput+0x28/0x40
 task_work_run+0x154/0x278
 do_notify_resume+0x1f0/0x220
 el0_svc+0xec/0xf0
 el0t_64_sync_handler+0xa0/0xe8
 el0t_64_sync+0x1ac/0x1b0

The reason is that trace_get_user will fail when processing a string
longer than FTRACE_BUFF_MAX, but not set the end of parser->buffer to 0.
Then an OOB access will be triggered in ftrace_regex_release->
ftrace_process_regex->strsep->strpbrk. We can solve this problem by
limiting access to parser->buffer when trace_get_user failed.

Fixes: 8c9af478c06b ("ftrace: Handle commands when closing set_ftrace_filter file")
Signed-off-by: Pu Lehui <pulehui@huawei.com>
---

v2:
- Add `fail` field to struct trace_parser to indicate parsing failed.

v1:
https://lore.kernel.org/all/20250805151203.1214790-1-pulehui@huaweicloud.com/

 kernel/trace/trace.c | 20 ++++++++++++++------
 kernel/trace/trace.h |  3 ++-
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 4283ed4e8f59..138212f4ecde 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1814,9 +1814,11 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
 	if (!*ppos)
 		trace_parser_clear(parser);
 
+	parser->fail = false;
+
 	ret = get_user(ch, ubuf++);
 	if (ret)
-		return ret;
+		goto fail;
 
 	read++;
 	cnt--;
@@ -1830,7 +1832,7 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
 		while (cnt && isspace(ch)) {
 			ret = get_user(ch, ubuf++);
 			if (ret)
-				return ret;
+				goto fail;
 			read++;
 			cnt--;
 		}
@@ -1848,12 +1850,14 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
 	while (cnt && !isspace(ch) && ch) {
 		if (parser->idx < parser->size - 1)
 			parser->buffer[parser->idx++] = ch;
-		else
-			return -EINVAL;
+		else {
+			ret = -EINVAL;
+			goto fail;
+		}
 
 		ret = get_user(ch, ubuf++);
 		if (ret)
-			return ret;
+			goto fail;
 		read++;
 		cnt--;
 	}
@@ -1868,11 +1872,15 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
 		/* Make sure the parsed string always terminates with '\0'. */
 		parser->buffer[parser->idx] = 0;
 	} else {
-		return -EINVAL;
+		ret = -EINVAL;
+		goto fail;
 	}
 
 	*ppos += read;
 	return read;
+fail:
+	parser->fail = true;
+	return ret;
 }
 
 /* TODO add a seq_buf_to_buffer() */
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 1dbf1d3cf2f1..5072bb25a860 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -1292,6 +1292,7 @@ bool ftrace_event_is_function(struct trace_event_call *call);
  */
 struct trace_parser {
 	bool		cont;
+	bool		fail;
 	char		*buffer;
 	unsigned	idx;
 	unsigned	size;
@@ -1299,7 +1300,7 @@ struct trace_parser {
 
 static inline bool trace_parser_loaded(struct trace_parser *parser)
 {
-	return (parser->idx != 0);
+	return !parser->fail && parser->idx != 0;
 }
 
 static inline bool trace_parser_cont(struct trace_parser *parser)
-- 
2.34.1
Re: [PATCH v2] tracing: Limit access to parser->buffer when trace_get_user failed
Posted by Steven Rostedt 1 month, 3 weeks ago 
On Wed,  6 Aug 2025 07:01:09 +0000
Pu Lehui <pulehui@huaweicloud.com> wrote:

> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 4283ed4e8f59..138212f4ecde 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -1814,9 +1814,11 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
>  	if (!*ppos)
>  		trace_parser_clear(parser);
>  
> +	parser->fail = false;

This should be set when the parser is initialized.

> +
>  	ret = get_user(ch, ubuf++);
>  	if (ret)
> -		return ret;
> +		goto fail;
>  
>  	read++;
>  	cnt--;
> @@ -1830,7 +1832,7 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
>  		while (cnt && isspace(ch)) {
>  			ret = get_user(ch, ubuf++);
>  			if (ret)
> -				return ret;
> +				goto fail;
>  			read++;
>  			cnt--;
>  		}
> @@ -1848,12 +1850,14 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
>  	while (cnt && !isspace(ch) && ch) {
>  		if (parser->idx < parser->size - 1)
>  			parser->buffer[parser->idx++] = ch;
> -		else
> -			return -EINVAL;
> +		else {
> +			ret = -EINVAL;
> +			goto fail;
> +		}
>  
>  		ret = get_user(ch, ubuf++);
>  		if (ret)
> -			return ret;
> +			goto fail;
>  		read++;
>  		cnt--;
>  	}
> @@ -1868,11 +1872,15 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
>  		/* Make sure the parsed string always terminates with '\0'. */
>  		parser->buffer[parser->idx] = 0;
>  	} else {
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto fail;
>  	}
>  
>  	*ppos += read;
>  	return read;
> +fail:
> +	parser->fail = true;

Should have a helper function called: trace_parser_fail(parser) and use
that.

-- Steve


> +	return ret;
>  }
>  
>  /* TODO add a seq_buf_to_buffer() */
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 1dbf1d3cf2f1..5072bb25a860 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -1292,6 +1292,7 @@ bool ftrace_event_is_function(struct trace_event_call *call);
>   */
>  struct trace_parser {
>  	bool		cont;
> +	bool		fail;
>  	char		*buffer;
>  	unsigned	idx;
>  	unsigned	size;
> @@ -1299,7 +1300,7 @@ struct trace_parser {
>  
>  static inline bool trace_parser_loaded(struct trace_parser *parser)
>  {
> -	return (parser->idx != 0);
> +	return !parser->fail && parser->idx != 0;
>  }
>  
>  static inline bool trace_parser_cont(struct trace_parser *parser)
Re: [PATCH v2] tracing: Limit access to parser->buffer when trace_get_user failed
Posted by Pu Lehui 1 month, 3 weeks ago 

On 2025/8/13 2:38, Steven Rostedt wrote:
> On Wed,  6 Aug 2025 07:01:09 +0000
> Pu Lehui <pulehui@huaweicloud.com> wrote:
> 
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index 4283ed4e8f59..138212f4ecde 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -1814,9 +1814,11 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
>>   	if (!*ppos)
>>   		trace_parser_clear(parser);
>>   
>> +	parser->fail = false;
> 
> This should be set when the parser is initialized.

Hi Steven,

parser->fail will always be set `false` when the parser is initialized, 
as trace_parser_get_init will do `memset(parser, 0, sizeof(*parser))`.

I will remove this in next.

> 
>> +
>>   	ret = get_user(ch, ubuf++);
>>   	if (ret)
>> -		return ret;
>> +		goto fail;
>>   
>>   	read++;
>>   	cnt--;
>> @@ -1830,7 +1832,7 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
>>   		while (cnt && isspace(ch)) {
>>   			ret = get_user(ch, ubuf++);
>>   			if (ret)
>> -				return ret;
>> +				goto fail;
>>   			read++;
>>   			cnt--;
>>   		}
>> @@ -1848,12 +1850,14 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
>>   	while (cnt && !isspace(ch) && ch) {
>>   		if (parser->idx < parser->size - 1)
>>   			parser->buffer[parser->idx++] = ch;
>> -		else
>> -			return -EINVAL;
>> +		else {
>> +			ret = -EINVAL;
>> +			goto fail;
>> +		}
>>   
>>   		ret = get_user(ch, ubuf++);
>>   		if (ret)
>> -			return ret;
>> +			goto fail;
>>   		read++;
>>   		cnt--;
>>   	}
>> @@ -1868,11 +1872,15 @@ int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
>>   		/* Make sure the parsed string always terminates with '\0'. */
>>   		parser->buffer[parser->idx] = 0;
>>   	} else {
>> -		return -EINVAL;
>> +		ret = -EINVAL;
>> +		goto fail;
>>   	}
>>   
>>   	*ppos += read;
>>   	return read;
>> +fail:
>> +	parser->fail = true;
> 
> Should have a helper function called: trace_parser_fail(parser) and use
> that.

Alright! Will make it next version.

> 
> -- Steve
> 
> 
>> +	return ret;
>>   }
>>   
>>   /* TODO add a seq_buf_to_buffer() */
>> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
>> index 1dbf1d3cf2f1..5072bb25a860 100644
>> --- a/kernel/trace/trace.h
>> +++ b/kernel/trace/trace.h
>> @@ -1292,6 +1292,7 @@ bool ftrace_event_is_function(struct trace_event_call *call);
>>    */
>>   struct trace_parser {
>>   	bool		cont;
>> +	bool		fail;
>>   	char		*buffer;
>>   	unsigned	idx;
>>   	unsigned	size;
>> @@ -1299,7 +1300,7 @@ struct trace_parser {
>>   
>>   static inline bool trace_parser_loaded(struct trace_parser *parser)
>>   {
>> -	return (parser->idx != 0);
>> +	return !parser->fail && parser->idx != 0;
>>   }
>>   
>>   static inline bool trace_parser_cont(struct trace_parser *parser)

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.