tracing: Fix synthetic event bug
Subsystems (2)
Steven Rostedt
maintainerv2 cover tracing: Fix synthetic event bug
The follow commands caused a crash:
# cd /sys/kernel/tracing # echo 's:open char file[]' > dynamic_events # echo 'hist:keys=common_pid:file=filename:onchange($file).trace(open,$file)' > events/syscalls/sys_enter_openat/trigger' # echo 1 > events/synthetic/open/enable
BOOM!
The problem is that the synthetic event field "char file[]" will read the value given to it as a string without any memory checks to make sure the address is valid. The above example will pass in the user space address and the sythetic event code will happily call strlen() on it and then strscpy() where either one will cause an oops when accessing user space addresses.
Changes since v1: https://lore.kernel.org/all/[email protected]/
- Handle "(fault)" printing when there's a fault
Steven Rostedt (Google) (3):
tracing: Move duplicate code of trace_kprobe/eprobe.c into header
tracing: Add "(fault)" name injection to kernel probes
tracing: Fix reading strings from synthetic events
----
kernel/trace/trace_eprobe.c | 60 ++------------------ kernel/trace/trace_events_synth.c | 23 ++++++-- kernel/trace/trace_kprobe.c | 60 ++------------------ kernel/trace/trace_probe_kernel.h | 115 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 142 insertions(+), 116 deletions(-) create mode 100644 kernel/trace/trace_probe_kernel.h
Steven Rostedt
maintainerPATCH v2 1/3 tracing: Move duplicate code of trace_kprobe/eprobe.c into header
From: "Steven Rostedt (Google)" <[email protected]>
The functions:
fetch_store_strlen_user()
fetch_store_strlen()
fetch_store_string_user()
fetch_store_string()
are identical in both trace_kprobe.c and trace_eprobe.c. Move them into a new header file trace_probe_kernel.h to share it. This code will later be used by the synthetic events as well.
Marked for stable as a fix for a crash in synthetic events requires it.
--- kernel/trace/trace_eprobe.c | 60 ++----------------- kernel/trace/trace_kprobe.c | 60 ++----------------- kernel/trace/trace_probe_kernel.h | 96 +++++++++++++++++++++++++++++++ 3 files changed, 106 insertions(+), 110 deletions(-) create mode 100644 kernel/trace/trace_probe_kernel.h
kernel/trace/trace_eprobe.c +5 -55
@@ -16,6 +16,7 @@#include "trace_dynevent.h" #include "trace_probe.h" #include "trace_probe_tmpl.h" +#include "trace_probe_kernel.h" #define EPROBE_EVENT_SYSTEM "eprobes"
@@ -456,29 +457,14 @@ NOKPROBE_SYMBOL(process_fetch_insn) static nokprobe_inline intfetch_store_strlen_user (unsigned long addr) { -const void __user *uaddr = (__force const void __user *)addr; - -return strnlen_user_nofault (uaddr,MAX_STRING_SIZE ); +return kern_fetch_store_strlen_user (addr); }/* Return the length of string -- including null terminal byte */ static nokprobe_inline intfetch_store_strlen (unsigned long addr) { -int ret, len =0 ; -u8 c; - -#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE -if (addr <TASK_SIZE ) -return fetch_store_strlen_user (addr); -#endif - -do { - ret =copy_from_kernel_nofault (&c, (u8 *)addr + len,1 ); - len++; - }while (c && ret ==0 && len <MAX_STRING_SIZE ); - -return (ret <0 ) ? ret : len; +return kern_fetch_store_strlen (addr); }/*
@@ -488,21 +474,7 @@ fetch_store_strlen(unsigned long addr) static nokprobe_inline intfetch_store_string_user (unsigned long addr,void *dest,void *base) { -const void __user *uaddr = (__force const void __user *)addr; -int maxlen =get_loc_len (*(u32 *)dest); -void *__dest; -long ret; - -if (unlikely (!maxlen)) -return -ENOMEM ; - - __dest =get_loc_data (dest, base); - - ret =strncpy_from_user_nofault (__dest, uaddr, maxlen); -if (ret >=0 ) - *(u32 *)dest =make_data_loc (ret, __dest - base); - -return ret; +return kern_fetch_store_string_user (addr, dest, base); }/*
@@ -512,29 +484,7 @@ fetch_store_string_user(unsigned long addr, void *dest, void *base) static nokprobe_inline intfetch_store_string (unsigned long addr,void *dest,void *base) { -int maxlen =get_loc_len (*(u32 *)dest); -void *__dest; -long ret; - -#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE -if ((unsigned long )addr <TASK_SIZE ) -return fetch_store_string_user (addr, dest, base); -#endif - -if (unlikely (!maxlen)) -return -ENOMEM ; - - __dest =get_loc_data (dest, base); - -/* -* Try to get string again, since the string can be changed while -* probing. -*/ - ret =strncpy_from_kernel_nofault (__dest, (void *)addr, maxlen); -if (ret >=0 ) - *(u32 *)dest =make_data_loc (ret, __dest - base); - -return ret; +return kern_fetch_store_string (addr, dest, base); }static nokprobe_inline int
kernel/trace/trace_kprobe.c +5 -55
90 lines not on this page · raw message
kernel/trace/trace_probe_kernel.h new file +96 -0
96 lines not on this page · raw message
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH v2 2/3 tracing: Add "(fault)" name injection to kernel probes
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Have the specific functions for kernel probes that read strings to inject the "(fault)" name directly. trace_probes.c does this too (for uprobes) but as the code to read strings are going to be used by synthetic events (and perhaps other utilities), it simplifies the code by making sure those other uses do not need to implement the "(fault)" name injection as well.
--- kernel/trace/trace_probe_kernel.h | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-)
kernel/trace/trace_probe_kernel.h +25 -6
@@ -2,6 +2,8 @@#ifndef __TRACE_PROBE_KERNEL_H_#define __TRACE_PROBE_KERNEL_H_ +#define FAULT_STRING "(fault)" +/* * This depends on trace_probe.h, but can not include it due to * the way trace_probe_tmpl.h is used by trace_kprobe.c and trace_eprobe.c.
@@ -13,8 +15,16 @@ static nokprobe_inline int kern_fetch_store_strlen_user(unsigned long addr) {const void __user *uaddr = (__force const void __user *)addr; +int ret; -return strnlen_user_nofault (uaddr,MAX_STRING_SIZE ); + ret =strnlen_user_nofault (uaddr,MAX_STRING_SIZE ); +/* +* strnlen_user_nofault returns zero on fault, insert the +* FAULT_STRING when that occurs. +*/ +if (ret <=0 ) +return strlen (FAULT_STRING ) +1 ; +return ret; }/* Return the length of string -- including null terminal byte */
@@ -34,7 +44,18 @@ kern_fetch_store_strlen(unsigned long addr) len++; }while (c && ret ==0 && len <MAX_STRING_SIZE ); -return (ret <0 ) ? ret : len; +/* For faults, return enough to hold the FAULT_STRING */ +return (ret <0 ) ?strlen (FAULT_STRING ) +1 : len; +} + +static nokprobe_inline void set_data_loc(int ret, void *dest, void *__dest, void *base, int len) +{ +if (ret >=0 ) { + *(u32 *)dest =make_data_loc (ret, __dest - base); + }else { +strscpy (__dest,FAULT_STRING , len); + ret =strlen (__dest) +1 ; + } }/*
@@ -55,8 +76,7 @@ kern_fetch_store_string_user(unsigned long addr, void *dest, void *base) __dest =get_loc_data (dest, base); ret =strncpy_from_user_nofault (__dest, uaddr, maxlen); -if (ret >=0 ) - *(u32 *)dest =make_data_loc (ret, __dest - base); +set_data_loc (ret, dest, __dest, base, maxlen);return ret; }
@@ -87,8 +107,7 @@ kern_fetch_store_string(unsigned long addr, void *dest, void *base)* probing. */ ret =strncpy_from_kernel_nofault (__dest, (void *)addr, maxlen); -if (ret >=0 ) - *(u32 *)dest =make_data_loc (ret, __dest - base); +set_data_loc (ret, dest, __dest, base, maxlen);return ret; }
Signature · 1 line
2.35.1
RE: [PATCH v2 2/3] tracing: Add "(fault)" name injection to kernel probes
From: Steven Rostedt
Quoted 34 lines
Sent: 12 October 2022 11:41
Have the specific functions for kernel probes that read strings to inject the "(fault)" name directly.
trace_probes.cdoes this too (for uprobes) but as the code to read strings are going to be used by synthetic events (and perhaps other utilities), it simplifies the code by making sure those other uses do not need to implement the "(fault)" name injection as well.Fixes bd82631d7ccdc ("tracing: Add support for dynamic strings to synthetic events")Signed-off-by Steven Rostedt (Google) <[email protected]>--- kernel/trace/trace_probe_kernel.h | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-)kernel/trace/trace_probe_kernel.h +3
-0@@ -2,6 +2,8 @@#ifndef __TRACE_PROBE_KERNEL_H_#define __TRACE_PROBE_KERNEL_H_ +#define FAULT_STRING "(fault)" +/* * This depends on trace_probe.h, but can not include it due to * the way trace_probe_tmpl.h is used by trace_kprobe.c and trace_eprobe.c. @@ -13,8 +15,16 @@ static nokprobe_inline int kern_fetch_store_strlen_user(unsigned long addr) {const void __user *uaddr = (__force const void __user *)addr; +int ret;
kernel/trace/trace_probe_kernel.h static nokprobe_inline int · line 28
Quoted 10 lines
kernel/trace/trace_probe_kernel.h +8
-1-return strnlen_user_nofault (uaddr,MAX_STRING_SIZE ); + ret =strnlen_user_nofault (uaddr,MAX_STRING_SIZE ); +/* +* strnlen_user_nofault returns zero on fault, insert the +* FAULT_STRING when that occurs. +*/ +if (ret <=0 ) +return strlen (FAULT_STRING ) +1 ; +return ret; }
Isn't that going to do the wrong thing if the user string is valid memory but just zero length??
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Steven Rostedt
maintainerRe: [PATCH v2 2/3] tracing: Add "(fault)" name injection to kernel probes
Quoted 6 lines · On Wed, 12 Oct 2022 12:34:45 +0000 David Laight <[email protected]> wrote:
Quoted 6 lines
kernel/trace/trace_probe_kernel.h +1
-0@@ -13,8 +15,16 @@ static nokprobe_inline int kern_fetch_store_strlen_user(unsigned long addr) {const void __user *uaddr = (__force const void __user *)addr; +int ret;
Quoted 13 lines
kernel/trace/trace_probe_kernel.h static nokprobe_inline int · line 28
Quoted 10 lines
kernel/trace/trace_probe_kernel.h +8
-1-return strnlen_user_nofault (uaddr,MAX_STRING_SIZE ); + ret =strnlen_user_nofault (uaddr,MAX_STRING_SIZE ); +/* +* strnlen_user_nofault returns zero on fault, insert the +* FAULT_STRING when that occurs. +*/ +if (ret <=0 ) +return strlen (FAULT_STRING ) +1 ; +return ret; }Isn't that going to do the wrong thing if the user string is valid memory but just zero length??
I thought so at first (and was in the process of changing things because of that) until I saw the comment above this code:
/* Return the length of string -- including null terminal byte */
And looking the function of strnlen_user_nofault():
* Returns the size of the string INCLUDING the terminating NUL.
That is, it returns 1 on a zero length string and 0 on fault :-p
Yes, I think we should fix that API, but that's another story.
-- Steve
Steven Rostedt
maintainerPATCH v2 3/3 tracing: Fix reading strings from synthetic events
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
The follow commands caused a crash:
# cd /sys/kernel/tracing # echo 's:open char file[]' > dynamic_events # echo 'hist:keys=common_pid:file=filename:onchange($file).trace(open,$file)' > events/syscalls/sys_enter_openat/trigger' # echo 1 > events/synthetic/open/enable
BOOM!
The problem is that the synthetic event field "char file[]" will read the value given to it as a string without any memory checks to make sure the address is valid. The above example will pass in the user space address and the sythetic event code will happily call strlen() on it and then strscpy() where either one will cause an oops when accessing user space addresses.
Use the helper functions from trace_kprobe and trace_eprobe that can read strings safely (and actually succeed when the address is from user space and the memory is mapped in).
Now the above can show:
packagekitd-1721 [000] ...2. 104.597170: open: file=/usr/lib/rpm/fileattrs/cmake.attr
in:imjournal-978 [006] ...2. 104.599642: open: file=/var/lib/rsyslog/imjournal.state.tmp
packagekitd-1721 [000] ...2. 104.626308: open: file=/usr/lib/rpm/fileattrs/debuginfo.attr--- kernel/trace/trace_events_synth.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)
kernel/trace/trace_events_synth.c +17 -6
@@ -17,6 +17,8 @@/* for gfp flag names */ #include <linux/trace_events.h> #include <trace/events/mmflags.h> +#include "trace_probe.h" +#include "trace_probe_kernel.h" #include "trace_synth.h"
@@ -409,6 +411,7 @@ static unsigned int trace_string(struct synth_trace_event *entry, {unsigned int len =0 ;char *str_field; +int ret;if (is_dynamic) {u32 data_offset;
@@ -417,19 +420,27 @@ static unsigned int trace_string(struct synth_trace_event *entry, data_offset += event->n_u64 *sizeof (u64); data_offset += data_size; - str_field = (char *)entry + data_offset; - - len =strlen (str_val) +1 ; -strscpy (str_field, str_val, len); + len =kern_fetch_store_strlen ((unsigned long )str_val); data_offset |= len <<16 ; *(u32 *)&entry->fields[*n_u64] = data_offset; + ret =kern_fetch_store_string ((unsigned long )str_val, &entry->fields[*n_u64], entry); + (*n_u64)++; }else { str_field = (char *)&entry->fields[*n_u64]; -strscpy (str_field, str_val,STR_VAR_LEN_MAX ); +#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE +if ((unsigned long )str_val <TASK_SIZE ) + ret =strncpy_from_user_nofault (str_field, str_val,STR_VAR_LEN_MAX ); + else +#endif + ret =strncpy_from_kernel_nofault (str_field, str_val,STR_VAR_LEN_MAX ); + +if (ret <0 ) +strcpy (str_field,FAULT_STRING ); + (*n_u64) +=STR_VAR_LEN_MAX /sizeof (u64); }
@@ -462,7 +473,7 @@ static notrace void trace_event_raw_event_synth(void *__data, val_idx = var_ref_idx[field_pos]; str_val = (char *)(long )var_ref_vals[val_idx]; - len =strlen (str_val) +1 ; + len =kern_fetch_store_strlen ((unsigned long )str_val); fields_size += len; }
Signature · 1 line
2.35.1
Hi,
2022年10月12日(水) 11:45 Steven Rostedt <[email protected]>:
Quoted 11 lines
The follow commands caused a crash:
# cd /sys/kernel/tracing # echo 's:open char file[]' > dynamic_events # echo 'hist:keys=common_pid:file=filename:onchange($file).trace(open,$file)' > events/syscalls/sys_enter_openat/trigger' # echo 1 > events/synthetic/open/enableBOOM!
Quoted 8 lines
The problem is that the synthetic event field "char file[]" will read the value given to it as a string without any memory checks to make sure the address is valid. The above example will pass in the user space address and the sythetic event code will happily call
strlen()on it and thenstrscpy()where either one will cause an oops when accessing user space addresses.Changes since v1: https://lore.kernel.org/all/[email protected]/
Thanks, this series of patches looks good to me.
Thank you!
Quoted 15 lines
- Handle "(fault)" printing when there's a fault
Steven Rostedt (Google) (3):
tracing: Move duplicate code oftrace_kprobe/eprobe.cinto header
tracing: Add "(fault)" name injection to kernel probes
tracing: Fix reading strings from synthetic events----
kernel/trace/trace_eprobe.c | 60 ++------------------ kernel/trace/trace_events_synth.c | 23 ++++++-- kernel/trace/trace_kprobe.c | 60 ++------------------ kernel/trace/trace_probe_kernel.h | 115 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 142 insertions(+), 116 deletions(-) create mode 100644 kernel/trace/trace_probe_kernel.h
Signature · 2 lines
Masami Hiramatsu mailto:[email protected]
Hi Steve,
Quoted 20 lines · Steven Rostedt
The follow commands caused a crash:
# cd /sys/kernel/tracing # echo 's:open char file[]' > dynamic_events # echo 'hist:keys=common_pid:file=filename:onchange($file).trace(open,$file) ' > events/syscalls/sys_enter_openat/trigger' # echo 1 > events/synthetic/open/enableBOOM!
The problem is that the synthetic event field "char file[]" will read the value given to it as a string without any memory checks to make sure
the address is valid. The above example will pass in the user space address and the sythetic event code will happily callstrlen()on it and thenstrscpy()where either one will cause an oops when accessing user space addresses.
Quoted 4 lines
Changes since v1:
https://lore.kernel.org/all/[email protected]/- Handle "(fault)" printing when there's a fault
Thanks for fixing the synthetic event string tracing bug, along with the other nice cleanup.
Quoted 15 lines
Steven Rostedt (Google) (3):
tracing: Move duplicate code oftrace_kprobe/eprobe.cinto header
tracing: Add "(fault)" name injection to kernel probes
tracing: Fix reading strings from synthetic events----
kernel/trace/trace_eprobe.c | 60 ++------------------kernel/trace/trace_events_synth.c| 23 ++++++-- kernel/trace/trace_kprobe.c | 60 ++------------------kernel/trace/trace_probe_kernel.h| 115
++++++++++++++++++++++++++++++++++++++4 files changed, 142 insertions(+), 116 deletions(-)create mode 100644
kernel/trace/trace_probe_kernel.h