* [PATCH] tracing: Fix bad hist from corrupting named_triggers list
@ 2025-02-25 17:53 Steven Rostedt
2025-02-26 15:58 ` Tomas Glozar
2025-02-27 19:53 ` Tom Zanussi
0 siblings, 2 replies; 5+ messages in thread
From: Steven Rostedt @ 2025-02-25 17:53 UTC (permalink / raw)
To: LKML, Linux Trace Kernel
Cc: Masami Hiramatsu, Mathieu Desnoyers, Tomas Glozar, Tom Zanussi
From: Steven Rostedt <rostedt@goodmis.org>
The following commands causes a crash:
~# cd /sys/kernel/tracing/events/rcu/rcu_callback
~# echo 'hist:name=bad:keys=common_pid:onmax(bogus).save(common_pid)' > trigger
bash: echo: write error: Invalid argument
~# echo 'hist:name=bad:keys=common_pid' > trigger
Because the following occurs:
event_trigger_write() {
trigger_process_regex() {
event_hist_trigger_parse() {
data = event_trigger_alloc(..);
event_trigger_register(.., data) {
cmd_ops->reg(.., data, ..) [hist_register_trigger()] {
data->ops->init() [event_hist_trigger_init()] {
save_named_trigger(name, data) {
list_add(&data->named_list, &named_triggers);
}
}
}
}
ret = create_actions(); (return -EINVAL)
if (ret)
goto out_unreg;
[..]
ret = hist_trigger_enable(data, ...) {
list_add_tail_rcu(&data->list, &file->triggers); <<<---- SKIPPED!!! (this is important!)
[..]
out_unreg:
event_hist_unregister(.., data) {
cmd_ops->unreg(.., data, ..) [hist_unregister_trigger()] {
list_for_each_entry(iter, &file->triggers, list) {
if (!hist_trigger_match(data, iter, named_data, false)) <- never matches
continue;
[..]
test = iter;
}
if (test && test->ops->free) <<<-- test is NULL
test->ops->free(test) [event_hist_trigger_free()] {
[..]
if (data->name)
del_named_trigger(data) {
list_del(&data->named_list); <<<<-- NEVER gets removed!
}
}
}
}
[..]
kfree(data); <<<-- frees item but it is still on list
The next time a hist with name is registered, it causes an u-a-f bug and
the kernel can crash.
Move the code around such that if event_trigger_register() succeeds, the
next thing called is hist_trigger_enable() which adds it to the list.
A bunch of actions is called if get_named_trigger_data() returns false.
But that doesn't need to be called after event_trigger_register(), so it
can be moved up, allowing event_trigger_register() to be called just
before hist_trigger_enable() keeping them together and allowing the
file->triggers to be properly populated.
Cc: stable@vger.kernel.org
Fixes: 067fe038e70f6 ("tracing: Add variable reference handling to hist triggers")
Reported-by: Tomas Glozar <tglozar@redhat.com>
Closes: https://lore.kernel.org/all/CAP4=nvTsxjckSBTz=Oe_UYh8keD9_sZC4i++4h72mJLic4_W4A@mail.gmail.com/
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
kernel/trace/trace_events_hist.c | 31 ++++++++++++++++---------------
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 261163b00137..c32adc372808 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -6724,27 +6724,28 @@ static int event_hist_trigger_parse(struct event_command *cmd_ops,
if (existing_hist_update_only(glob, trigger_data, file))
goto out_free;
- ret = event_trigger_register(cmd_ops, file, glob, trigger_data);
- if (ret < 0)
- goto out_free;
+ if (!get_named_trigger_data(trigger_data)) {
- if (get_named_trigger_data(trigger_data))
- goto enable;
+ ret = create_actions(hist_data);
+ if (ret)
+ goto out_free;
- ret = create_actions(hist_data);
- if (ret)
- goto out_unreg;
+ if (has_hist_vars(hist_data) || hist_data->n_var_refs) {
+ ret = save_hist_vars(hist_data);
+ if (ret)
+ goto out_free;
+ }
- if (has_hist_vars(hist_data) || hist_data->n_var_refs) {
- ret = save_hist_vars(hist_data);
+ ret = tracing_map_init(hist_data->map);
if (ret)
- goto out_unreg;
+ goto out_free;
}
- ret = tracing_map_init(hist_data->map);
- if (ret)
- goto out_unreg;
-enable:
+ ret = event_trigger_register(cmd_ops, file, glob, trigger_data);
+ if (ret < 0)
+ goto out_free;
+
+
ret = hist_trigger_enable(trigger_data, file);
if (ret)
goto out_unreg;
--
2.47.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] tracing: Fix bad hist from corrupting named_triggers list
2025-02-25 17:53 [PATCH] tracing: Fix bad hist from corrupting named_triggers list Steven Rostedt
@ 2025-02-26 15:58 ` Tomas Glozar
2025-02-27 19:53 ` Tom Zanussi
1 sibling, 0 replies; 5+ messages in thread
From: Tomas Glozar @ 2025-02-26 15:58 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Linux Trace Kernel, Masami Hiramatsu, Mathieu Desnoyers,
Tom Zanussi
út 25. 2. 2025 v 18:53 odesílatel Steven Rostedt <rostedt@goodmis.org> napsal:
>
> From: Steven Rostedt <rostedt@goodmis.org>
>
> The following commands causes a crash:
>
> ~# cd /sys/kernel/tracing/events/rcu/rcu_callback
> ~# echo 'hist:name=bad:keys=common_pid:onmax(bogus).save(common_pid)' > trigger
> bash: echo: write error: Invalid argument
> ~# echo 'hist:name=bad:keys=common_pid' > trigger
>
> Because the following occurs:
>
> event_trigger_write() {
> trigger_process_regex() {
> event_hist_trigger_parse() {
>
> data = event_trigger_alloc(..);
>
> event_trigger_register(.., data) {
> cmd_ops->reg(.., data, ..) [hist_register_trigger()] {
> data->ops->init() [event_hist_trigger_init()] {
> save_named_trigger(name, data) {
> list_add(&data->named_list, &named_triggers);
> }
> }
> }
> }
>
> ret = create_actions(); (return -EINVAL)
> if (ret)
> goto out_unreg;
> [..]
> ret = hist_trigger_enable(data, ...) {
> list_add_tail_rcu(&data->list, &file->triggers); <<<---- SKIPPED!!! (this is important!)
> [..]
> out_unreg:
> event_hist_unregister(.., data) {
> cmd_ops->unreg(.., data, ..) [hist_unregister_trigger()] {
> list_for_each_entry(iter, &file->triggers, list) {
> if (!hist_trigger_match(data, iter, named_data, false)) <- never matches
> continue;
> [..]
> test = iter;
> }
> if (test && test->ops->free) <<<-- test is NULL
>
> test->ops->free(test) [event_hist_trigger_free()] {
> [..]
> if (data->name)
> del_named_trigger(data) {
> list_del(&data->named_list); <<<<-- NEVER gets removed!
> }
> }
> }
> }
>
> [..]
> kfree(data); <<<-- frees item but it is still on list
>
> The next time a hist with name is registered, it causes an u-a-f bug and
> the kernel can crash.
>
> Move the code around such that if event_trigger_register() succeeds, the
> next thing called is hist_trigger_enable() which adds it to the list.
>
> A bunch of actions is called if get_named_trigger_data() returns false.
> But that doesn't need to be called after event_trigger_register(), so it
> can be moved up, allowing event_trigger_register() to be called just
> before hist_trigger_enable() keeping them together and allowing the
> file->triggers to be properly populated.
>
> Cc: stable@vger.kernel.org
> Fixes: 067fe038e70f6 ("tracing: Add variable reference handling to hist triggers")
> Reported-by: Tomas Glozar <tglozar@redhat.com>
> Closes: https://lore.kernel.org/all/CAP4=nvTsxjckSBTz=Oe_UYh8keD9_sZC4i++4h72mJLic4_W4A@mail.gmail.com/
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
> kernel/trace/trace_events_hist.c | 31 ++++++++++++++++---------------
> 1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 261163b00137..c32adc372808 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -6724,27 +6724,28 @@ static int event_hist_trigger_parse(struct event_command *cmd_ops,
> if (existing_hist_update_only(glob, trigger_data, file))
> goto out_free;
>
> - ret = event_trigger_register(cmd_ops, file, glob, trigger_data);
> - if (ret < 0)
> - goto out_free;
> + if (!get_named_trigger_data(trigger_data)) {
>
> - if (get_named_trigger_data(trigger_data))
> - goto enable;
> + ret = create_actions(hist_data);
> + if (ret)
> + goto out_free;
>
> - ret = create_actions(hist_data);
> - if (ret)
> - goto out_unreg;
> + if (has_hist_vars(hist_data) || hist_data->n_var_refs) {
> + ret = save_hist_vars(hist_data);
> + if (ret)
> + goto out_free;
> + }
>
> - if (has_hist_vars(hist_data) || hist_data->n_var_refs) {
> - ret = save_hist_vars(hist_data);
> + ret = tracing_map_init(hist_data->map);
> if (ret)
> - goto out_unreg;
> + goto out_free;
> }
>
> - ret = tracing_map_init(hist_data->map);
> - if (ret)
> - goto out_unreg;
> -enable:
> + ret = event_trigger_register(cmd_ops, file, glob, trigger_data);
> + if (ret < 0)
> + goto out_free;
> +
> +
> ret = hist_trigger_enable(trigger_data, file);
> if (ret)
> goto out_unreg;
> --
> 2.47.2
>
Applied this on 6.14.0-rc4 and the problem is gone.
Tested-by: Tomas Glozar <tglozar@redhat.com>
Tomas
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tracing: Fix bad hist from corrupting named_triggers list
2025-02-25 17:53 [PATCH] tracing: Fix bad hist from corrupting named_triggers list Steven Rostedt
2025-02-26 15:58 ` Tomas Glozar
@ 2025-02-27 19:53 ` Tom Zanussi
2025-02-27 21:37 ` Steven Rostedt
1 sibling, 1 reply; 5+ messages in thread
From: Tom Zanussi @ 2025-02-27 19:53 UTC (permalink / raw)
To: Steven Rostedt, LKML, Linux Trace Kernel
Cc: Masami Hiramatsu, Mathieu Desnoyers, Tomas Glozar
Hi Steve,
On Tue, 2025-02-25 at 12:53 -0500, Steven Rostedt wrote:
> From: Steven Rostedt <rostedt@goodmis.org>
>
> The following commands causes a crash:
>
> ~# cd /sys/kernel/tracing/events/rcu/rcu_callback
> ~# echo 'hist:name=bad:keys=common_pid:onmax(bogus).save(common_pid)' > trigger
> bash: echo: write error: Invalid argument
> ~# echo 'hist:name=bad:keys=common_pid' > trigger
>
> Because the following occurs:
>
> event_trigger_write() {
> trigger_process_regex() {
> event_hist_trigger_parse() {
>
> data = event_trigger_alloc(..);
>
> event_trigger_register(.., data) {
> cmd_ops->reg(.., data, ..) [hist_register_trigger()] {
> data->ops->init() [event_hist_trigger_init()] {
> save_named_trigger(name, data) {
> list_add(&data->named_list, &named_triggers);
> }
> }
> }
> }
>
> ret = create_actions(); (return -EINVAL)
> if (ret)
> goto out_unreg;
> [..]
> ret = hist_trigger_enable(data, ...) {
> list_add_tail_rcu(&data->list, &file->triggers); <<<---- SKIPPED!!! (this is important!)
> [..]
> out_unreg:
> event_hist_unregister(.., data) {
> cmd_ops->unreg(.., data, ..) [hist_unregister_trigger()] {
> list_for_each_entry(iter, &file->triggers, list) {
> if (!hist_trigger_match(data, iter, named_data, false)) <- never matches
> continue;
> [..]
> test = iter;
> }
> if (test && test->ops->free) <<<-- test is NULL
>
> test->ops->free(test) [event_hist_trigger_free()] {
> [..]
> if (data->name)
> del_named_trigger(data) {
> list_del(&data->named_list); <<<<-- NEVER gets removed!
> }
> }
> }
> }
>
> [..]
> kfree(data); <<<-- frees item but it is still on list
>
> The next time a hist with name is registered, it causes an u-a-f bug and
> the kernel can crash.
>
> Move the code around such that if event_trigger_register() succeeds, the
> next thing called is hist_trigger_enable() which adds it to the list.
>
> A bunch of actions is called if get_named_trigger_data() returns false.
> But that doesn't need to be called after event_trigger_register(), so it
> can be moved up, allowing event_trigger_register() to be called just
> before hist_trigger_enable() keeping them together and allowing the
> file->triggers to be properly populated.
>
> Cc: stable@vger.kernel.org
> Fixes: 067fe038e70f6 ("tracing: Add variable reference handling to hist triggers")
> Reported-by: Tomas Glozar <tglozar@redhat.com>
> Closes: https://lore.kernel.org/all/CAP4=nvTsxjckSBTz=Oe_UYh8keD9_sZC4i++4h72mJLic4_W4A@mail.gmail.com/
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Looks like a good fix, and is cleaner without the goto as well. Small typo below...
Reviewed-by: Tom Zanussi <zanussi@kernel.org>
> ---
> kernel/trace/trace_events_hist.c | 31 ++++++++++++++++---------------
> 1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 261163b00137..c32adc372808 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -6724,27 +6724,28 @@ static int event_hist_trigger_parse(struct event_command *cmd_ops,
> if (existing_hist_update_only(glob, trigger_data, file))
> goto out_free;
>
> - ret = event_trigger_register(cmd_ops, file, glob, trigger_data);
> - if (ret < 0)
> - goto out_free;
> + if (!get_named_trigger_data(trigger_data)) {
>
> - if (get_named_trigger_data(trigger_data))
> - goto enable;
> + ret = create_actions(hist_data);
> + if (ret)
> + goto out_free;
>
> - ret = create_actions(hist_data);
> - if (ret)
> - goto out_unreg;
> + if (has_hist_vars(hist_data) || hist_data->n_var_refs) {
> + ret = save_hist_vars(hist_data);
> + if (ret)
> + goto out_free;
> + }
>
> - if (has_hist_vars(hist_data) || hist_data->n_var_refs) {
> - ret = save_hist_vars(hist_data);
> + ret = tracing_map_init(hist_data->map);
> if (ret)
> - goto out_unreg;
> + goto out_free;
> }
>
> - ret = tracing_map_init(hist_data->map);
> - if (ret)
> - goto out_unreg;
> -enable:
> + ret = event_trigger_register(cmd_ops, file, glob, trigger_data);
> + if (ret < 0)
> + goto out_free;
> +
> +
Extra space added here.
> ret = hist_trigger_enable(trigger_data, file);
> if (ret)
> goto out_unreg;
Thanks,
Tom
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tracing: Fix bad hist from corrupting named_triggers list
2025-02-27 19:53 ` Tom Zanussi
@ 2025-02-27 21:37 ` Steven Rostedt
0 siblings, 0 replies; 5+ messages in thread
From: Steven Rostedt @ 2025-02-27 21:37 UTC (permalink / raw)
To: Tom Zanussi
Cc: LKML, Linux Trace Kernel, Masami Hiramatsu, Mathieu Desnoyers,
Tomas Glozar
On Thu, 27 Feb 2025 13:53:27 -0600
Tom Zanussi <zanussi@kernel.org> wrote:
> > -enable:
> > + ret = event_trigger_register(cmd_ops, file, glob, trigger_data);
> > + if (ret < 0)
> > + goto out_free;
> > +
> > +
>
> Extra space added here.
Bah, I'll remove that when adding your tested by.
I've already ran it through my entire test suite, but I'm going to break my
rule and just build and boot to test removing that line, and not run the
entire test suite.
Now, if there's another bug that shows up, it will get run through that.
Thanks,
-- Steve
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] tracing: Fix bad hist from corrupting named_triggers list
@ 2025-02-27 21:39 Steven Rostedt
0 siblings, 0 replies; 5+ messages in thread
From: Steven Rostedt @ 2025-02-27 21:39 UTC (permalink / raw)
To: LKML, Linux Trace Kernel
Cc: Masami Hiramatsu, Mathieu Desnoyers, Tom Zanussi, Tomas Glozar
From: Steven Rostedt <rostedt@goodmis.org>
The following commands causes a crash:
~# cd /sys/kernel/tracing/events/rcu/rcu_callback
~# echo 'hist:name=bad:keys=common_pid:onmax(bogus).save(common_pid)' > trigger
bash: echo: write error: Invalid argument
~# echo 'hist:name=bad:keys=common_pid' > trigger
Because the following occurs:
event_trigger_write() {
trigger_process_regex() {
event_hist_trigger_parse() {
data = event_trigger_alloc(..);
event_trigger_register(.., data) {
cmd_ops->reg(.., data, ..) [hist_register_trigger()] {
data->ops->init() [event_hist_trigger_init()] {
save_named_trigger(name, data) {
list_add(&data->named_list, &named_triggers);
}
}
}
}
ret = create_actions(); (return -EINVAL)
if (ret)
goto out_unreg;
[..]
ret = hist_trigger_enable(data, ...) {
list_add_tail_rcu(&data->list, &file->triggers); <<<---- SKIPPED!!! (this is important!)
[..]
out_unreg:
event_hist_unregister(.., data) {
cmd_ops->unreg(.., data, ..) [hist_unregister_trigger()] {
list_for_each_entry(iter, &file->triggers, list) {
if (!hist_trigger_match(data, iter, named_data, false)) <- never matches
continue;
[..]
test = iter;
}
if (test && test->ops->free) <<<-- test is NULL
test->ops->free(test) [event_hist_trigger_free()] {
[..]
if (data->name)
del_named_trigger(data) {
list_del(&data->named_list); <<<<-- NEVER gets removed!
}
}
}
}
[..]
kfree(data); <<<-- frees item but it is still on list
The next time a hist with name is registered, it causes an u-a-f bug and
the kernel can crash.
Move the code around such that if event_trigger_register() succeeds, the
next thing called is hist_trigger_enable() which adds it to the list.
A bunch of actions is called if get_named_trigger_data() returns false.
But that doesn't need to be called after event_trigger_register(), so it
can be moved up, allowing event_trigger_register() to be called just
before hist_trigger_enable() keeping them together and allowing the
file->triggers to be properly populated.
Cc: stable@vger.kernel.org
Fixes: 067fe038e70f6 ("tracing: Add variable reference handling to hist triggers")
Reported-by: Tomas Glozar <tglozar@redhat.com>
Tested-by: Tomas Glozar <tglozar@redhat.com>
Reviewed-by: Tom Zanussi <zanussi@kernel.org>
Closes: https://lore.kernel.org/all/CAP4=nvTsxjckSBTz=Oe_UYh8keD9_sZC4i++4h72mJLic4_W4A@mail.gmail.com/
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
Changes since v1: https://lore.kernel.org/20250225125356.29236cd1@gandalf.local.home
- Remove extra blank line.
kernel/trace/trace_events_hist.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 261163b00137..ad7419e24055 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -6724,27 +6724,27 @@ static int event_hist_trigger_parse(struct event_command *cmd_ops,
if (existing_hist_update_only(glob, trigger_data, file))
goto out_free;
- ret = event_trigger_register(cmd_ops, file, glob, trigger_data);
- if (ret < 0)
- goto out_free;
+ if (!get_named_trigger_data(trigger_data)) {
- if (get_named_trigger_data(trigger_data))
- goto enable;
+ ret = create_actions(hist_data);
+ if (ret)
+ goto out_free;
- ret = create_actions(hist_data);
- if (ret)
- goto out_unreg;
+ if (has_hist_vars(hist_data) || hist_data->n_var_refs) {
+ ret = save_hist_vars(hist_data);
+ if (ret)
+ goto out_free;
+ }
- if (has_hist_vars(hist_data) || hist_data->n_var_refs) {
- ret = save_hist_vars(hist_data);
+ ret = tracing_map_init(hist_data->map);
if (ret)
- goto out_unreg;
+ goto out_free;
}
- ret = tracing_map_init(hist_data->map);
- if (ret)
- goto out_unreg;
-enable:
+ ret = event_trigger_register(cmd_ops, file, glob, trigger_data);
+ if (ret < 0)
+ goto out_free;
+
ret = hist_trigger_enable(trigger_data, file);
if (ret)
goto out_unreg;
--
2.47.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-02-27 21:39 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-25 17:53 [PATCH] tracing: Fix bad hist from corrupting named_triggers list Steven Rostedt
2025-02-26 15:58 ` Tomas Glozar
2025-02-27 19:53 ` Tom Zanussi
2025-02-27 21:37 ` Steven Rostedt
-- strict thread matches above, loose matches on Subject: below --
2025-02-27 21:39 Steven Rostedt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox