1. Patchew
  2. linux
  3. View series

[PATCH] tracing: Zero-initialize parser->buffer to avoid OOB access

Pu Lehui posted 1 patch 2 months ago 
kernel/trace/trace.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
[PATCH] tracing: Zero-initialize parser->buffer to avoid OOB access
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 #3 NONE
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
zero-initializing parser->buffer.

Fixes: 8c9af478c06b ("ftrace: Handle commands when closing set_ftrace_filter file")
Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
 kernel/trace/trace.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 4283ed4e8f59..97e66cbd3617 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1776,7 +1776,7 @@ int trace_parser_get_init(struct trace_parser *parser, int size)
 {
 	memset(parser, 0, sizeof(*parser));
 
-	parser->buffer = kmalloc(size, GFP_KERNEL);
+	parser->buffer = kzalloc(size, GFP_KERNEL);
 	if (!parser->buffer)
 		return 1;
 
@@ -1860,13 +1860,10 @@ 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) || !ch) {
-		parser->buffer[parser->idx] = 0;
 		parser->cont = false;
 	} else if (parser->idx < parser->size - 1) {
 		parser->cont = true;
 		parser->buffer[parser->idx++] = ch;
-		/* Make sure the parsed string always terminates with '\0'. */
-		parser->buffer[parser->idx] = 0;
 	} else {
 		return -EINVAL;
 	}
-- 
2.34.1
Re: [PATCH] tracing: Zero-initialize parser->buffer to avoid OOB access
Posted by Steven Rostedt 2 months ago 
On Tue,  5 Aug 2025 15:12:03 +0000
Pu Lehui <pulehui@huaweicloud.com> wrote:

> 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 #3 NONE
> 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
> zero-initializing parser->buffer.
> 
> Fixes: 8c9af478c06b ("ftrace: Handle commands when closing set_ftrace_filter file")

Fix one bug then create another!!!

> Signed-off-by: Pu Lehui <pulehui@huawei.com>
> ---
>  kernel/trace/trace.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 4283ed4e8f59..97e66cbd3617 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -1776,7 +1776,7 @@ int trace_parser_get_init(struct trace_parser *parser, int size)
>  {
>  	memset(parser, 0, sizeof(*parser));
>  
> -	parser->buffer = kmalloc(size, GFP_KERNEL);
> +	parser->buffer = kzalloc(size, GFP_KERNEL);
>  	if (!parser->buffer)
>  		return 1;
>  
> @@ -1860,13 +1860,10 @@ 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) || !ch) {
> -		parser->buffer[parser->idx] = 0;
>  		parser->cont = false;
>  	} else if (parser->idx < parser->size - 1) {
>  		parser->cont = true;
>  		parser->buffer[parser->idx++] = ch;
> -		/* Make sure the parsed string always terminates with '\0'. */
> -		parser->buffer[parser->idx] = 0;

The returned buffer is expected to end with a '0'. This now removes that if
the idx is the length of the parser.

>  	} else {
>  		return -EINVAL;
>  	}

