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

[Masami Hiramatsu <mhiramat@kernel.org>]

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>