linux-kernel - Re: [PATCH v2] tracing: Limit access to parser->buffer when trace_get

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] [day] [month] [year] [list]

Message-ID: <83c1747d-1e8e-45e9-a6a4-619bad4db1d4@huaweicloud.com>
Date: Wed, 13 Aug 2025 11:48:21 +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,
 pulehui@...wei.com
Subject: Re: [PATCH v2] tracing: Limit access to parser->buffer when
 trace_get_user failed



On 2025/8/13 2:38, Steven Rostedt wrote:
> On Wed,  6 Aug 2025 07:01:09 +0000
> Pu Lehui <pulehui@...weicloud.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)