[PATCH -tip 0/2] kprobes/x86: RO text code bugfix and cleanup

[PATCH -tip 0/2] kprobes/x86: RO text code bugfix and cleanup

[Masami Hiramatsu]

Hi,

This series fixes a kprobe-x86 bug related to RO text and
cleans up addressof operators.

The first one is an obvious bug that misses to set memory
RO when the function fails. I've just add set_memory_ro()
on error exit.

And the second one is just a cleanup patch to remove
addressof operators ("&") since it is meaningless anymore.

Please apply these, since this series is independent from
another bugfix related to optprobe discussing in other
thread.

Thanks,

---

Masami Hiramatsu (2):
      kprobes/x86: Don't forget to set memory back to RO on failure
      kprobes/x86: Remove addressof operators


 arch/x86/include/asm/kprobes.h |    4 ++--
 arch/x86/kernel/kprobes/core.c |    4 +++-
 arch/x86/kernel/kprobes/opt.c  |    9 +++++----
 3 files changed, 10 insertions(+), 7 deletions(-)

--
Masami Hiramatsu <mhiramat@kernel.org>

[PATCH -tip 1/2] kprobes/x86: Don't forget to set memory back to RO on failure

[Masami Hiramatsu]

Do not forget to set kprobes insn buffer memory back
to RO on failure path. Without this fix, if there is
an unexpected error on copying instructions, kprobes
insn buffer kept RW, which can allow unexpected modifying
instruction buffer.

Fixes: d0381c81c2f7 ("kprobes/x86: Set kprobes pages read-only")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/kprobes/core.c |    4 +++-
 arch/x86/kernel/kprobes/opt.c  |    1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index f0153714ddac..b16b10114e20 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -435,8 +435,10 @@ static int arch_copy_kprobe(struct kprobe *p)
 
 	/* Copy an instruction with recovering if other optprobe modifies it.*/
 	len = __copy_instruction(p->ainsn.insn, p->addr, &insn);
-	if (!len)
+	if (!len) {
+		set_memory_ro((unsigned long)p->ainsn.insn & PAGE_MASK, 1);
 		return -EINVAL;
+	}
 
 	/*
 	 * __copy_instruction can modify the displacement of the instruction,
diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 69ea0bc1cfa3..853614560a4f 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -368,6 +368,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op,
 	ret = copy_optimized_instructions(buf + TMPL_END_IDX, op->kp.addr);
 	if (ret < 0) {
 		__arch_remove_optimized_kprobe(op, 0);
+		set_memory_ro((unsigned long)buf & PAGE_MASK, 1);
 		return ret;
 	}
 	op->optinsn.size = ret;

[PATCH -tip 2/2] kprobes/x86: Remove addressof operators

[Masami Hiramatsu]

Since commit 54a7d50b9205 ("x86: mark kprobe templates as
character arrays, not single characters") changes
optprobe_template_* to arrays, we can remove addressof
operators from those symbols.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/include/asm/kprobes.h |    4 ++--
 arch/x86/kernel/kprobes/opt.c  |    8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/kprobes.h b/arch/x86/include/asm/kprobes.h
index 6cf65437b5e5..9f2e3102e0bb 100644
--- a/arch/x86/include/asm/kprobes.h
+++ b/arch/x86/include/asm/kprobes.h
@@ -58,8 +58,8 @@ extern __visible kprobe_opcode_t optprobe_template_call[];
 extern __visible kprobe_opcode_t optprobe_template_end[];
 #define MAX_OPTIMIZED_LENGTH (MAX_INSN_SIZE + RELATIVE_ADDR_SIZE)
 #define MAX_OPTINSN_SIZE 				\
-	(((unsigned long)&optprobe_template_end -	\
-	  (unsigned long)&optprobe_template_entry) +	\
+	(((unsigned long)optprobe_template_end -	\
+	  (unsigned long)optprobe_template_entry) +	\
 	 MAX_OPTIMIZED_LENGTH + RELATIVEJUMP_SIZE)
 
 extern const int kretprobe_blacklist_size;
diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 853614560a4f..021eb6c9dde1 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -141,11 +141,11 @@ void optprobe_template_func(void);
 STACK_FRAME_NON_STANDARD(optprobe_template_func);
 
 #define TMPL_MOVE_IDX \
-	((long)&optprobe_template_val - (long)&optprobe_template_entry)
+	((long)optprobe_template_val - (long)optprobe_template_entry)
 #define TMPL_CALL_IDX \
-	((long)&optprobe_template_call - (long)&optprobe_template_entry)
+	((long)optprobe_template_call - (long)optprobe_template_entry)
 #define TMPL_END_IDX \
-	((long)&optprobe_template_end - (long)&optprobe_template_entry)
+	((long)optprobe_template_end - (long)optprobe_template_entry)
 
 #define INT3_SIZE sizeof(kprobe_opcode_t)
 
@@ -374,7 +374,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op,
 	op->optinsn.size = ret;
 
 	/* Copy arch-dep-instance from template */
