Hi Andreas,
On Thu, 14 Feb 2019 16:53:26 +0100
Andreas Ziegler <andreas.ziegler@xxxxxx> wrote:
> Hi Masami,
>
> yes, that patch looks like a good compromise between the
> original code and the new patch. To reduce the number of if
> statements involving ret we would need to move more things
> around and I think it's best to avoid that (as you also said).
>
> I'd say you can update this for the other kernel stable trees
> as well.
Thank you very much!
>
> Thank you!
>
> Andreas
>
> P.S.: Do we need to explain in the commit message why
> backporting is needed?
>
> > On 14. Feb 2019, at 15:30, Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
> >
> > Hi Andreas,
> >
> > I've update this patch to include "ret == maxlen" case fix
> > as similar to the latest kernel code.
> >
> > Please review it. If it is OK, I'll update patches for other
> > stable trees.
> >
> > Thank you,
> >
> > On Thu, 14 Feb 2019 23:24:29 +0900
> > Masami Hiramatsu <mhiramat@xxxxxxxxxx> 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';
> >>
> >
> >
> > --
> > Masami Hiramatsu <mhiramat@xxxxxxxxxx>
>
--
Masami Hiramatsu <mhiramat@xxxxxxxxxx>
