Re: [PATCH v4 02/10] user_events: Add minimal support for trace_event into ftrace

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 8 Nov 2021 12:25:27 -0800
Beau Belgrave <beaub@linux.microsoft.com> wrote:

> It seems there are 2 concerns:
> 1. If data comes in and it's not in the size that is specified, it's
> suspicious and should either be truncated or ignored. Maybe under
> ignore, over truncate.
> 
> 2. If the data is more than specified, it must be checked to see if
> there are __data_loc / __rel_loc entries and they must be validated as
> within range of accepted limits. If there are no __data_loc / __rel_loc
> it should either be truncated or ignored.
> 
> Is there more that I may have missed?
> 
> I'd like to know if I do fix them that the features like filtering will still
> be available to user_events or if it's better to just add flags to disable
> kernel filtering?

If these are "user defined" then perhaps we add a wrapper to the filtering
that is called instead of the normal filtering for theses events that
verify the fields of the events being filtered are located on the ring
buffer. Although, strings and such are rare or just slow in filtering that
we could make sure the content is still on the buffer that is being
filtered.

> 
> I'm still unsure this is limited to just user_events.
> 
> For example, why doesn't filter_pred_strloc and filter_pred_pchar in
> trace_events_filter.c check the boundary it will be accessing?
> 
> It seems like tracepoints from kernel modules, while more trusted, can also
> cause this kind of thing due to bugs, etc.

Yes, it's the fact that the code is created in the kernel, and the only way
that the filtering could be out of bounds is if there's a bug in the
kernel. We rather not check for that if it slows down the tracing. But
perhaps if we can show that the checks are only done for dynamic strings
and arrays, that it doesn't cause noticeable overhead, it may be fine to
keep for all events.

-- Steve