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

[Beau Belgrave <beaub@linux.microsoft.com>]

On Sun, Nov 07, 2021 at 11:31:15PM +0900, Masami Hiramatsu wrote:
> Hi Beau,
> 
> At first, thanks for breaking down your patch into this series!
> 
> Now I found that a suspicious security design issue in this patch.
> 
> On Thu,  4 Nov 2021 10:04:25 -0700
> Beau Belgrave <beaub@linux.microsoft.com> wrote:
> 
> > +
> > +static enum print_line_t user_event_print_trace(struct trace_iterator *iter,
> > +						int flags,
> > +						struct trace_event *event)
> > +{
> > +	/* Unsafe to try to decode user provided print_fmt, use hex */
> > +	trace_print_hex_dump_seq(&iter->seq, "", DUMP_PREFIX_OFFSET, 16,
> > +				 1, iter->ent, iter->ent_size, true);
> 
> You said this is "Unsafe to try to decode user provided" here, because
> this doesn't check the event data sanity, especially non-fixed size data.
> 
> However, it is not enough that you don't decode it here. Because synthetic
> events (histograms) and event filters need to decode this recorded data entry
> using the event format information.
> 
> This means this can cause a buffer overrun issue on the ring buffer, because
> __data_loc (and __rel_loc too) can be compromised by the user.
> 
> If you want to just trace the user events with digit parameters, there is
> a way to close this issue - support only the fixed size types (IOW, drop
> __data_loc/rel_loc support) and always checks the 'length' of the written
> data size. This ensures that those filters/synthetic events can access
> those parameters as 'values'. Maybe eprobes still has to reject the user
> events but the other parts will work well.
> 
> If you want to log some "string", it is hard. Maybe it needs a special
> check when writing the event (address, length, and null termination.),
> but it will increase the recording overhead.
> 
> Thank you,
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

Does that mean the decoders in eprobes/histogram don't check event
record sizes before accessing the data? Shouldn't that get fix
centrally? That would mean a loaded module could do the same thing
(user_events only works if the user has access to tracefs, so it's not
like it's open to all users).

Is it possible to mark trace_events with a flag that says "don't trust
this"? That way eBPF, ftrace and perf would still allow recording and
decoding offline (in a safer context).

From user_events code, an entry is always allocated with enough data and
if it fails (either too big or alloc failure) the event is not written
out. This appears to be purely in the decode logic that is not within
this patch?

Thanks,
-Beau