Re: [PATCH v2] tracing/uprobes: Fix output for multiple string arguments
On Thu, 17 Jan 2019 16:14:44 +0100
Andreas Ziegler wrote:
> One more thing maybe, the code in question was rewritten
> by Masami in 9178412ddf5a ("tracing: probeevent: Return consumed
> bytes of dynamic area”) which is in v4.20 but the problem traces back
> to the original commit mentioned above, so it will need manual so we
> will need a slightly modified version for earlier kernels.
That's fine. When Greg (or other stable maintainer) process the commit,
it will produce a "FAILED" message and Cc all those in the tags
(obviously, you'll be one of them as the author), and then you can
reply to that email with the fixed up version and Greg (or whoever)
will take that patch in.
Thanks!
-- Steve
Re: [PATCH v2] tracing/uprobes: Fix output for multiple string arguments
On 17. Jan 2019, at 15:51, Steven Rostedt wrote:
>
> On Thu, 17 Jan 2019 15:29:09 +0100
> Andreas Ziegler wrote:
>
>> From my side it's basically good to go, but I just realized that there
>> is no "Fixes: " tag in it but the problem it fixes goes back to
>> 5baaa59ef09e ("tracing/probes: Implement 'memory' fetch method for
>> uprobes") from 2013 (and is present in all current longterm and stable
>> kernels)... should I add a tag and resend a v3?
>>
>
> I was just about to ask you to provide the fixes tag. No need to
> resend, I'll just add it manually. I asked Masami to resend because it
> was a patch series. If it was just a single patch, I would have just
> asked what the fixes was for that particular patch. I can handle one
> patch, but not more than one ;-)
>
> -- Steve
One more thing maybe, the code in question was rewritten
by Masami in 9178412ddf5a ("tracing: probeevent: Return consumed
bytes of dynamic area”) which is in v4.20 but the problem traces back
to the original commit mentioned above, so it will need manual so we
will need a slightly modified version for earlier kernels.
Regards,
Andreas
Re: [PATCH v2] tracing/uprobes: Fix output for multiple string arguments
On Thu, 17 Jan 2019 15:29:09 +0100
Andreas Ziegler wrote:
> From my side it's basically good to go, but I just realized that there
> is no "Fixes: " tag in it but the problem it fixes goes back to
> 5baaa59ef09e ("tracing/probes: Implement 'memory' fetch method for
> uprobes") from 2013 (and is present in all current longterm and stable
> kernels)... should I add a tag and resend a v3?
>
I was just about to ask you to provide the fixes tag. No need to
resend, I'll just add it manually. I asked Masami to resend because it
was a patch series. If it was just a single patch, I would have just
asked what the fixes was for that particular patch. I can handle one
patch, but not more than one ;-)
-- Steve
Re: [PATCH v2] tracing/uprobes: Fix output for multiple string arguments
On 1/17/19 3:20 PM, Steven Rostedt wrote:
On Thu, 17 Jan 2019 16:58:07 +0900
Masami Hiramatsu wrote:
Ah, I got it.
Hmm, in that case, I have to update my patch in the previous mail.
Anyway,
Acked-by: Masami Hiramatsu
So this patch is good to go? If so, I'll pull it in and start testing
it.
I'm currently traveling (as you probably received my OoO emails), but I
can still do a bit of work remotely.
-- Steve
From my side it's basically good to go, but I just realized that there
is no "Fixes: " tag in it but the problem it fixes goes back to
5baaa59ef09e ("tracing/probes: Implement 'memory' fetch method for
uprobes") from 2013 (and is present in all current longterm and stable
kernels)... should I add a tag and resend a v3?
Thanks,
Andreas
Re: [PATCH v2] tracing/uprobes: Fix output for multiple string arguments
On Thu, 17 Jan 2019 16:58:07 +0900 Masami Hiramatsu wrote: > Ah, I got it. > Hmm, in that case, I have to update my patch in the previous mail. > Anyway, > > Acked-by: Masami Hiramatsu So this patch is good to go? If so, I'll pull it in and start testing it. I'm currently traveling (as you probably received my OoO emails), but I can still do a bit of work remotely. -- Steve
Re: [PATCH v2] tracing/uprobes: Fix output for multiple string arguments
On Thu, 17 Jan 2019 08:40:05 +0100
Andreas Ziegler wrote:
> On 17.01.19 07:01, Masami Hiramatsu wrote:
> > On Wed, 16 Jan 2019 15:16:29 +0100
> > Andreas Ziegler 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
Thank you!!
>
> Thanks,
>
> Andreas
>
> > Could you fix fetch_store_strlen in the same patch?
> >
> > Thank you,
> >
> >> Signed-off-by: Andreas Ziegler
> >> ---
> >> 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
Re: [PATCH v2] tracing/uprobes: Fix output for multiple string arguments
On 17.01.19 07:01, Masami Hiramatsu wrote:
> On Wed, 16 Jan 2019 15:16:29 +0100
> Andreas Ziegler 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().
Thanks,
Andreas
> Could you fix fetch_store_strlen in the same patch?
>
> Thank you,
>
>> Signed-off-by: Andreas Ziegler
>> ---
>> 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
>>
>
>
Re: [PATCH v2] tracing/uprobes: Fix output for multiple string arguments
On Wed, 16 Jan 2019 15:16:29 +0100
Andreas Ziegler 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)
Could you fix fetch_store_strlen in the same patch?
Thank you,
> Signed-off-by: Andreas Ziegler
> ---
> 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
Re: [PATCH v2] tracing/uprobes: Fix output for multiple string arguments
On Wed, 16 Jan 2019 15:16:29 +0100
Andreas Ziegler 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
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
[PATCH v2] tracing/uprobes: Fix output for multiple string arguments
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
---
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
