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

[Pu Lehui <pulehui@huaweicloud.com>]

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)