Messages in this thread | | | Date | Wed, 16 Jan 2019 09:34:18 -0500 | From | Steven Rostedt <> | Subject | Re: [PATCH v2] tracing/uprobes: Fix output for multiple string arguments |
| |
On Wed, 16 Jan 2019 15:16:29 +0100 Andreas Ziegler <andreas.ziegler@fau.de> wrote:
> When printing multiple uprobe arguments as strings the output for the > earlier arguments would also include all later string arguments. > > This is best explained in an example: > > Consider adding a uprobe to a function receiving two strings as > parameters which is at offset 0xa0 in strlib.so and we want to print > both parameters when the uprobe is hit (on x86_64): > > $ echo 'p:func /lib/strlib.so:0xa0 +0(%di):string +0(%si):string' > \ > /sys/kernel/debug/tracing/uprobe_events > > When the function is called as func("foo", "bar") and we hit the probe, > the trace file shows a line like the following: > > [...] func: (0x7f7e683706a0) arg1="foobar" arg2="bar" > > Note the extra "bar" printed as part of arg1. This behaviour stacks up > for additional string arguments. > > The strings are stored in a dynamically growing part of the uprobe > buffer by fetch_store_string() after copying them from userspace via > strncpy_from_user(). The return value of strncpy_from_user() is then > directly used as the required size for the string. However, this does > not take the terminating null byte into account as the documentation > for strncpy_from_user() cleary states that it "[...] returns the > length of the string (not including the trailing NUL)" even though the > null byte will be copied to the destination. > > Therefore, subsequent calls to fetch_store_string() will overwrite > the terminating null byte of the most recently fetched string with > the first character of the current string, leading to the > "accumulation" of strings in earlier arguments in the output. > > Fix this by incrementing the return value of strncpy_from_user() by > one if we did not hit the maximum buffer size. > > Signed-off-by: Andreas Ziegler <andreas.ziegler@fau.de>
Masami,
Care to give a Reviewed-by on this patch?
-- Steve
> --- > v2: removed a wrong check for (ret > 0) > > kernel/trace/trace_uprobe.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c > index e335576b9411..3a1d5ab6b4ba 100644 > --- a/kernel/trace/trace_uprobe.c > +++ b/kernel/trace/trace_uprobe.c > @@ -160,6 +160,13 @@ fetch_store_string(unsigned long addr, void *dest, void *base) > if (ret >= 0) { > if (ret == maxlen) > dst[ret - 1] = '\0'; > + else > + /* > + * Include the terminating null byte. In this case it > + * was copied by strncpy_from_user but not accounted > + * for in ret. > + */ > + ret++; > *(u32 *)dest = make_data_loc(ret, (void *)dst - base); > } > > -- > 2.17.1
| |