On Thu, Feb 14, 2019 at 11:24:29PM +0900, Masami Hiramatsu wrote:
> From: Andreas Ziegler <andreas.ziegler@xxxxxx>
>
> commit 0722069a5374b904ec1a67f91249f90e1cfae259 upstream.
>
> 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.
>
> Link: http://lkml.kernel.org/r/20190116141629.5752-1-andreas.ziegler@xxxxxx
>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 5baaa59ef09e ("tracing/probes: Implement 'memory' fetch method for uprobes")
> Acked-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> Signed-off-by: Andreas Ziegler <andreas.ziegler@xxxxxx>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>
> Signed-off-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> ---
> kernel/trace/trace_uprobe.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index e696667da29a..bf93ae152c22 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -141,7 +141,14 @@ static void FETCH_FUNC_NAME(memory, string)(struct pt_regs *regs,
>
> ret = strncpy_from_user(dst, src, maxlen);
> if (ret == maxlen)
> - dst[--ret] = '\0';
> + dst[ret - 1] = '\0';
> + else if (ret >= 0)
> + /*
> + * Include the terminating null byte. In this case it
> + * was copied by strncpy_from_user but not accounted
> + * for in ret.
> + */
> + ret++;
>
> if (ret < 0) { /* Failed to fetch string */
> ((u8 *)get_rloc_data(dest))[0] = '\0';
>
All now queued up, thanks.
greg k-h
