| 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
| ||
|
Message-Id: <20220527010314.e5a2d9418cfdf5500d75b9b2@kernel.org>
Date: Fri, 27 May 2022 01:03:14 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: Linyu Yuan <quic_linyyuan@...cinc.com>,
Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
Masami Hiramatsu <mhiramat@...nel.org>,
Tom Zanussi <zanussi@...nel.org>
Subject: Re: [PATCH 2/2] tracing/probes: make match function safe
On Tue, 24 May 2022 18:34:03 -0400
Steven Rostedt <rostedt@...dmis.org> wrote:
> Masami and Tom, what are you thoughts on this?
>
> -- Steve
Thanks for forwarding.
>
> On Sun, May 01, 2022 at 05:34:11PM +0800, Linyu Yuan wrote:
> > When delete one kprobe/uprobe/eprobe event entry
> > using /sys/kernel/debug/tracing/dynamic_events file,
> > it will loop all dynamic envent entries,
> > as user will not input dyn_event_operations type,
> > when call the match function of kprobe/uprobe/eprobe,
> > the dynamic event may have different dyn_event_operations type,
> > but currently match function may return a match.
> >
> > Fix by check dyn_event_operations type first.
Sorry, NACK. That check is not necessary, because the 'match' operation
is chosen by each event::ops as below.
int dyn_event_release(const char *raw_command, struct dyn_event_operations *type)
{
struct dyn_event *pos, *n;
...
mutex_lock(&event_mutex);
for_each_dyn_event_safe(pos, n) {
if (type && type != pos->ops)
continue;
if (!pos->ops->match(system, event,
argc - 1, (const char **)argv + 1, pos))
continue;
...
The @pos is dyn_event. Thus @pos->ops must be the appropriate
dyn_event_operations for that event. For example, if there is an
eprobe event @ev, then @ev->ops must be eprobe_dyn_event_ops and
@ev->ops->match must be eprobe_dyn_event_match() (unless the match
function is shared with another dyn_event_operations.)
Thank you,
> >
> > Signed-off-by: Linyu Yuan <quic_linyyuan@...cinc.com>
> > ---
> > kernel/trace/trace_eprobe.c | 31 +++++++++++++++++++++++--------
> > kernel/trace/trace_kprobe.c | 3 +++
> > kernel/trace/trace_uprobe.c | 3 +++
> > 3 files changed, 29 insertions(+), 8 deletions(-)
> >
> > diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> > index b16e067..0029840 100644
> > --- a/kernel/trace/trace_eprobe.c
> > +++ b/kernel/trace/trace_eprobe.c
> > @@ -19,6 +19,21 @@
> >
> > #define EPROBE_EVENT_SYSTEM "eprobes"
> >
> > +static int eprobe_dyn_event_create(const char *raw_command);
> > +static int eprobe_dyn_event_show(struct seq_file *m, struct dyn_event *ev);
> > +static bool eprobe_dyn_event_is_busy(struct dyn_event *ev);
> > +static int eprobe_dyn_event_release(struct dyn_event *ev);
> > +static bool eprobe_dyn_event_match(const char *system, const char *event,
> > + int argc, const char **argv, struct dyn_event *ev);
> > +
> > +static struct dyn_event_operations eprobe_dyn_event_ops = {
> > + .create = eprobe_dyn_event_create,
> > + .show = eprobe_dyn_event_show,
> > + .is_busy = eprobe_dyn_event_is_busy,
> > + .free = eprobe_dyn_event_release,
> > + .match = eprobe_dyn_event_match,
> > +};
> > +
> > struct trace_eprobe {
> > /* tracepoint system */
> > const char *event_system;
> > @@ -39,6 +54,11 @@ struct eprobe_data {
> >
> > static int __trace_eprobe_create(int argc, const char *argv[]);
> >
> > +static bool is_trace_eprobe(struct dyn_event *ev)
> > +{
> > + return ev->ops == &eprobe_dyn_event_ops;
> > +}
> > +
> > static void trace_event_probe_cleanup(struct trace_eprobe *ep)
> > {
> > if (!ep)
> > @@ -121,6 +141,9 @@ static bool eprobe_dyn_event_match(const char *system, const char *event,
> > struct trace_eprobe *ep = to_trace_eprobe(ev);
> > const char *slash;
> >
> > + if (!is_trace_eprobe(ev))
> > + return false;
> > +
> > /*
> > * We match the following:
> > * event only - match all eprobes with event name
> > @@ -174,14 +197,6 @@ static bool eprobe_dyn_event_match(const char *system, const char *event,
> > return trace_probe_match_command_args(&ep->tp, argc, argv);
> > }
> >
> > -static struct dyn_event_operations eprobe_dyn_event_ops = {
> > - .create = eprobe_dyn_event_create,
> > - .show = eprobe_dyn_event_show,
> > - .is_busy = eprobe_dyn_event_is_busy,
> > - .free = eprobe_dyn_event_release,
> > - .match = eprobe_dyn_event_match,
> > -};
> > -
> > static struct trace_eprobe *alloc_event_probe(const char *group,
> > const char *this_event,
> > struct trace_event_call *event,
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index 2cd8ef9..f63abfa 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -163,6 +163,9 @@ static bool trace_kprobe_match(const char *system, const char *event,
> > {
> > struct trace_kprobe *tk = to_trace_kprobe(ev);
> >
> > + if (!is_trace_kprobe(ev))
> > + return false;
> > +
> > return (event[0] == '\0' ||
> > strcmp(trace_probe_name(&tk->tp), event) == 0) &&
> > (!system || strcmp(trace_probe_group_name(&tk->tp), system) == 0) &&
> > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> > index a935c08..ee55ed0 100644
> > --- a/kernel/trace/trace_uprobe.c
> > +++ b/kernel/trace/trace_uprobe.c
> > @@ -312,6 +312,9 @@ static bool trace_uprobe_match(const char *system, const char *event,
> > {
> > struct trace_uprobe *tu = to_trace_uprobe(ev);
> >
> > + if (!is_trace_uprobe(ev))
> > + return false;
> > +
> > return (event[0] == '\0' ||
> > strcmp(trace_probe_name(&tu->tp), event) == 0) &&
> > (!system || strcmp(trace_probe_group_name(&tu->tp), system) == 0) &&
> > --
> > 2.7.4
--
Masami Hiramatsu (Google) <mhiramat@...nel.org>
Powered by blists - more mailing lists