* [PATCH -tip v2 0/2] kprobes/x86: RO text code bugfix and cleanup
@ 2017-08-12 0:43 Masami Hiramatsu
2017-08-12 0:44 ` [PATCH -tip v2 1/2] kprobes/x86: Don't forget to set memory back to RO on failure Masami Hiramatsu
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Masami Hiramatsu @ 2017-08-12 0:43 UTC (permalink / raw)
To: Ingo Molnar
Cc: Ingo Molnar, H . Peter Anvin, x86, Masami Hiramatsu,
Ananth N Mavinakayanahalli, Anil S Keshavamurthy,
David S . Miller, linux-kernel
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. And the second one is just a
cleanup patch to remove addressof operators ("&") since
it is meaningless anymore.
V2 has just a following update:
- [1/2] Use a helper variable instead of using p->ainsn.insn
directly.
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 | 15 +++++++++------
arch/x86/kernel/kprobes/opt.c | 9 +++++----
3 files changed, 16 insertions(+), 12 deletions(-)
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH -tip v2 1/2] kprobes/x86: Don't forget to set memory back to RO on failure
2017-08-12 0:43 [PATCH -tip v2 0/2] kprobes/x86: RO text code bugfix and cleanup Masami Hiramatsu
@ 2017-08-12 0:44 ` Masami Hiramatsu
2017-08-12 0:45 ` [PATCH -tip v2 2/2] kprobes/x86: Remove addressof operators Masami Hiramatsu
2017-08-17 9:55 ` [PATCH -tip v2 0/2] kprobes/x86: RO text code bugfix and cleanup Ingo Molnar
2 siblings, 0 replies; 5+ messages in thread
From: Masami Hiramatsu @ 2017-08-12 0:44 UTC (permalink / raw)
To: Ingo Molnar
Cc: Ingo Molnar, H . Peter Anvin, x86, Masami Hiramatsu,
Ananth N Mavinakayanahalli, Anil S Keshavamurthy,
David S . Miller, linux-kernel
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>
---
Changes in v2:
- Use a helper variable instead of using p->ainsn.insn directly.
---
arch/x86/kernel/kprobes/core.c | 15 +++++++++------
arch/x86/kernel/kprobes/opt.c | 1 +
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index f0153714ddac..5d8194b9a068 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -428,15 +428,18 @@ void free_insn_page(void *page)
static int arch_copy_kprobe(struct kprobe *p)
{
+ kprobe_opcode_t *buf = p->ainsn.insn;
struct insn insn;
int len;
- set_memory_rw((unsigned long)p->ainsn.insn & PAGE_MASK, 1);
+ set_memory_rw((unsigned long)buf & PAGE_MASK, 1);
/* Copy an instruction with recovering if other optprobe modifies it.*/
- len = __copy_instruction(p->ainsn.insn, p->addr, &insn);
- if (!len)
+ len = __copy_instruction(buf, p->addr, &insn);
+ if (!len) {
+ set_memory_ro((unsigned long)buf & PAGE_MASK, 1);
return -EINVAL;
+ }
/*
* __copy_instruction can modify the displacement of the instruction,
@@ -444,13 +447,13 @@ static int arch_copy_kprobe(struct kprobe *p)
*/
prepare_boost(p, &insn);
- set_memory_ro((unsigned long)p->ainsn.insn & PAGE_MASK, 1);
+ set_memory_ro((unsigned long)buf & PAGE_MASK, 1);
/* Check whether the instruction modifies Interrupt Flag or not */
- p->ainsn.if_modifier = is_IF_modifier(p->ainsn.insn);
+ p->ainsn.if_modifier = is_IF_modifier(buf);
/* Also, displacement change doesn't affect the first byte */
- p->opcode = p->ainsn.insn[0];
+ p->opcode = buf[0];
return 0;
}
diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 4f98aad38237..86b9a883b712 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -371,6 +371,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;
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH -tip v2 2/2] kprobes/x86: Remove addressof operators
2017-08-12 0:43 [PATCH -tip v2 0/2] kprobes/x86: RO text code bugfix and cleanup Masami Hiramatsu
2017-08-12 0:44 ` [PATCH -tip v2 1/2] kprobes/x86: Don't forget to set memory back to RO on failure Masami Hiramatsu
@ 2017-08-12 0:45 ` Masami Hiramatsu
2017-08-17 9:55 ` [PATCH -tip v2 0/2] kprobes/x86: RO text code bugfix and cleanup Ingo Molnar
2 siblings, 0 replies; 5+ messages in thread
From: Masami Hiramatsu @ 2017-08-12 0:45 UTC (permalink / raw)
To: Ingo Molnar
Cc: Ingo Molnar, H . Peter Anvin, x86, Masami Hiramatsu,
Ananth N Mavinakayanahalli, Anil S Keshavamurthy,
David S . Miller, linux-kernel
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 86b9a883b712..d85fac42b512 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -142,11 +142,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)
@@ -377,7 +377,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);
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH -tip v2 0/2] kprobes/x86: RO text code bugfix and cleanup
2017-08-12 0:43 [PATCH -tip v2 0/2] kprobes/x86: RO text code bugfix and cleanup Masami Hiramatsu
2017-08-12 0:44 ` [PATCH -tip v2 1/2] kprobes/x86: Don't forget to set memory back to RO on failure Masami Hiramatsu
2017-08-12 0:45 ` [PATCH -tip v2 2/2] kprobes/x86: Remove addressof operators Masami Hiramatsu
@ 2017-08-17 9:55 ` Ingo Molnar
2017-08-17 15:07 ` Masami Hiramatsu
2 siblings, 1 reply; 5+ messages in thread
From: Ingo Molnar @ 2017-08-17 9:55 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Ingo Molnar, H . Peter Anvin, x86, Ananth N Mavinakayanahalli,
Anil S Keshavamurthy, David S . Miller, linux-kernel
* Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 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. And the second one is just a
> cleanup patch to remove addressof operators ("&") since
> it is meaningless anymore.
>
> V2 has just a following update:
> - [1/2] Use a helper variable instead of using p->ainsn.insn
> directly.
>
> 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 | 15 +++++++++------
> arch/x86/kernel/kprobes/opt.c | 9 +++++----
> 3 files changed, 16 insertions(+), 12 deletions(-)
So I'm totally opposed to the whole approach of modifying the permissions of the
kernel text virtual memory area.
Firstly, it's racy against other kernel subsystems: what happens if some other
code patching mechanism does a similar 'mark RWX and then back to RX'? Who
provides the synchronization? set_memory_*() certainly does not.
Secondly, it's racy against attackers: if an attacker can time the attack to the
time when a kprobe is installed, then the kernel is still vulnerable.
So how about avoiding the problem altogether by patching the kernel not in its
virtual text address, but in the direct mappings? Then page permissions won't have
to be modified, and the whole solution will be more robust and secure.
Is there anything I'm missing?
Thanks,
Ingo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH -tip v2 0/2] kprobes/x86: RO text code bugfix and cleanup
2017-08-17 9:55 ` [PATCH -tip v2 0/2] kprobes/x86: RO text code bugfix and cleanup Ingo Molnar
@ 2017-08-17 15:07 ` Masami Hiramatsu
0 siblings, 0 replies; 5+ messages in thread
From: Masami Hiramatsu @ 2017-08-17 15:07 UTC (permalink / raw)
To: Ingo Molnar
Cc: Ingo Molnar, H . Peter Anvin, x86, Ananth N Mavinakayanahalli,
Anil S Keshavamurthy, David S . Miller, linux-kernel
On Thu, 17 Aug 2017 11:55:30 +0200
Ingo Molnar <mingo@kernel.org> wrote:
>
> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> > 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. And the second one is just a
> > cleanup patch to remove addressof operators ("&") since
> > it is meaningless anymore.
> >
> > V2 has just a following update:
> > - [1/2] Use a helper variable instead of using p->ainsn.insn
> > directly.
> >
> > 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 | 15 +++++++++------
> > arch/x86/kernel/kprobes/opt.c | 9 +++++----
> > 3 files changed, 16 insertions(+), 12 deletions(-)
>
> So I'm totally opposed to the whole approach of modifying the permissions of the
> kernel text virtual memory area.
>
> Firstly, it's racy against other kernel subsystems: what happens if some other
> code patching mechanism does a similar 'mark RWX and then back to RX'? Who
> provides the synchronization? set_memory_*() certainly does not.
Hmm, this sounds common problem for set_memory_*() users.
> Secondly, it's racy against attackers: if an attacker can time the attack to the
> time when a kprobe is installed, then the kernel is still vulnerable.
Right, since this is not against for attackers, but for some unexpected memory
corruption bugs. Yes, this is vulnerable against attackers.
> So how about avoiding the problem altogether by patching the kernel not in its
> virtual text address, but in the direct mappings? Then page permissions won't have
> to be modified, and the whole solution will be more robust and secure.
So would you mean using text_poke()?
OK, that's a good idea. I'll try to rewrite it again with text_poke().
Thank you!
>
> Is there anything I'm missing?
>
> Thanks,
>
> Ingo
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-08-17 15:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-12 0:43 [PATCH -tip v2 0/2] kprobes/x86: RO text code bugfix and cleanup Masami Hiramatsu
2017-08-12 0:44 ` [PATCH -tip v2 1/2] kprobes/x86: Don't forget to set memory back to RO on failure Masami Hiramatsu
2017-08-12 0:45 ` [PATCH -tip v2 2/2] kprobes/x86: Remove addressof operators Masami Hiramatsu
2017-08-17 9:55 ` [PATCH -tip v2 0/2] kprobes/x86: RO text code bugfix and cleanup Ingo Molnar
2017-08-17 15:07 ` Masami Hiramatsu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).