The real fix would be:

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index d0b1964648c1..422224a55f1d 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -815,7 +815,7 @@ int trace_pid_write(struct trace_pid_list *filtered_pids,
 	loff_t pos;
 	pid_t pid;
 
-	if (trace_parser_get_init(&parser, PID_BUF_SIZE + 1))
+	if (trace_parser_get_init(&parser, PID_BUF_SIZE))
 		return -ENOMEM;
 
 	/*
@@ -1776,7 +1776,7 @@ int trace_parser_get_init(struct trace_parser *parser, int size)
 {
 	memset(parser, 0, sizeof(*parser));
 
-	parser->buffer = kmalloc(size, GFP_KERNEL);
+	parser->buffer = kmalloc(size + 1, GFP_KERNEL);
 	if (!parser->buffer)
 		return 1;
 
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 05447b958a1a..a3ce88a48947 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -1507,7 +1507,7 @@ ftrace_event_write(struct file *file, const char __user *ubuf,
 	if (ret < 0)
 		return ret;
 
-	if (trace_parser_get_init(&parser, EVENT_BUF_SIZE + 1))
+	if (trace_parser_get_init(&parser, EVENT_BUF_SIZE))
 		return -ENOMEM;
 
 	read = trace_get_user(&parser, ubuf, cnt, ppos);

-- Steve
Re: [PATCH] tracing: Zero-initialize parser->buffer to avoid OOB access
Posted by Pu Lehui 2 months ago 

On 2025/8/5 23:19, Steven Rostedt wrote:
> On Tue,  5 Aug 2025 15:12:03 +0000
> Pu Lehui <pulehui@huaweicloud.com> wrote:
> 
>> 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 #3 NONE
>> 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
>> zero-initializing parser->buffer.
>>
>> Fixes: 8c9af478c06b ("ftrace: Handle commands when closing set_ftrace_filter file")
> 
> Fix one bug then create another!!!
> 
>> Signed-off-by: Pu Lehui <pulehui@huawei.com>
>> ---
>>   kernel/trace/trace.c | 5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index 4283ed4e8f59..97e66cbd3617 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -1776,7 +1776,7 @@ int trace_parser_get_init(struct trace_parser *parser, int size)
>>   {
>>   	memset(parser, 0, sizeof(*parser));
>>   
>> -	parser->buffer = kmalloc(size, GFP_KERNEL);
>> +	parser->buffer = kzalloc(size, GFP_KERNEL);
>>   	if (!parser->buffer)
>>   		return 1;
>>   
>> @@ -1860,13 +1860,10 @@ 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) || !ch) {
>> -		parser->buffer[parser->idx] = 0;
>>   		parser->cont = false;
>>   	} else if (parser->idx < parser->size - 1) {
>>   		parser->cont = true;
>>   		parser->buffer[parser->idx++] = ch;
>> -		/* Make sure the parsed string always terminates with '\0'. */
>> -		parser->buffer[parser->idx] = 0;
> 
> The returned buffer is expected to end with a '0'. This now removes that if
> the idx is the length of the parser.

Hi Steven,

Not really, if the idx is the length of the parser, which also mean that 
the length of user string is parser->size, then it will goto fail branch 
[0]. And with this patch, it will also terminates with '\0'.

Link: 
https://elixir.bootlin.com/linux/v6.16/source/kernel/trace/trace.c#L1903 [0]

> 
>>   	} else {
>>   		return -EINVAL;
>>   	}
> 
> The real fix would be:
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index d0b1964648c1..422224a55f1d 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -815,7 +815,7 @@ int trace_pid_write(struct trace_pid_list *filtered_pids,
>   	loff_t pos;
>   	pid_t pid;
>   
> -	if (trace_parser_get_init(&parser, PID_BUF_SIZE + 1))
> +	if (trace_parser_get_init(&parser, PID_BUF_SIZE))
>   		return -ENOMEM;
>   
>   	/*
> @@ -1776,7 +1776,7 @@ int trace_parser_get_init(struct trace_parser *parser, int size)
>   {
>   	memset(parser, 0, sizeof(*parser));
>   
> -	parser->buffer = kmalloc(size, GFP_KERNEL);
> +	parser->buffer = kmalloc(size + 1, GFP_KERNEL);
>   	if (!parser->buffer)
>   		return 1;
>   
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 05447b958a1a..a3ce88a48947 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -1507,7 +1507,7 @@ ftrace_event_write(struct file *file, const char __user *ubuf,
>   	if (ret < 0)
>   		return ret;
>   
> -	if (trace_parser_get_init(&parser, EVENT_BUF_SIZE + 1))
> +	if (trace_parser_get_init(&parser, EVENT_BUF_SIZE))
>   		return -ENOMEM;
>   
>   	read = trace_get_user(&parser, ubuf, cnt, ppos);
> 
> -- Steve
Re: [PATCH] tracing: Zero-initialize parser->buffer to avoid OOB access
Posted by Pu Lehui 2 months ago 

On 2025/8/6 11:14, Pu Lehui wrote:
> 
> 
> On 2025/8/5 23:19, Steven Rostedt wrote:
>> On Tue,  5 Aug 2025 15:12:03 +0000
>> Pu Lehui <pulehui@huaweicloud.com> wrote:
>>
>>> 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 #3 NONE
>>> 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
>>> zero-initializing parser->buffer.
>>>
>>> Fixes: 8c9af478c06b ("ftrace: Handle commands when closing 
>>> set_ftrace_filter file")
>>
>> Fix one bug then create another!!!
>>
>>> Signed-off-by: Pu Lehui <pulehui@huawei.com>
>>> ---
>>>   kernel/trace/trace.c | 5 +----
>>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>>> index 4283ed4e8f59..97e66cbd3617 100644
>>> --- a/kernel/trace/trace.c
>>> +++ b/kernel/trace/trace.c
>>> @@ -1776,7 +1776,7 @@ int trace_parser_get_init(struct trace_parser 
>>> *parser, int size)
>>>   {
>>>       memset(parser, 0, sizeof(*parser));
>>> -    parser->buffer = kmalloc(size, GFP_KERNEL);
>>> +    parser->buffer = kzalloc(size, GFP_KERNEL);
>>>       if (!parser->buffer)
>>>           return 1;
>>> @@ -1860,13 +1860,10 @@ 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) || !ch) {
>>> -        parser->buffer[parser->idx] = 0;
>>>           parser->cont = false;
>>>       } else if (parser->idx < parser->size - 1) {
>>>           parser->cont = true;
>>>           parser->buffer[parser->idx++] = ch;
>>> -        /* Make sure the parsed string always terminates with '\0'. */
>>> -        parser->buffer[parser->idx] = 0;
>>
>> The returned buffer is expected to end with a '0'. This now removes 
>> that if
>> the idx is the length of the parser.
> 
> Hi Steven,
> 
> Not really, if the idx is the length of the parser, which also mean that 
> the length of user string is parser->size, then it will goto fail branch 
> [0]. And with this patch, it will also terminates with '\0'.
> 
> Link: 
> https://elixir.bootlin.com/linux/v6.16/source/kernel/trace/trace.c#L1903 
> [0]
> 

Hi Steven,

After a while of thinking, I think we should limit the use of 
parser->buffer when trace_get_user fails to parse. And I sent a new 
version [0]. Hope that will be fine.

Link: 
https://lore.kernel.org/all/20250806070109.1320165-1-pulehui@huaweicloud.com/ 
[0]

>>
>>>       } else {
>>>           return -EINVAL;
>>>       }
>>
>> The real fix would be:
>>
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index d0b1964648c1..422224a55f1d 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -815,7 +815,7 @@ int trace_pid_write(struct trace_pid_list 
>> *filtered_pids,
>>       loff_t pos;
>>       pid_t pid;
>> -    if (trace_parser_get_init(&parser, PID_BUF_SIZE + 1))
>> +    if (trace_parser_get_init(&parser, PID_BUF_SIZE))
>>           return -ENOMEM;
>>       /*
>> @@ -1776,7 +1776,7 @@ int trace_parser_get_init(struct trace_parser 
>> *parser, int size)
>>   {
>>       memset(parser, 0, sizeof(*parser));
>> -    parser->buffer = kmalloc(size, GFP_KERNEL);
>> +    parser->buffer = kmalloc(size + 1, GFP_KERNEL);
>>       if (!parser->buffer)
>>           return 1;
>> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
>> index 05447b958a1a..a3ce88a48947 100644
>> --- a/kernel/trace/trace_events.c
>> +++ b/kernel/trace/trace_events.c
>> @@ -1507,7 +1507,7 @@ ftrace_event_write(struct file *file, const char 
>> __user *ubuf,
>>       if (ret < 0)
>>           return ret;
>> -    if (trace_parser_get_init(&parser, EVENT_BUF_SIZE + 1))
>> +    if (trace_parser_get_init(&parser, EVENT_BUF_SIZE))
>>           return -ENOMEM;
>>       read = trace_get_user(&parser, ubuf, cnt, ppos);
>>
>> -- Steve

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.