Re: [PATCH v2] tracing: Allow for max buffer data size trace_marker writes

[Steven Rostedt]

On Tue, 12 Dec 2023 09:33:11 -0500
Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx> wrote:

> On 2023-12-12 09:00, Steven Rostedt wrote:
> [...]
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -7272,6 +7272,7 @@ tracing_mark_write(struct file *filp, const char __user *ubuf,
> >   	enum event_trigger_type tt = ETT_NONE;
> >   	struct trace_buffer *buffer;
> >   	struct print_entry *entry;
> > +	int meta_size;
> >   	ssize_t written;
> >   	int size;
> >   	int len;
> > @@ -7286,12 +7287,9 @@ tracing_mark_write(struct file *filp, const char __user *ubuf,
> >   	if (!(tr->trace_flags & TRACE_ITER_MARKERS))
> >   		return -EINVAL;
> >   
> > -	if (cnt > TRACE_BUF_SIZE)
> > -		cnt = TRACE_BUF_SIZE;  
> 
> You're removing an early bound check for a size_t userspace input...
> 
> > -
> > -	BUILD_BUG_ON(TRACE_BUF_SIZE >= PAGE_SIZE);
> > -
> > -	size = sizeof(*entry) + cnt + 2; /* add '\0' and possible '\n' */
> > +	meta_size = sizeof(*entry) + 2;  /* add '\0' and possible '\n' */
> > + again:
> > +	size = cnt + meta_size;  
> 
> ... and then implicitly casting it into a "int" size variable, which
> can therefore become a negative value.
> 
> Just for the sake of not having to rely on ring_buffer_lock_reserve
> catching (length > BUF_MAX_DATA_SIZE), I would recommend to add an
> early check for negative here.

size_t is not signed, so nothing should be negative. But you are right, I
need to have "size" be of size_t type too to prevent the overflow.

And I could make cnt of ssize_t type and check for negative and fail early
in such a case.

Thanks!

> 
> >   
> >   	/* If less than "<faulted>", then make sure we can still add
> > that */ if (cnt < FAULTED_SIZE)
> > @@ -7300,9 +7298,25 @@ tracing_mark_write(struct file *filp, const char
> > __user *ubuf, buffer = tr->array_buffer.buffer;
> >   	event = __trace_buffer_lock_reserve(buffer, TRACE_PRINT, size,
> >   					    tracing_gen_ctx());
> > -	if (unlikely(!event))
> > +	if (unlikely(!event)) {
> > +		/*
> > +		 * If the size was greated than what was allowed, then
> >  
> 
> greater ?

Nah, the size is "greated" like "greated cheese" ;-)

Thanks for the review, I'll send out a v3.

-- Steve