[PATCH] kprobes/x86: Use the current code when modified by Kprobes using ftrace

[PATCH] kprobes/x86: Use the current code when modified by Kprobes using ftrace

[Petr Mladek]

can_probe() checks if the given address points to the beginning of
an instruction. It analyzes all the instructions from the beginning
of the function until the given address. The code might be modified
by another Kprobe. In this case, the current code is read into a buffer,
int3 breakpoint is replaced by the saved opcode in the buffer, and
can_probe() analyzes the buffer instead.

There is a bug that __recover_probed_insn() tries to restore
the original code even for Kprobes using the ftrace framework.
But in this case, the opcode is not stored. See the difference
between arch_prepare_kprobe() and arch_prepare_kprobe_ftrace().
The opcode is stored by arch_copy_kprobe() only from
arch_prepare_kprobe().

This patch makes Kprobe to use the current code when it is modified
by ftrace. It is not the original one but it is a valid instruction
of the same length. Therefore it is perfectly fine for the check.

Note that I found this problem when playing with Kprobes. I did it
on x86_64 with gcc-4.8.3 that supported -mfentry. I modified
samples/kprobes/kprobe_example.c and added offset 5 to put
the probe right after the fentry area:

--- cut ---
 static struct kprobe kp = {
 	.symbol_name	= "do_fork",
+	.offset = 5,
 };
--- cut ---

Then I was able to load kprobe_example before jprobe_example
but not the other way around:

$> modprobe jprobe_example
$> modprobe kprobe_example
modprobe: ERROR: could not insert 'kprobe_example': Invalid or incomplete multibyte or wide character

It did not make much sense and debugging pointed to the bug
described above.

Signed-off-by: Petr Mladek <pmladek@suse.cz>
---
 arch/x86/kernel/kprobes/core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 98f654d466e5..ef321caae3ba 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -225,8 +225,14 @@ __recover_probed_insn(kprobe_opcode_t *buf, unsigned long addr)
 	struct kprobe *kp;
 
 	kp = get_kprobe((void *)addr);
