Re: [PATCH v2] tracing/uprobes: Fix output for multiple string arguments

[Masami Hiramatsu <mhiramat@kernel.org>]

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>