lkml.org 
[lkml]   [2019]   [Jan]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] tracing/uprobes: Fix output for multiple string arguments
From
Date
On 1/16/19 2:40 PM, Steven Rostedt wrote:
> On Wed, 16 Jan 2019 10:41:12 +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.
>>
>> Signed-off-by: Andreas Ziegler <andreas.ziegler@fau.de>
>> ---
>> 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..dfb9bbc7fd82 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 if (ret > 0)
>
> Do we need the ret > 0 check? What if the value is ""?
>
> Doesn't that cause the same issue?
>
> -- Steve
>

yes, it does. With this patch an empty string will also print "(fault)",
I missed that, sorry. I'll send a v2.

Thanks,

Andreas

>> + /*
>> + * 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);
>> }
>>
>

[unhandled content-type:application/pkcs7-signature]
\
 
 \ /
  Last update: 2019-01-16 15:14    [W:0.108 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site