On Wed, Mar 01, 2023 at 08:00:53PM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
> Reported-by: Mark Rutland <mark.rutland@xxxxxxx>
> Fixes: c6afad49d127f ("tracing: Add hist trigger 'sym' and 'sym-offset' modifiers")
> Signed-off-by: Steven Rostedt (Google) <rostedt@xxxxxxxxxxx>
Tested-by: Mark Rutland <mark.rutland@xxxxxxx>
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
