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

[Masami Hiramatsu <mhiramat@kernel.org>]

Hi Andreas,

On Thu, 14 Feb 2019 14:00:03 +0100
Andreas Ziegler <andreas.ziegler@fau.de> wrote:

> 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...

Yes, I found that too. And I thought it was another fix.

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

Hmm, in that case, I would like to make "--ret" to "ret - 1" as the
latest kernel does. Keeping the final code similar will help us
if we need another fix to backport.

Any thought?

Thank you,

> 
> 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


-- 
Masami Hiramatsu <mhiramat@kernel.org>