Hi Andreas,
On Thu, 14 Feb 2019 14:00:03 +0100
Andreas Ziegler <andreas.ziegler@xxxxxx> 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@xxxxxxxxxx>
