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

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]

Message-ID: <acf17610-e427-4b78-949f-2b1d03400d5f@huaweicloud.com>
Date: Wed, 6 Aug 2025 11:14:32 +0800
From: Pu Lehui <pulehui@...weicloud.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: mhiramat@...nel.org, mathieu.desnoyers@...icios.com,
 linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH] tracing: Zero-initialize parser->buffer to avoid OOB
 access



On 2025/8/5 23:19, Steven Rostedt wrote:
> On Tue,  5 Aug 2025 15:12:03 +0000
> Pu Lehui <pulehui@...weicloud.com> wrote:
> 
>> From: Pu Lehui <pulehui@...wei.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@...wei.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