-	/* There is no probe, return original address */
-	if (!kp)
+	/*
+	 * Use the current code if it is not modified by Kprobe
+	 * or when the Kprobe is using ftrace. In the second case
+	 * we do not have any information about the original code
+	 * but it is not a problem. Ftrace has put there a valid
+	 * instruction of the same length.
+	 */
+	if (!kp || kprobe_ftrace(kp))
 		return addr;
 
 	/*
-- 
1.8.5.6

Re: [PATCH] kprobes/x86: Use the current code when modified by Kprobes using ftrace

[Masami Hiramatsu]

(2015/01/31 0:45), Petr Mladek wrote:
> can_probe() checks if the given address points to the beginning of
> an instruction. It analyzes all the instructions from the beginning
> of the function until the given address. The code might be modified
> by another Kprobe. In this case, the current code is read into a buffer,
> int3 breakpoint is replaced by the saved opcode in the buffer, and
> can_probe() analyzes the buffer instead.
> 
> There is a bug that __recover_probed_insn() tries to restore
> the original code even for Kprobes using the ftrace framework.
> But in this case, the opcode is not stored. See the difference
> between arch_prepare_kprobe() and arch_prepare_kprobe_ftrace().
> The opcode is stored by arch_copy_kprobe() only from
> arch_prepare_kprobe().

Good catch!

> This patch makes Kprobe to use the current code when it is modified
> by ftrace. It is not the original one but it is a valid instruction
> of the same length. Therefore it is perfectly fine for the check.

Hmm, but there is a chance we can hit it in the period of replacing code.
This means we'll see 5byte nop but started with int3. (see P6_NOP5 in
arch/x86/include/asm/nops.h)
So, I think we'd better return a special buffer which has a 5byte NOP.

Thank you for reporting!

> 
> Note that I found this problem when playing with Kprobes. I did it
> on x86_64 with gcc-4.8.3 that supported -mfentry. I modified
> samples/kprobes/kprobe_example.c and added offset 5 to put
> the probe right after the fentry area:
> 
> --- cut ---
>  static struct kprobe kp = {
>  	.symbol_name	= "do_fork",
> +	.offset = 5,
>  };
> --- cut ---
> 
> Then I was able to load kprobe_example before jprobe_example
> but not the other way around:
> 
> $> modprobe jprobe_example
> $> modprobe kprobe_example
> modprobe: ERROR: could not insert 'kprobe_example': Invalid or incomplete multibyte or wide character
> 
> It did not make much sense and debugging pointed to the bug
> described above.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.cz>
> ---
>  arch/x86/kernel/kprobes/core.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index 98f654d466e5..ef321caae3ba 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -225,8 +225,14 @@ __recover_probed_insn(kprobe_opcode_t *buf, unsigned long addr)
>  	struct kprobe *kp;
>  
>  	kp = get_kprobe((void *)addr);
> -	/* There is no probe, return original address */
> -	if (!kp)
> +	/*
> +	 * Use the current code if it is not modified by Kprobe
> +	 * or when the Kprobe is using ftrace. In the second case
> +	 * we do not have any information about the original code
> +	 * but it is not a problem. Ftrace has put there a valid
> +	 * instruction of the same length.
> +	 */
> +	if (!kp || kprobe_ftrace(kp))
>  		return addr;
>  
>  	/*
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH] kprobes/x86: Use the current code when modified by Kprobes using ftrace

[Petr Mladek]

On Mon 2015-02-02 17:40:10, Masami Hiramatsu wrote:
> (2015/01/31 0:45), Petr Mladek wrote:
> > can_probe() checks if the given address points to the beginning of
> > an instruction. It analyzes all the instructions from the beginning
> > of the function until the given address. The code might be modified
> > by another Kprobe. In this case, the current code is read into a buffer,
> > int3 breakpoint is replaced by the saved opcode in the buffer, and
> > can_probe() analyzes the buffer instead.
> > 
> > There is a bug that __recover_probed_insn() tries to restore
> > the original code even for Kprobes using the ftrace framework.
> > But in this case, the opcode is not stored. See the difference
> > between arch_prepare_kprobe() and arch_prepare_kprobe_ftrace().
> > The opcode is stored by arch_copy_kprobe() only from
> > arch_prepare_kprobe().
> 
> Good catch!
> 
> > This patch makes Kprobe to use the current code when it is modified
> > by ftrace. It is not the original one but it is a valid instruction
> > of the same length. Therefore it is perfectly fine for the check.
> 
> Hmm, but there is a chance we can hit it in the period of replacing code.
> This means we'll see 5byte nop but started with int3. (see P6_NOP5 in
> arch/x86/include/asm/nops.h)
> So, I think we'd better return a special buffer which has a 5byte NOP.

Good point! I though that we were synchronized against ftrace by
text_mutex and kprobe_mutex but we were not. Ftrace does not take
text_mutex and it can modify the ftrace location also from other
reasons, e.g. to enable tracing.

Therefore we need to always check if the ftrace location is included
in the helper buffer and replace it by the 5-byte nop. It need not be
on the beginning of the instruction if mcount is used.

I am going to prepare v2 of the patch.

Best Regards,
Petr


> Thank you for reporting!
> 
> > 
> > Note that I found this problem when playing with Kprobes. I did it
> > on x86_64 with gcc-4.8.3 that supported -mfentry. I modified
> > samples/kprobes/kprobe_example.c and added offset 5 to put
> > the probe right after the fentry area:
> > 
> > --- cut ---
> >  static struct kprobe kp = {
> >  	.symbol_name	= "do_fork",
> > +	.offset = 5,
> >  };
> > --- cut ---
> > 
> > Then I was able to load kprobe_example before jprobe_example
> > but not the other way around:
> > 
> > $> modprobe jprobe_example
> > $> modprobe kprobe_example
> > modprobe: ERROR: could not insert 'kprobe_example': Invalid or incomplete multibyte or wide character
> > 
> > It did not make much sense and debugging pointed to the bug
> > described above.
> > 
> > Signed-off-by: Petr Mladek <pmladek@suse.cz>
> > ---
> >  arch/x86/kernel/kprobes/core.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> > index 98f654d466e5..ef321caae3ba 100644
> > --- a/arch/x86/kernel/kprobes/core.c
> > +++ b/arch/x86/kernel/kprobes/core.c
> > @@ -225,8 +225,14 @@ __recover_probed_insn(kprobe_opcode_t *buf, unsigned long addr)
> >  	struct kprobe *kp;
> >  
> >  	kp = get_kprobe((void *)addr);
> > -	/* There is no probe, return original address */
> > -	if (!kp)
> > +	/*
> > +	 * Use the current code if it is not modified by Kprobe
> > +	 * or when the Kprobe is using ftrace. In the second case
> > +	 * we do not have any information about the original code
> > +	 * but it is not a problem. Ftrace has put there a valid
> > +	 * instruction of the same length.
> > +	 */
> > +	if (!kp || kprobe_ftrace(kp))
> >  		return addr;
> >  
> >  	/*
> > 
> 
> 
> -- 
> Masami HIRAMATSU
> Software Platform Research Dept. Linux Technology Research Center
> Hitachi, Ltd., Yokohama Research Laboratory
> E-mail: masami.hiramatsu.pt@hitachi.com
> 
> 

Re: [PATCH] kprobes/x86: Use the current code when modified by Kprobes using ftrace

[Petr Mladek]

On Mon 2015-02-02 15:57:20, Petr Mladek wrote:
> On Mon 2015-02-02 17:40:10, Masami Hiramatsu wrote:
> > (2015/01/31 0:45), Petr Mladek wrote:
> > > can_probe() checks if the given address points to the beginning of
> > > an instruction. It analyzes all the instructions from the beginning
> > > of the function until the given address. The code might be modified
> > > by another Kprobe. In this case, the current code is read into a buffer,
> > > int3 breakpoint is replaced by the saved opcode in the buffer, and
> > > can_probe() analyzes the buffer instead.
> > > 
> > > There is a bug that __recover_probed_insn() tries to restore
> > > the original code even for Kprobes using the ftrace framework.
> > > But in this case, the opcode is not stored. See the difference
> > > between arch_prepare_kprobe() and arch_prepare_kprobe_ftrace().
> > > The opcode is stored by arch_copy_kprobe() only from
> > > arch_prepare_kprobe().
> > 
> > Good catch!
> > 
> > > This patch makes Kprobe to use the current code when it is modified
> > > by ftrace. It is not the original one but it is a valid instruction
> > > of the same length. Therefore it is perfectly fine for the check.
> > 
> > Hmm, but there is a chance we can hit it in the period of replacing code.
> > This means we'll see 5byte nop but started with int3. (see P6_NOP5 in
> > arch/x86/include/asm/nops.h)
> > So, I think we'd better return a special buffer which has a 5byte NOP.
> 
> Good point! I though that we were synchronized against ftrace by
> text_mutex and kprobe_mutex but we were not. Ftrace does not take
> text_mutex and it can modify the ftrace location also from other
> reasons, e.g. to enable tracing.
> 
> Therefore we need to always check if the ftrace location is included
> in the helper buffer and replace it by the 5-byte nop. It need not be
> on the beginning of the instruction if mcount is used.

Ah, I thought that we needed to detect the ftrace location anywhere
in the insn buffer because we wanted a valid code there. But it seems
that we are always interested only into the first instruction.
I hope that I get it right. If yes, it will be easy in the end.

Best Regards,
Petr

> I am going to prepare v2 of the patch.
> 
> Best Regards,
> Petr
> 
> 
> > Thank you for reporting!
> > 
> > > 
> > > Note that I found this problem when playing with Kprobes. I did it
> > > on x86_64 with gcc-4.8.3 that supported -mfentry. I modified
> > > samples/kprobes/kprobe_example.c and added offset 5 to put
> > > the probe right after the fentry area:
> > > 
> > > --- cut ---
> > >  static struct kprobe kp = {
> > >  	.symbol_name	= "do_fork",
> > > +	.offset = 5,
> > >  };
> > > --- cut ---
> > > 
> > > Then I was able to load kprobe_example before jprobe_example
> > > but not the other way around:
> > > 
> > > $> modprobe jprobe_example
> > > $> modprobe kprobe_example
> > > modprobe: ERROR: could not insert 'kprobe_example': Invalid or incomplete multibyte or wide character
> > > 
> > > It did not make much sense and debugging pointed to the bug
> > > described above.
> > > 
> > > Signed-off-by: Petr Mladek <pmladek@suse.cz>
> > > ---
> > >  arch/x86/kernel/kprobes/core.c | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> > > index 98f654d466e5..ef321caae3ba 100644
> > > --- a/arch/x86/kernel/kprobes/core.c
> > > +++ b/arch/x86/kernel/kprobes/core.c
> > > @@ -225,8 +225,14 @@ __recover_probed_insn(kprobe_opcode_t *buf, unsigned long addr)
> > >  	struct kprobe *kp;
> > >  
> > >  	kp = get_kprobe((void *)addr);
> > > -	/* There is no probe, return original address */
> > > -	if (!kp)
> > > +	/*
> > > +	 * Use the current code if it is not modified by Kprobe
> > > +	 * or when the Kprobe is using ftrace. In the second case
> > > +	 * we do not have any information about the original code
> > > +	 * but it is not a problem. Ftrace has put there a valid
> > > +	 * instruction of the same length.
> > > +	 */
> > > +	if (!kp || kprobe_ftrace(kp))
> > >  		return addr;
> > >  
> > >  	/*
> > > 
> > 
> > 
> > -- 
> > Masami HIRAMATSU
> > Software Platform Research Dept. Linux Technology Research Center
> > Hitachi, Ltd., Yokohama Research Laboratory
> > E-mail: masami.hiramatsu.pt@hitachi.com
> > 
> >