Messages in this thread | | | Date | Thu, 17 Jan 2019 16:58:07 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH v2] tracing/uprobes: Fix output for multiple string arguments |
| |
On Thu, 17 Jan 2019 08:40:05 +0100 Andreas Ziegler <andreas.ziegler@fau.de> wrote:
> On 17.01.19 07:01, Masami Hiramatsu wrote: > > 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. > >> > > > > Yeah, I had eventually same conclusion. However, you also have to increse > > the return value of fetch_store_strlen() too. (And I found another issue) > > > > I don't think we need to increase that since the documentation for > strnlen_user() says that it "[r]eturns the size of the string > INCLUDING the terminating NUL." so its return value will already be > one more than that of strncpy_from_user().
Ah, I got it. Hmm, in that case, I have to update my patch in the previous mail. Anyway,
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Thank you!!
> > Thanks, > > Andreas > > > Could you fix fetch_store_strlen in the same patch? > > > > Thank you, > > > >> Signed-off-by: Andreas Ziegler <andreas.ziegler@fau.de> > >> --- > >> 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 > >> > > > > >
-- Masami Hiramatsu <mhiramat@kernel.org>
| |