linux-kernel - Re: [PATCH v4 02/10] user_events: Add minimal support for trace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 8 Nov 2021 12:25:27 -0800
From:   Beau Belgrave <beaub@...ux.microsoft.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Masami Hiramatsu <mhiramat@...nel.org>,
        linux-trace-devel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 02/10] user_events: Add minimal support for
 trace_event into ftrace

On Mon, Nov 08, 2021 at 01:16:39PM -0500, Steven Rostedt wrote:
> On Mon, 8 Nov 2021 09:13:36 -0800
> Beau Belgrave <beaub@...ux.microsoft.com> wrote:
> 
> 
> > 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).
> 
> There's checks to make sure everything fits in eprobes and kprobes. If it
> doesn't then the event is simply dropped.
> 
> For example, if you look at __eprobe_trace_func() in trace_eprobe.c, you'll
> see that it calls get_eprobe_size(), which goes through and just reads what
> it is about to accept. Then it reserves the amount of data on the ring
> buffer, and then calls store_trace_args() which also passes in the size
> that it found, in case things change. If it's too big, it only records what
> it originally intended.
> 
> -- Steve

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?

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.

Thanks,
-Beau