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

[Beau Belgrave]

On Mon, Nov 08, 2021 at 04:00:27PM -0500, Steven Rostedt wrote:
> On Mon, 8 Nov 2021 12:25:27 -0800
> Beau Belgrave <beaub@xxxxxxxxxxxxxxxxxxx> 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.
> 

It seems like both histograms and filter both reference field flags to
determine how to get the data.

How would you feel about another FILTER_* flag on fields, like:
FILTER_DYN_STRING_SAFE
FILTER_PTR_STRING_SAFE

user_events when parsing would instead of leaving FILTER_OTHER for
__data_loc / __rel_loc switch to the above.

The predicate filter method would then switch based on those types to
safer versions.

That way other parts could take advantage of this if needed beyond
user_events.

If this is addressed at the filter/histogram level, would then the write
callsites still check bounds per-write? Or maybe only care about the
undersized data cases?

> > 
> > 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

Thanks,
-Beau