Re: [PATCH stable-4.19] tracing/uprobes: Fix output for multiple string arguments

[Andreas Ziegler <andreas.ziegler@xxxxxx>]

Hi,

> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index e696667da29a..04c1769a8c7c 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -142,6 +142,13 @@ static void FETCH_FUNC_NAME(memory, string)(struct pt_regs *regs,
>   	ret = strncpy_from_user(dst, src, maxlen);
>   	if (ret == maxlen)
>   		dst[--ret] = '\0';
> +	else if (ret >= 0)

with this fix the maxlen case still decrements ret by one which does not 
include the terminating \0 (and leaves maxlen = 1 in 
store_trace_args()). Usually this only happens with the last element but 
I still feel that it's inconsistent...

Would'nt it be easier to simply add a '+ 1' to to the make_rloc_data() 
statement, like this:

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index e696667da29a..ca0051a84ce8 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -147,7 +147,7 @@ static void FETCH_FUNC_NAME(memory, string)(struct 
pt_regs *regs,
                ((u8 *)get_rloc_data(dest))[0] = '\0';
                *(u32 *)dest = make_data_rloc(0, get_rloc_offs(rloc));
        } else {
-               *(u32 *)dest = make_data_rloc(ret, get_rloc_offs(rloc));
+               *(u32 *)dest = make_data_rloc(ret + 1, get_rloc_offs(rloc));
        }
 }

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

Regards,

Andreas