-	memcpy(buf, &optprobe_template_entry, TMPL_END_IDX);
+	memcpy(buf, optprobe_template_entry, TMPL_END_IDX);
 
 	/* Set probe information */
 	synthesize_set_arg1(buf + TMPL_MOVE_IDX, (unsigned long)op);

Re: [PATCH -tip 1/2] kprobes/x86: Don't forget to set memory back to RO on failure

[Ingo Molnar]

* Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Do not forget to set kprobes insn buffer memory back
> to RO on failure path. Without this fix, if there is
> an unexpected error on copying instructions, kprobes
> insn buffer kept RW, which can allow unexpected modifying
> instruction buffer.
> 
> Fixes: d0381c81c2f7 ("kprobes/x86: Set kprobes pages read-only")
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  arch/x86/kernel/kprobes/core.c |    4 +++-
>  arch/x86/kernel/kprobes/opt.c  |    1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index f0153714ddac..b16b10114e20 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -435,8 +435,10 @@ static int arch_copy_kprobe(struct kprobe *p)
>  
>  	/* Copy an instruction with recovering if other optprobe modifies it.*/
>  	len = __copy_instruction(p->ainsn.insn, p->addr, &insn);
> -	if (!len)
> +	if (!len) {
> +		set_memory_ro((unsigned long)p->ainsn.insn & PAGE_MASK, 1);
>  		return -EINVAL;
> +	}

So variable usage in the arch_copy_kprobe() is really awful: 'p->ainsn.insn' is 
repeated 6 times!

Please consolidate all that via a helper variable.

Also, regarding the merits of the patch: do we know that the page in question was 
RO before? If it was RW we'll unexpectedly mark it RO here in the failure path ...

> index 69ea0bc1cfa3..853614560a4f 100644
> --- a/arch/x86/kernel/kprobes/opt.c
> +++ b/arch/x86/kernel/kprobes/opt.c
> @@ -368,6 +368,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op,
>  	ret = copy_optimized_instructions(buf + TMPL_END_IDX, op->kp.addr);
>  	if (ret < 0) {
>  		__arch_remove_optimized_kprobe(op, 0);
> +		set_memory_ro((unsigned long)buf & PAGE_MASK, 1);
>  		return ret;
>  	}
>  	op->optinsn.size = ret;

Ditto.

Thanks,

	Ingo

Re: [PATCH -tip 1/2] kprobes/x86: Don't forget to set memory back to RO on failure

[Masami Hiramatsu]

On Thu, 10 Aug 2017 17:29:56 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Do not forget to set kprobes insn buffer memory back
> > to RO on failure path. Without this fix, if there is
> > an unexpected error on copying instructions, kprobes
> > insn buffer kept RW, which can allow unexpected modifying
> > instruction buffer.
> > 
> > Fixes: d0381c81c2f7 ("kprobes/x86: Set kprobes pages read-only")
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  arch/x86/kernel/kprobes/core.c |    4 +++-
> >  arch/x86/kernel/kprobes/opt.c  |    1 +
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> > index f0153714ddac..b16b10114e20 100644
> > --- a/arch/x86/kernel/kprobes/core.c
> > +++ b/arch/x86/kernel/kprobes/core.c
> > @@ -435,8 +435,10 @@ static int arch_copy_kprobe(struct kprobe *p)
> >  
> >  	/* Copy an instruction with recovering if other optprobe modifies it.*/
> >  	len = __copy_instruction(p->ainsn.insn, p->addr, &insn);
> > -	if (!len)
> > +	if (!len) {
> > +		set_memory_ro((unsigned long)p->ainsn.insn & PAGE_MASK, 1);
> >  		return -EINVAL;
> > +	}
> 
> So variable usage in the arch_copy_kprobe() is really awful: 'p->ainsn.insn' is 
> repeated 6 times!
> 
> Please consolidate all that via a helper variable.

OK, I'll cleanup it soon.

> 
> Also, regarding the merits of the patch: do we know that the page in question was 
> RO before? If it was RW we'll unexpectedly mark it RO here in the failure path ...

No need to take care previous state, this page has to be RO after setup (even if
it was failed), since the page is shared by other kprobes. If we missed it,
insn buffers for other kprobes will be left in RW state.

> > index 69ea0bc1cfa3..853614560a4f 100644
> > --- a/arch/x86/kernel/kprobes/opt.c
> > +++ b/arch/x86/kernel/kprobes/opt.c
> > @@ -368,6 +368,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op,
> >  	ret = copy_optimized_instructions(buf + TMPL_END_IDX, op->kp.addr);
> >  	if (ret < 0) {
> >  		__arch_remove_optimized_kprobe(op, 0);
> > +		set_memory_ro((unsigned long)buf & PAGE_MASK, 1);
> >  		return ret;
> >  	}
> >  	op->optinsn.size = ret;
> 
> Ditto.

As same as above, this page is shared by other optprobes.

Thank you,

> 
> Thanks,
> 
> 	Ingo


-- 
Masami Hiramatsu <mhiramat@kernel.org>