Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 17 Jan 2019 15:01:07 +0900
From:   Masami Hiramatsu <mhiramat@kernel.org>
To:     Andreas Ziegler <andreas.ziegler@fau.de>
Cc:     Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@redhat.com>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] tracing/uprobes: Fix output for multiple string
 arguments
Message-Id: <20190117150107.17f2c0c37e41126120c5eebb@kernel.org>
In-Reply-To: <20190116141629.5752-1-andreas.ziegler@fau.de>
References: <20190116084021.34b0beee@gandalf.local.home>
        <20190116141629.5752-1-andreas.ziegler@fau.de>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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)

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>