* [PATCH 0/2] tracing: Fix adding some modifiers to histogram values
@ 2023-03-02 1:00 Steven Rostedt
2023-03-02 1:00 ` [PATCH 1/2] tracing: Do not let histogram values have some modifiers Steven Rostedt
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Steven Rostedt @ 2023-03-02 1:00 UTC (permalink / raw)
To: linux-kernel, linux-trace-kernel
Cc: Masami Hiramatsu, Mark Rutland, Andrew Morton
Mark Rutland reported to me on IRC that he accidentally added the
".buckets=8" modifier to hitcount. This should not be allowed, but it
did not error. Worse yet, when reading the hist file, it would crash
as there was a NULL pointer dereference due to the values not having
fields assigned to them.
The first fix is to make sure that histogram values do not get assigned
modifiers that they can't use.
The the second patch is to not crash if a NULL pointer is passed to
hist_field_name() (which is what happens if you allow some of these
modifiers to be used by values).
Steven Rostedt (Google) (2):
tracing: Do not let histogram values have some modifiers
tracing: Check field value in hist_field_name()
----
kernel/trace/trace_events_hist.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] tracing: Do not let histogram values have some modifiers
2023-03-02 1:00 [PATCH 0/2] tracing: Fix adding some modifiers to histogram values Steven Rostedt
@ 2023-03-02 1:00 ` Steven Rostedt
2023-03-02 14:24 ` Mark Rutland
2023-03-02 1:00 ` [PATCH 2/2] tracing: Check field value in hist_field_name() Steven Rostedt
2023-03-02 13:18 ` [PATCH 0/2] tracing: Fix adding some modifiers to histogram values Steven Rostedt
2 siblings, 1 reply; 6+ messages in thread
From: Steven Rostedt @ 2023-03-02 1:00 UTC (permalink / raw)
To: linux-kernel, linux-trace-kernel
Cc: Masami Hiramatsu, Mark Rutland, Andrew Morton, stable
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
Histogram values can not be strings, stacktraces, graphs, symbols,
syscalls, or grouped in buckets or log. Give an error if a value is set to
do so.
Note, the histogram code was not prepared to handle these modifiers for
histograms and caused a bug.
Mark Rutland reported:
# echo 'p:copy_to_user __arch_copy_to_user n=$arg2' >> /sys/kernel/tracing/kprobe_events
# echo 'hist:keys=n:vals=hitcount.buckets=8:sort=hitcount' > /sys/kernel/tracing/events/kprobes/copy_to_user/trigger
# cat /sys/kernel/tracing/events/kprobes/copy_to_user/hist
[ 143.694628] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[ 143.695190] Mem abort info:
[ 143.695362] ESR = 0x0000000096000004
[ 143.695604] EC = 0x25: DABT (current EL), IL = 32 bits
[ 143.695889] SET = 0, FnV = 0
[ 143.696077] EA = 0, S1PTW = 0
[ 143.696302] FSC = 0x04: level 0 translation fault
[ 143.702381] Data abort info:
[ 143.702614] ISV = 0, ISS = 0x00000004
[ 143.702832] CM = 0, WnR = 0
[ 143.703087] user pgtable: 4k pages, 48-bit VAs, pgdp=00000000448f9000
[ 143.703407] [0000000000000000] pgd=0000000000000000, p4d=0000000000000000
[ 143.704137] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
[ 143.704714] Modules linked in:
[ 143.705273] CPU: 0 PID: 133 Comm: cat Not tainted 6.2.0-00003-g6fc512c10a7c #3
[ 143.706138] Hardware name: linux,dummy-virt (DT)
[ 143.706723] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 143.707120] pc : hist_field_name.part.0+0x14/0x140
[ 143.707504] lr : hist_field_name.part.0+0x104/0x140
[ 143.707774] sp : ffff800008333a30
[ 143.707952] x29: ffff800008333a30 x28: 0000000000000001 x27: 0000000000400cc0
[ 143.708429] x26: ffffd7a653b20260 x25: 0000000000000000 x24: ffff10d303ee5800
[ 143.708776] x23: ffffd7a6539b27b0 x22: ffff10d303fb8c00 x21: 0000000000000001
[ 143.709127] x20: ffff10d303ec2000 x19: 0000000000000000 x18: 0000000000000000
[ 143.709478] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
[ 143.709824] x14: 0000000000000000 x13: 203a6f666e692072 x12: 6567676972742023
[ 143.710179] x11: 0a230a6d6172676f x10: 000000000000002c x9 : ffffd7a6521e018c
[ 143.710584] x8 : 000000000000002c x7 : 7f7f7f7f7f7f7f7f x6 : 000000000000002c
[ 143.710915] x5 : ffff10d303b0103e x4 : ffffd7a653b20261 x3 : 000000000000003d
[ 143.711239] x2 : 0000000000020001 x1 : 0000000000000001 x0 : 0000000000000000
[ 143.711746] Call trace:
[ 143.712115] hist_field_name.part.0+0x14/0x140
[ 143.712642] hist_field_name.part.0+0x104/0x140
[ 143.712925] hist_field_print+0x28/0x140
[ 143.713125] event_hist_trigger_print+0x174/0x4d0
[ 143.713348] hist_show+0xf8/0x980
[ 143.713521] seq_read_iter+0x1bc/0x4b0
[ 143.713711] seq_read+0x8c/0xc4
[ 143.713876] vfs_read+0xc8/0x2a4
[ 143.714043] ksys_read+0x70/0xfc
[ 143.714218] __arm64_sys_read+0x24/0x30
[ 143.714400] invoke_syscall+0x50/0x120
[ 143.714587] el0_svc_common.constprop.0+0x4c/0x100
[ 143.714807] do_el0_svc+0x44/0xd0
[ 143.714970] el0_svc+0x2c/0x84
[ 143.715134] el0t_64_sync_handler+0xbc/0x140
[ 143.715334] el0t_64_sync+0x190/0x194
[ 143.715742] Code: a9bd7bfd 910003fd a90153f3 aa0003f3 (f9400000)
[ 143.716510] ---[ end trace 0000000000000000 ]---
Segmentation fault
Cc: stable@vger.kernel.org
Fixes: c6afad49d127f ("tracing: Add hist trigger 'sym' and 'sym-offset' modifiers")
Reported-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
kernel/trace/trace_events_hist.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 89877a18f933..6e8ab726a7b5 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -4235,6 +4235,15 @@ static int __create_val_field(struct hist_trigger_data *hist_data,
goto out;
}
+ /* Some types cannot be a value */
+ if (hist_field->flags & (HIST_FIELD_FL_GRAPH | HIST_FIELD_FL_PERCENT |
+ HIST_FIELD_FL_BUCKET | HIST_FIELD_FL_LOG2 |
+ HIST_FIELD_FL_SYM | HIST_FIELD_FL_SYM_OFFSET |
+ HIST_FIELD_FL_SYSCALL | HIST_FIELD_FL_STACKTRACE)) {
+ hist_err(file->tr, HIST_ERR_BAD_FIELD_MODIFIER, errpos(field_str));
+ ret = -EINVAL;
+ }
+
hist_data->fields[val_idx] = hist_field;
++hist_data->n_vals;
--
2.39.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] tracing: Check field value in hist_field_name()
2023-03-02 1:00 [PATCH 0/2] tracing: Fix adding some modifiers to histogram values Steven Rostedt
2023-03-02 1:00 ` [PATCH 1/2] tracing: Do not let histogram values have some modifiers Steven Rostedt
@ 2023-03-02 1:00 ` Steven Rostedt
2023-03-02 14:18 ` Mark Rutland
2023-03-02 13:18 ` [PATCH 0/2] tracing: Fix adding some modifiers to histogram values Steven Rostedt
2 siblings, 1 reply; 6+ messages in thread
From: Steven Rostedt @ 2023-03-02 1:00 UTC (permalink / raw)
To: linux-kernel, linux-trace-kernel
Cc: Masami Hiramatsu, Mark Rutland, Andrew Morton, stable
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
The function hist_field_name() cannot handle being passed a NULL field
parameter. It should never be NULL, but due to a previous bug, NULL was
passed to the function and the kernel crashed due to a NULL dereference.
Mark Rutland reported this to me on IRC.
The bug was fixed, but to prevent future bugs from crashing the kernel,
check the field and add a WARN_ON() if it is NULL.
Cc: stable@vger.kernel.org
Reported-by: Mark Rutland <mark.rutland@arm.com>
Fixes: c6afad49d127f ("tracing: Add hist trigger 'sym' and 'sym-offset' modifiers")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
kernel/trace/trace_events_hist.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 6e8ab726a7b5..486cca3c2b75 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1331,6 +1331,9 @@ static const char *hist_field_name(struct hist_field *field,
{
const char *field_name = "";
+ if (WARN_ON_ONCE(!field))
+ return field_name;
+
if (level > 1)
return field_name;
--
2.39.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] tracing: Fix adding some modifiers to histogram values
2023-03-02 1:00 [PATCH 0/2] tracing: Fix adding some modifiers to histogram values Steven Rostedt
2023-03-02 1:00 ` [PATCH 1/2] tracing: Do not let histogram values have some modifiers Steven Rostedt
2023-03-02 1:00 ` [PATCH 2/2] tracing: Check field value in hist_field_name() Steven Rostedt
@ 2023-03-02 13:18 ` Steven Rostedt
2 siblings, 0 replies; 6+ messages in thread
From: Steven Rostedt @ 2023-03-02 13:18 UTC (permalink / raw)
To: linux-kernel, linux-trace-kernel
Cc: Masami Hiramatsu, Mark Rutland, Andrew Morton, Tom Zanussi
I forgot to add Tom Zanussi on this series.
-- Steve
On Wed, 01 Mar 2023 20:00:51 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> Mark Rutland reported to me on IRC that he accidentally added the
> ".buckets=8" modifier to hitcount. This should not be allowed, but it
> did not error. Worse yet, when reading the hist file, it would crash
> as there was a NULL pointer dereference due to the values not having
> fields assigned to them.
>
> The first fix is to make sure that histogram values do not get assigned
> modifiers that they can't use.
>
> The the second patch is to not crash if a NULL pointer is passed to
> hist_field_name() (which is what happens if you allow some of these
> modifiers to be used by values).
>
> Steven Rostedt (Google) (2):
> tracing: Do not let histogram values have some modifiers
> tracing: Check field value in hist_field_name()
>
> ----
> kernel/trace/trace_events_hist.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] tracing: Check field value in hist_field_name()
2023-03-02 1:00 ` [PATCH 2/2] tracing: Check field value in hist_field_name() Steven Rostedt
@ 2023-03-02 14:18 ` Mark Rutland
0 siblings, 0 replies; 6+ messages in thread
From: Mark Rutland @ 2023-03-02 14:18 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, linux-trace-kernel, Masami Hiramatsu, Andrew Morton,
stable
On Wed, Mar 01, 2023 at 08:00:53PM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
>
> The function hist_field_name() cannot handle being passed a NULL field
> parameter. It should never be NULL, but due to a previous bug, NULL was
> passed to the function and the kernel crashed due to a NULL dereference.
> Mark Rutland reported this to me on IRC.
>
> The bug was fixed, but to prevent future bugs from crashing the kernel,
> check the field and add a WARN_ON() if it is NULL.
>
> Cc: stable@vger.kernel.org
> Reported-by: Mark Rutland <mark.rutland@arm.com>
> Fixes: c6afad49d127f ("tracing: Add hist trigger 'sym' and 'sym-offset' modifiers")
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Tested-by: Mark Rutland <mark.rutland@arm.com>
I gave this patch a spin on its own (without the prior patch), and it behaves
as expected. When deliberately triggering the aforementioned bug I hit the
WARN_ON_ONCE() without crashing the kernel:
| # echo 'p:copy_to_user __arch_copy_to_user n=$arg2' >> /sys/kernel/tracing/kprobe_events
| # echo 'hist:keys=n:vals=hitcount.buckets=8:sort=hitcount' > /sys/kernel/tracing/events/kprobes/copy_to_user/trigger
| # cat /sys/kernel/tracing/events/kprobes/copy_to_user/hist
| ------------[ cut here ]------------
| WARNING: CPU: 0 PID: 133 at kernel/trace/trace_events_hist.c:1337 hist_field_name+0x94/0x144
| Modules linked in:
| CPU: 0 PID: 133 Comm: cat Not tainted 6.2.0-00003-g785bb684c534 #2
| Hardware name: linux,dummy-virt (DT)
| pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
| pc : hist_field_name+0x94/0x144
| lr : hist_field_name+0xbc/0x144
| sp : ffff800008343a60
| x29: ffff800008343a60 x28: 0000000000000001 x27: 0000000000400cc0
| x26: ffffaed00953fcd0 x25: 0000000000000000 x24: ffff65c743e8bf00
| x23: ffffaed0093d2488 x22: ffff65c743fadc00 x21: 0000000000000001
| x20: ffff65c743ec1000 x19: ffff65c743fadc00 x18: 0000000000000000
| x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
| x14: 0000000000000000 x13: 203a6f666e692072 x12: 6567676972742023
| x11: 0a230a6d6172676f x10: 000000000000002c x9 : ffffaed007be1fcc
| x8 : 000000000000002c x7 : 7f7f7f7f7f7f7f7f x6 : 000000000000002c
| x5 : ffff65c743b0103e x4 : ffffaed00953fcd1 x3 : 000000000000003d
| x2 : 0000000000020001 x1 : 0000000000000001 x0 : 0000000000000000
| Call trace:
| hist_field_name+0x94/0x144
| hist_field_print+0x28/0x14c
| event_hist_trigger_print+0x174/0x4d0
| hist_show+0xf8/0x980
| seq_read_iter+0x1bc/0x4b0
| seq_read+0x8c/0xc4
| vfs_read+0xc8/0x2a4
| ksys_read+0x70/0xfc
| __arm64_sys_read+0x24/0x30
| invoke_syscall+0x50/0x120
| el0_svc_common.constprop.0+0x4c/0x100
| do_el0_svc+0x44/0xd0
| el0_svc+0x2c/0x84
| el0t_64_sync_handler+0xbc/0x140
| el0t_64_sync+0x190/0x194
| ---[ end trace 0000000000000000 ]---
| # event histogram
| #
| # trigger info: hist:keys=n:vals=hitcount,.buckets=8:sort=hitcount:size=2048 [active]
| #
|
| { n: 18446574505247538232 } hitcount: 1 : 1
| { n: 18446574505249480120 } hitcount: 1 : 1
| { n: 18446574505255937966 } hitcount: 1 : 1
| { n: 18446574505234423224 } hitcount: 1 : 1
[...]
| Totals:
| Hits: 371
| Entries: 263
| Dropped: 0
Note: the 'n' values are large because '$arg2' is actually the 'from' pointer
here, another mistake of mine (I had meant to capture '$arg3').
Thanks,
Mark.
> ---
> kernel/trace/trace_events_hist.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 6e8ab726a7b5..486cca3c2b75 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -1331,6 +1331,9 @@ static const char *hist_field_name(struct hist_field *field,
> {
> const char *field_name = "";
>
> + if (WARN_ON_ONCE(!field))
> + return field_name;
> +
> if (level > 1)
> return field_name;
>
> --
> 2.39.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] tracing: Do not let histogram values have some modifiers
2023-03-02 1:00 ` [PATCH 1/2] tracing: Do not let histogram values have some modifiers Steven Rostedt
@ 2023-03-02 14:24 ` Mark Rutland
0 siblings, 0 replies; 6+ messages in thread
From: Mark Rutland @ 2023-03-02 14:24 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, linux-trace-kernel, Masami Hiramatsu, Andrew Morton,
stable
On Wed, Mar 01, 2023 at 08:00:52PM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
>
> Histogram values can not be strings, stacktraces, graphs, symbols,
> syscalls, or grouped in buckets or log. Give an error if a value is set to
> do so.
>
> Note, the histogram code was not prepared to handle these modifiers for
> histograms and caused a bug.
> Cc: stable@vger.kernel.org
> Fixes: c6afad49d127f ("tracing: Add hist trigger 'sym' and 'sym-offset' modifiers")
> Reported-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Tested-by: Mark Rutland <mark.rutland@arm.com>
I gave this a spin, and I see that the buckets modifier gerts rejected for
hitcount, but is usable for other values as it should be:
| # echo 'p:copy_to_user __arch_copy_to_user n=$arg3' >> /sys/kernel/tracing/kprobe_events
| # echo 'hist:keys=n:vals=hitcount.buckets=8:sort=hitcount' > /sys/kernel/tracing/events/kprobes/copy_to_user/trigger
| sh: write error: Invalid argument
| # echo 'hist:keys=n.buckets=8:vals=hitcount:sort=hitcount' > /sys/kernel/tracing/events/kprobes/copy_to_user/trigger
| # cat /sys/kernel/tracing/events/kprobes/copy_to_user/hist
| # event histogram
| #
| # trigger info: hist:keys=n.buckets=8:vals=hitcount:sort=hitcount:size=2048 [active]
| #
|
| { n: ~ 336-343 } hitcount: 1
| { n: ~ 16-23 } hitcount: 2
| { n: ~ 32-39 } hitcount: 2
| { n: ~ 832-839 } hitcount: 3
| { n: ~ 8-15 } hitcount: 3
| { n: ~ 128-135 } hitcount: 5
| { n: ~ 0-7 } hitcount: 57
|
| Totals:
| Hits: 73
| Entries: 7
| Dropped: 0
Thanks,
Mark.
> ---
> kernel/trace/trace_events_hist.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 89877a18f933..6e8ab726a7b5 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -4235,6 +4235,15 @@ static int __create_val_field(struct hist_trigger_data *hist_data,
> goto out;
> }
>
> + /* Some types cannot be a value */
> + if (hist_field->flags & (HIST_FIELD_FL_GRAPH | HIST_FIELD_FL_PERCENT |
> + HIST_FIELD_FL_BUCKET | HIST_FIELD_FL_LOG2 |
> + HIST_FIELD_FL_SYM | HIST_FIELD_FL_SYM_OFFSET |
> + HIST_FIELD_FL_SYSCALL | HIST_FIELD_FL_STACKTRACE)) {
> + hist_err(file->tr, HIST_ERR_BAD_FIELD_MODIFIER, errpos(field_str));
> + ret = -EINVAL;
> + }
> +
> hist_data->fields[val_idx] = hist_field;
>
> ++hist_data->n_vals;
> --
> 2.39.1
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-02 14:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-02 1:00 [PATCH 0/2] tracing: Fix adding some modifiers to histogram values Steven Rostedt
2023-03-02 1:00 ` [PATCH 1/2] tracing: Do not let histogram values have some modifiers Steven Rostedt
2023-03-02 14:24 ` Mark Rutland
2023-03-02 1:00 ` [PATCH 2/2] tracing: Check field value in hist_field_name() Steven Rostedt
2023-03-02 14:18 ` Mark Rutland
2023-03-02 13:18 ` [PATCH 0/2] tracing: Fix adding some modifiers to histogram values Steven Rostedt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).