kprobes string reading broken on s390

kprobes string reading broken on s390

[Sven Schnelle]

Hi Christoph,

with the latest linux-next i noticed that some tests in the
ftrace test suites are failing on s390, namely:

[FAIL] Kprobe event symbol argument
[FAIL] Kprobe event with comm arguments

The following doesn't work anymore:

cd /sys/kernel/tracing
echo 'p:testprobe _do_fork comm=$comm ' >kprobe_events
echo 1 >/sys/kernel/tracing/events/kprobes/testprobe/enable
cat /sys/kernel/tracing/trace

it will just show

test.sh-519   [012] ....    18.580625: testprobe: (_do_fork+0x0/0x3c8) comm=(fault)

Looking at d411a9c4e95a ("tracing/kprobes: handle mixed kernel/userspace probes
better") i see that there are two helpers for reading strings:

fetch_store_string_user() -> read string from user space
fetch_store_string() -> read string from kernel space(?)

but in the end both are using strncpy_from_user_nofault(), but i would
think that fetch_store_string() should use strncpy_from_kernel_nofault().
However, i'm not sure about the exact semantics of fetch_store_string(),
as there where a lot of wrong assumptions in the past, especially since
on x86 you usually don't fail if you use the same function for accessing kernel
and userspace although it's technically wrong.

Regards,
Sven

commit 81408eab8fcc79dc0871a95462b13176d3446f5e
Author: Sven Schnelle <svens@linux.ibm.com>
Date:   Fri Jun 5 13:01:24 2020 +0200

    kprobes: use strncpy_from_kernel_nofault() in fetch_store_string()

    Signed-off-by: Sven Schnelle <svens@linux.ibm.com>

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index b1f21d558e45..ea8d0b094f1b 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -1278,7 +1278,7 @@ fetch_store_string(unsigned long addr, void *dest, void *base)
 	 * Try to get string again, since the string can be changed while
 	 * probing.
 	 */
-	ret = strncpy_from_user_nofault(__dest, (void *)addr, maxlen);
+	ret = strncpy_from_kernel_nofault(__dest, (void *)addr, maxlen);
 	if (ret >= 0)
 		*(u32 *)dest = make_data_loc(ret, __dest - base);
 

Re: kprobes string reading broken on s390

[Christoph Hellwig]

Yes, this looks correct.  You probably want to write a small changelog
and add a Fixes tag, though.

On Fri, Jun 05, 2020 at 01:05:34PM +0200, Sven Schnelle wrote:
> Hi Christoph,
> 
> with the latest linux-next i noticed that some tests in the
> ftrace test suites are failing on s390, namely:
> 
> [FAIL] Kprobe event symbol argument
> [FAIL] Kprobe event with comm arguments
> 
> The following doesn't work anymore:
> 
> cd /sys/kernel/tracing
> echo 'p:testprobe _do_fork comm=$comm ' >kprobe_events
> echo 1 >/sys/kernel/tracing/events/kprobes/testprobe/enable
> cat /sys/kernel/tracing/trace
> 
> it will just show
> 
> test.sh-519   [012] ....    18.580625: testprobe: (_do_fork+0x0/0x3c8) comm=(fault)
> 
> Looking at d411a9c4e95a ("tracing/kprobes: handle mixed kernel/userspace probes
> better") i see that there are two helpers for reading strings:
> 
> fetch_store_string_user() -> read string from user space
> fetch_store_string() -> read string from kernel space(?)
> 
> but in the end both are using strncpy_from_user_nofault(), but i would
> think that fetch_store_string() should use strncpy_from_kernel_nofault().
> However, i'm not sure about the exact semantics of fetch_store_string(),
> as there where a lot of wrong assumptions in the past, especially since
> on x86 you usually don't fail if you use the same function for accessing kernel
> and userspace although it's technically wrong.
> 
> Regards,
> Sven
> 
> commit 81408eab8fcc79dc0871a95462b13176d3446f5e
> Author: Sven Schnelle <svens@linux.ibm.com>
> Date:   Fri Jun 5 13:01:24 2020 +0200
> 
>     kprobes: use strncpy_from_kernel_nofault() in fetch_store_string()
> 
>     Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index b1f21d558e45..ea8d0b094f1b 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -1278,7 +1278,7 @@ fetch_store_string(unsigned long addr, void *dest, void *base)
>  	 * Try to get string again, since the string can be changed while
>  	 * probing.
>  	 */
> -	ret = strncpy_from_user_nofault(__dest, (void *)addr, maxlen);
> +	ret = strncpy_from_kernel_nofault(__dest, (void *)addr, maxlen);
>  	if (ret >= 0)
>  		*(u32 *)dest = make_data_loc(ret, __dest - base);
>  
---end quoted text---

Re: kprobes string reading broken on s390

[Masami Hiramatsu]

Hi Sven,

On Fri, 5 Jun 2020 15:25:41 +0200
Christoph Hellwig <hch@lst.de> wrote:

> Yes, this looks correct.  You probably want to write a small changelog
> and add a Fixes tag, though.
> 
> On Fri, Jun 05, 2020 at 01:05:34PM +0200, Sven Schnelle wrote:
> > Hi Christoph,
> > 
> > with the latest linux-next i noticed that some tests in the
> > ftrace test suites are failing on s390, namely:
> > 
> > [FAIL] Kprobe event symbol argument
> > [FAIL] Kprobe event with comm arguments
> > 
> > The following doesn't work anymore:
> > 
> > cd /sys/kernel/tracing
> > echo 'p:testprobe _do_fork comm=$comm ' >kprobe_events
> > echo 1 >/sys/kernel/tracing/events/kprobes/testprobe/enable
> > cat /sys/kernel/tracing/trace
> > 
> > it will just show
> > 
> > test.sh-519   [012] ....    18.580625: testprobe: (_do_fork+0x0/0x3c8) comm=(fault)
> > 
> > Looking at d411a9c4e95a ("tracing/kprobes: handle mixed kernel/userspace probes
> > better") i see that there are two helpers for reading strings:
> > 
> > fetch_store_string_user() -> read string from user space
> > fetch_store_string() -> read string from kernel space(?)
> > 
> > but in the end both are using strncpy_from_user_nofault(), but i would
> > think that fetch_store_string() should use strncpy_from_kernel_nofault().
> > However, i'm not sure about the exact semantics of fetch_store_string(),
> > as there where a lot of wrong assumptions in the past, especially since
> > on x86 you usually don't fail if you use the same function for accessing kernel
> > and userspace although it's technically wrong.

Thanks for fixing!
This report can be a good changelog.
Please resend it with Fixed tag as Christoph said.

Christoph, it seems your series in the akpm tree, so this also should
be sent to Andrew?

Thank you,

> > 
> > Regards,
> > Sven
> > 
> > commit 81408eab8fcc79dc0871a95462b13176d3446f5e
> > Author: Sven Schnelle <svens@linux.ibm.com>
> > Date:   Fri Jun 5 13:01:24 2020 +0200
> > 
> >     kprobes: use strncpy_from_kernel_nofault() in fetch_store_string()
> > 
> >     Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
> > 
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index b1f21d558e45..ea8d0b094f1b 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -1278,7 +1278,7 @@ fetch_store_string(unsigned long addr, void *dest, void *base)
> >  	 * Try to get string again, since the string can be changed while
> >  	 * probing.
> >  	 */
> > -	ret = strncpy_from_user_nofault(__dest, (void *)addr, maxlen);
> > +	ret = strncpy_from_kernel_nofault(__dest, (void *)addr, maxlen);
> >  	if (ret >= 0)
> >  		*(u32 *)dest = make_data_loc(ret, __dest - base);
> >  
> ---end quoted text---


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: kprobes string reading broken on s390

[Sven Schnelle]

Masami,

On Sat, Jun 06, 2020 at 01:58:06AM +0900, Masami Hiramatsu wrote:
> Hi Sven,
> 
> On Fri, 5 Jun 2020 15:25:41 +0200
> Christoph Hellwig <hch@lst.de> wrote:
> 
> > Yes, this looks correct.  You probably want to write a small changelog
> > and add a Fixes tag, though.
> > 
> > On Fri, Jun 05, 2020 at 01:05:34PM +0200, Sven Schnelle wrote:
> > > Hi Christoph,
> > > 
> > > with the latest linux-next i noticed that some tests in the
> > > ftrace test suites are failing on s390, namely:
> > > 
> > > [FAIL] Kprobe event symbol argument
> > > [FAIL] Kprobe event with comm arguments
> > > 
> > > The following doesn't work anymore:
> > > 
> > > cd /sys/kernel/tracing
> > > echo 'p:testprobe _do_fork comm=$comm ' >kprobe_events
> > > echo 1 >/sys/kernel/tracing/events/kprobes/testprobe/enable
> > > cat /sys/kernel/tracing/trace
> > > 
> > > it will just show
> > > 
> > > test.sh-519   [012] ....    18.580625: testprobe: (_do_fork+0x0/0x3c8) comm=(fault)
> > > 
> > > Looking at d411a9c4e95a ("tracing/kprobes: handle mixed kernel/userspace probes
> > > better") i see that there are two helpers for reading strings:
> > > 
> > > fetch_store_string_user() -> read string from user space
> > > fetch_store_string() -> read string from kernel space(?)
> > > 
> > > but in the end both are using strncpy_from_user_nofault(), but i would
> > > think that fetch_store_string() should use strncpy_from_kernel_nofault().
> > > However, i'm not sure about the exact semantics of fetch_store_string(),
> > > as there where a lot of wrong assumptions in the past, especially since
> > > on x86 you usually don't fail if you use the same function for accessing kernel
> > > and userspace although it's technically wrong.
> 
> Thanks for fixing!
> This report can be a good changelog.
> Please resend it with Fixed tag as Christoph said.

Which SHA1 should the Fixes tag carry? The one from linux-next?

Regards
Sven

Re: kprobes string reading broken on s390

[Christoph Hellwig]

On Fri, Jun 05, 2020 at 07:44:33PM +0200, Sven Schnelle wrote:
> > Thanks for fixing!
> > This report can be a good changelog.
> > Please resend it with Fixed tag as Christoph said.
> 
> Which SHA1 should the Fixes tag carry? The one from linux-next?

Oh, I thought it had hit mainline already.  If this is just in -mm
there is no need for a fixes tag, just send it to Andrew with the
acks from Masami and me.

Acked-by: Christoph Hellwig <hch@lst.de>