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

[Pu Lehui]

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