linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] arm64: kprobe: make page to RO mode when allocate it
@ 2018-10-30 11:38 Anders Roxell
  2018-10-30 11:45 ` Will Deacon
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Anders Roxell @ 2018-10-30 11:38 UTC (permalink / raw)
  To: catalin.marinas, will.deacon
  Cc: linux-arm-kernel, linux-kernel, masami.hiramatsu, Anders Roxell,
	Laura Abbott, Arnd Bergmann, Ard Biesheuvel

Commit 1404d6f13e47 ("arm64: dump: Add checking for writable and exectuable pages")
has successfully identified code that leaves a page with W+X
permissions.

[    3.245140] arm64/mm: Found insecure W+X mapping at address (____ptrval____)/0xffff000000d90000
[    3.245771] WARNING: CPU: 0 PID: 1 at ../arch/arm64/mm/dump.c:232 note_page+0x410/0x420
[    3.246141] Modules linked in:
[    3.246653] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.19.0-rc5-next-20180928-00001-ge70ae259b853-dirty #62
[    3.247008] Hardware name: linux,dummy-virt (DT)
[    3.247347] pstate: 80000005 (Nzcv daif -PAN -UAO)
[    3.247623] pc : note_page+0x410/0x420
[    3.247898] lr : note_page+0x410/0x420
[    3.248071] sp : ffff00000804bcd0
[    3.248254] x29: ffff00000804bcd0 x28: ffff000009274000
[    3.248578] x27: ffff00000921a000 x26: ffff80007dfff000
[    3.248845] x25: ffff0000093f5000 x24: ffff000009526f6a
[    3.249109] x23: 0000000000000004 x22: ffff000000d91000
[    3.249396] x21: ffff000000d90000 x20: 0000000000000000
[    3.249661] x19: ffff00000804bde8 x18: 0000000000000400
[    3.249924] x17: 0000000000000000 x16: 0000000000000000
[    3.250271] x15: ffffffffffffffff x14: 295f5f5f5f6c6176
[    3.250594] x13: 7274705f5f5f5f28 x12: 2073736572646461
[    3.250941] x11: 20746120676e6970 x10: 70616d20582b5720
[    3.251252] x9 : 6572756365736e69 x8 : 3039643030303030
[    3.251519] x7 : 306666666678302f x6 : ffff0000095467b2
[    3.251802] x5 : 0000000000000000 x4 : 0000000000000000
[    3.252060] x3 : 0000000000000000 x2 : ffffffffffffffff
[    3.252323] x1 : 4d151327adc50b00 x0 : 0000000000000000
[    3.252664] Call trace:
[    3.252953]  note_page+0x410/0x420
[    3.253186]  walk_pgd+0x12c/0x238
[    3.253417]  ptdump_check_wx+0x68/0xf8
[    3.253637]  mark_rodata_ro+0x68/0x98
[    3.253847]  kernel_init+0x38/0x160
[    3.254103]  ret_from_fork+0x10/0x18

kprobes allocates a writable executable page with module_alloc() in
order to store executable code.
Reworked to that when allocate a page it sets mode RO. Inspired by
commit 63fef14fc98a ("kprobes/x86: Make insn buffer always ROX and use text_poke()").

Cc: Laura Abbott <labbott@redhat.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Co-developed-by: Arnd Bergmann <arnd@arndb.de>
Co-developed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
---
 arch/arm64/kernel/probes/kprobes.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index 9b65132e789a..decf483b4153 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -23,7 +23,9 @@
 #include <linux/slab.h>
 #include <linux/stop_machine.h>
 #include <linux/sched/debug.h>
+#include <linux/set_memory.h>
 #include <linux/stringify.h>
+#include <linux/vmalloc.h>
 #include <asm/traps.h>
 #include <asm/ptrace.h>
 #include <asm/cacheflush.h>
@@ -42,10 +44,21 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 static void __kprobes
 post_kprobe_handler(struct kprobe_ctlblk *, struct pt_regs *);
 
+static int __kprobes patch_text(kprobe_opcode_t *addr, u32 opcode)
+{
+	void *addrs[1];
+	u32 insns[1];
+
+	addrs[0] = (void *)addr;
+	insns[0] = (u32)opcode;
+
+	return aarch64_insn_patch_text(addrs, insns, 1);
+}
+
 static void __kprobes arch_prepare_ss_slot(struct kprobe *p)
 {
 	/* prepare insn slot */
-	p->ainsn.api.insn[0] = cpu_to_le32(p->opcode);
+	patch_text(p->ainsn.api.insn, p->opcode);
 
 	flush_icache_range((uintptr_t) (p->ainsn.api.insn),
 			   (uintptr_t) (p->ainsn.api.insn) +
@@ -118,15 +131,15 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 	return 0;
 }
 
-static int __kprobes patch_text(kprobe_opcode_t *addr, u32 opcode)
+void *alloc_insn_page(void)
 {
-	void *addrs[1];
-	u32 insns[1];
+	void *page;
 
-	addrs[0] = (void *)addr;
-	insns[0] = (u32)opcode;
+	page = vmalloc_exec(PAGE_SIZE);
+	if (page)
+		set_memory_ro((unsigned long)page, 1);
 
-	return aarch64_insn_patch_text(addrs, insns, 1);
+	return page;
 }
 
 /* arm kprobe: install breakpoint in text */
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] arm64: kprobe: make page to RO mode when allocate it
  2018-10-30 11:38 [PATCH v2] arm64: kprobe: make page to RO mode when allocate it Anders Roxell
@ 2018-10-30 11:45 ` Will Deacon
  2018-10-30 11:49 ` Ard Biesheuvel
  2018-11-02 17:31 ` Catalin Marinas
  2 siblings, 0 replies; 7+ messages in thread
From: Will Deacon @ 2018-10-30 11:45 UTC (permalink / raw)
  To: Anders Roxell
  Cc: catalin.marinas, linux-arm-kernel, linux-kernel,
	masami.hiramatsu, Laura Abbott, Arnd Bergmann, Ard Biesheuvel

On Tue, Oct 30, 2018 at 12:38:50PM +0100, Anders Roxell wrote:
> Commit 1404d6f13e47 ("arm64: dump: Add checking for writable and exectuable pages")
> has successfully identified code that leaves a page with W+X
> permissions.
> 
> [    3.245140] arm64/mm: Found insecure W+X mapping at address (____ptrval____)/0xffff000000d90000
> [    3.245771] WARNING: CPU: 0 PID: 1 at ../arch/arm64/mm/dump.c:232 note_page+0x410/0x420
> [    3.246141] Modules linked in:
> [    3.246653] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.19.0-rc5-next-20180928-00001-ge70ae259b853-dirty #62
> [    3.247008] Hardware name: linux,dummy-virt (DT)
> [    3.247347] pstate: 80000005 (Nzcv daif -PAN -UAO)
> [    3.247623] pc : note_page+0x410/0x420
> [    3.247898] lr : note_page+0x410/0x420
> [    3.248071] sp : ffff00000804bcd0
> [    3.248254] x29: ffff00000804bcd0 x28: ffff000009274000
> [    3.248578] x27: ffff00000921a000 x26: ffff80007dfff000
> [    3.248845] x25: ffff0000093f5000 x24: ffff000009526f6a
> [    3.249109] x23: 0000000000000004 x22: ffff000000d91000
> [    3.249396] x21: ffff000000d90000 x20: 0000000000000000
> [    3.249661] x19: ffff00000804bde8 x18: 0000000000000400
> [    3.249924] x17: 0000000000000000 x16: 0000000000000000
> [    3.250271] x15: ffffffffffffffff x14: 295f5f5f5f6c6176
> [    3.250594] x13: 7274705f5f5f5f28 x12: 2073736572646461
> [    3.250941] x11: 20746120676e6970 x10: 70616d20582b5720
> [    3.251252] x9 : 6572756365736e69 x8 : 3039643030303030
> [    3.251519] x7 : 306666666678302f x6 : ffff0000095467b2
> [    3.251802] x5 : 0000000000000000 x4 : 0000000000000000
> [    3.252060] x3 : 0000000000000000 x2 : ffffffffffffffff
> [    3.252323] x1 : 4d151327adc50b00 x0 : 0000000000000000
> [    3.252664] Call trace:
> [    3.252953]  note_page+0x410/0x420
> [    3.253186]  walk_pgd+0x12c/0x238
> [    3.253417]  ptdump_check_wx+0x68/0xf8
> [    3.253637]  mark_rodata_ro+0x68/0x98
> [    3.253847]  kernel_init+0x38/0x160
> [    3.254103]  ret_from_fork+0x10/0x18
> 
> kprobes allocates a writable executable page with module_alloc() in
> order to store executable code.
> Reworked to that when allocate a page it sets mode RO. Inspired by
> commit 63fef14fc98a ("kprobes/x86: Make insn buffer always ROX and use text_poke()").
> 
> Cc: Laura Abbott <labbott@redhat.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Co-developed-by: Arnd Bergmann <arnd@arndb.de>
> Co-developed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> ---
>  arch/arm64/kernel/probes/kprobes.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index 9b65132e789a..decf483b4153 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -23,7 +23,9 @@
>  #include <linux/slab.h>
>  #include <linux/stop_machine.h>
>  #include <linux/sched/debug.h>
> +#include <linux/set_memory.h>
>  #include <linux/stringify.h>
> +#include <linux/vmalloc.h>
>  #include <asm/traps.h>
>  #include <asm/ptrace.h>
>  #include <asm/cacheflush.h>
> @@ -42,10 +44,21 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
>  static void __kprobes
>  post_kprobe_handler(struct kprobe_ctlblk *, struct pt_regs *);
>  
> +static int __kprobes patch_text(kprobe_opcode_t *addr, u32 opcode)
> +{
> +	void *addrs[1];
> +	u32 insns[1];
> +
> +	addrs[0] = (void *)addr;
> +	insns[0] = (u32)opcode;

I know they exist already, but I think you can drop these casts (Catalin can
do it when he picks this up -- no need to respin).

With that:

Acked-by: Will Deacon <will.deacon@arm.com>

Thanks for respinning so quickly.

Will

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] arm64: kprobe: make page to RO mode when allocate it
  2018-10-30 11:38 [PATCH v2] arm64: kprobe: make page to RO mode when allocate it Anders Roxell
  2018-10-30 11:45 ` Will Deacon
@ 2018-10-30 11:49 ` Ard Biesheuvel
  2018-10-30 14:10   ` Ard Biesheuvel
  2018-11-02 17:31 ` Catalin Marinas
  2 siblings, 1 reply; 7+ messages in thread
From: Ard Biesheuvel @ 2018-10-30 11:49 UTC (permalink / raw)
  To: Anders Roxell
  Cc: catalin.marinas, will.deacon, linux-arm-kernel, linux-kernel,
	masami.hiramatsu, Laura Abbott, Arnd Bergmann

Hi Anders,

> On 30 Oct 2018, at 08:38, Anders Roxell <anders.roxell@linaro.org> wrote:
> 
> Commit 1404d6f13e47 ("arm64: dump: Add checking for writable and exectuable pages")
> has successfully identified code that leaves a page with W+X
> permissions.
> 
> [    3.245140] arm64/mm: Found insecure W+X mapping at address (____ptrval____)/0xffff000000d90000
> [    3.245771] WARNING: CPU: 0 PID: 1 at ../arch/arm64/mm/dump.c:232 note_page+0x410/0x420
> [    3.246141] Modules linked in:
> [    3.246653] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.19.0-rc5-next-20180928-00001-ge70ae259b853-dirty #62
> [    3.247008] Hardware name: linux,dummy-virt (DT)
> [    3.247347] pstate: 80000005 (Nzcv daif -PAN -UAO)
> [    3.247623] pc : note_page+0x410/0x420
> [    3.247898] lr : note_page+0x410/0x420
> [    3.248071] sp : ffff00000804bcd0
> [    3.248254] x29: ffff00000804bcd0 x28: ffff000009274000
> [    3.248578] x27: ffff00000921a000 x26: ffff80007dfff000
> [    3.248845] x25: ffff0000093f5000 x24: ffff000009526f6a
> [    3.249109] x23: 0000000000000004 x22: ffff000000d91000
> [    3.249396] x21: ffff000000d90000 x20: 0000000000000000
> [    3.249661] x19: ffff00000804bde8 x18: 0000000000000400
> [    3.249924] x17: 0000000000000000 x16: 0000000000000000
> [    3.250271] x15: ffffffffffffffff x14: 295f5f5f5f6c6176
> [    3.250594] x13: 7274705f5f5f5f28 x12: 2073736572646461
> [    3.250941] x11: 20746120676e6970 x10: 70616d20582b5720
> [    3.251252] x9 : 6572756365736e69 x8 : 3039643030303030
> [    3.251519] x7 : 306666666678302f x6 : ffff0000095467b2
> [    3.251802] x5 : 0000000000000000 x4 : 0000000000000000
> [    3.252060] x3 : 0000000000000000 x2 : ffffffffffffffff
> [    3.252323] x1 : 4d151327adc50b00 x0 : 0000000000000000
> [    3.252664] Call trace:
> [    3.252953]  note_page+0x410/0x420
> [    3.253186]  walk_pgd+0x12c/0x238
> [    3.253417]  ptdump_check_wx+0x68/0xf8
> [    3.253637]  mark_rodata_ro+0x68/0x98
> [    3.253847]  kernel_init+0x38/0x160
> [    3.254103]  ret_from_fork+0x10/0x18
> 
> kprobes allocates a writable executable page with module_alloc() in
> order to store executable code.
> Reworked to that when allocate a page it sets mode RO. Inspired by
> commit 63fef14fc98a ("kprobes/x86: Make insn buffer always ROX and use text_poke()").
> 
> Cc: Laura Abbott <labbott@redhat.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Co-developed-by: Arnd Bergmann <arnd@arndb.de>
> Co-developed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Please remove these SOBs, Arnd and I provided input to this patch but you are the one sending it (sob does not assert authorship or anything like that, it just asserts that the code in the patch was made available under a compatible license)

Also, please add the acks you received from Masami and Laura.

> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> ---
> arch/arm64/kernel/probes/kprobes.c | 27 ++++++++++++++++++++-------
> 1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index 9b65132e789a..decf483b4153 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -23,7 +23,9 @@
> #include <linux/slab.h>
> #include <linux/stop_machine.h>
> #include <linux/sched/debug.h>
> +#include <linux/set_memory.h>
> #include <linux/stringify.h>
> +#include <linux/vmalloc.h>
> #include <asm/traps.h>
> #include <asm/ptrace.h>
> #include <asm/cacheflush.h>
> @@ -42,10 +44,21 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
> static void __kprobes
> post_kprobe_handler(struct kprobe_ctlblk *, struct pt_regs *);
> 
> +static int __kprobes patch_text(kprobe_opcode_t *addr, u32 opcode)
> +{
> +    void *addrs[1];
> +    u32 insns[1];
> +
> +    addrs[0] = (void *)addr;
> +    insns[0] = (u32)opcode;
> +
> +    return aarch64_insn_patch_text(addrs, insns, 1);
> +}
> +
> static void __kprobes arch_prepare_ss_slot(struct kprobe *p)
> {
>    /* prepare insn slot */
> -    p->ainsn.api.insn[0] = cpu_to_le32(p->opcode);
> +    patch_text(p->ainsn.api.insn, p->opcode);
> 
>    flush_icache_range((uintptr_t) (p->ainsn.api.insn),
>               (uintptr_t) (p->ainsn.api.insn) +
> @@ -118,15 +131,15 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
>    return 0;
> }
> 
> -static int __kprobes patch_text(kprobe_opcode_t *addr, u32 opcode)
> +void *alloc_insn_page(void)
> {
> -    void *addrs[1];
> -    u32 insns[1];
> +    void *page;
> 
> -    addrs[0] = (void *)addr;
> -    insns[0] = (u32)opcode;
> +    page = vmalloc_exec(PAGE_SIZE);
> +    if (page)
> +        set_memory_ro((unsigned long)page, 1);
> 
> -    return aarch64_insn_patch_text(addrs, insns, 1);
> +    return page;
> }
> 
> /* arm kprobe: install breakpoint in text */
> -- 
> 2.19.1
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] arm64: kprobe: make page to RO mode when allocate it
  2018-10-30 11:49 ` Ard Biesheuvel
@ 2018-10-30 14:10   ` Ard Biesheuvel
  2018-11-02 17:40     ` Catalin Marinas
  0 siblings, 1 reply; 7+ messages in thread
From: Ard Biesheuvel @ 2018-10-30 14:10 UTC (permalink / raw)
  To: Anders Roxell
  Cc: Catalin Marinas, Will Deacon, linux-arm-kernel,
	Linux Kernel Mailing List, Masami Hiramatsu, Laura Abbott,
	Arnd Bergmann

On 30 October 2018 at 08:49, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Hi Anders,
>
>> On 30 Oct 2018, at 08:38, Anders Roxell <anders.roxell@linaro.org> wrote:
>>
>> Commit 1404d6f13e47 ("arm64: dump: Add checking for writable and exectuable pages")
>> has successfully identified code that leaves a page with W+X
>> permissions.
>>
>> [    3.245140] arm64/mm: Found insecure W+X mapping at address (____ptrval____)/0xffff000000d90000
>> [    3.245771] WARNING: CPU: 0 PID: 1 at ../arch/arm64/mm/dump.c:232 note_page+0x410/0x420
>> [    3.246141] Modules linked in:
>> [    3.246653] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.19.0-rc5-next-20180928-00001-ge70ae259b853-dirty #62
>> [    3.247008] Hardware name: linux,dummy-virt (DT)
>> [    3.247347] pstate: 80000005 (Nzcv daif -PAN -UAO)
>> [    3.247623] pc : note_page+0x410/0x420
>> [    3.247898] lr : note_page+0x410/0x420
>> [    3.248071] sp : ffff00000804bcd0
>> [    3.248254] x29: ffff00000804bcd0 x28: ffff000009274000
>> [    3.248578] x27: ffff00000921a000 x26: ffff80007dfff000
>> [    3.248845] x25: ffff0000093f5000 x24: ffff000009526f6a
>> [    3.249109] x23: 0000000000000004 x22: ffff000000d91000
>> [    3.249396] x21: ffff000000d90000 x20: 0000000000000000
>> [    3.249661] x19: ffff00000804bde8 x18: 0000000000000400
>> [    3.249924] x17: 0000000000000000 x16: 0000000000000000
>> [    3.250271] x15: ffffffffffffffff x14: 295f5f5f5f6c6176
>> [    3.250594] x13: 7274705f5f5f5f28 x12: 2073736572646461
>> [    3.250941] x11: 20746120676e6970 x10: 70616d20582b5720
>> [    3.251252] x9 : 6572756365736e69 x8 : 3039643030303030
>> [    3.251519] x7 : 306666666678302f x6 : ffff0000095467b2
>> [    3.251802] x5 : 0000000000000000 x4 : 0000000000000000
>> [    3.252060] x3 : 0000000000000000 x2 : ffffffffffffffff
>> [    3.252323] x1 : 4d151327adc50b00 x0 : 0000000000000000
>> [    3.252664] Call trace:
>> [    3.252953]  note_page+0x410/0x420
>> [    3.253186]  walk_pgd+0x12c/0x238
>> [    3.253417]  ptdump_check_wx+0x68/0xf8
>> [    3.253637]  mark_rodata_ro+0x68/0x98
>> [    3.253847]  kernel_init+0x38/0x160
>> [    3.254103]  ret_from_fork+0x10/0x18
>>
>> kprobes allocates a writable executable page with module_alloc() in
>> order to store executable code.
>> Reworked to that when allocate a page it sets mode RO. Inspired by
>> commit 63fef14fc98a ("kprobes/x86: Make insn buffer always ROX and use text_poke()").
>>
>> Cc: Laura Abbott <labbott@redhat.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Co-developed-by: Arnd Bergmann <arnd@arndb.de>
>> Co-developed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Please remove these SOBs, Arnd and I provided input to this patch but you are the one sending it (sob does not assert authorship or anything like that, it just asserts that the code in the patch was made available under a compatible license)
>

As Anders points out in a private communication, the Documentation/
explicitly requires signoffs for Co-developed-by credits. Perhaps we
should enhance that document to clarify that that does not mean you
can simply add signoffs on someone else's behalf.

But the patch is fine as it stands (with the received acks added)

> Also, please add the acks you received from Masami and Laura.
>
>> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
>> ---
>> arch/arm64/kernel/probes/kprobes.c | 27 ++++++++++++++++++++-------
>> 1 file changed, 20 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
>> index 9b65132e789a..decf483b4153 100644
>> --- a/arch/arm64/kernel/probes/kprobes.c
>> +++ b/arch/arm64/kernel/probes/kprobes.c
>> @@ -23,7 +23,9 @@
>> #include <linux/slab.h>
>> #include <linux/stop_machine.h>
>> #include <linux/sched/debug.h>
>> +#include <linux/set_memory.h>
>> #include <linux/stringify.h>
>> +#include <linux/vmalloc.h>
>> #include <asm/traps.h>
>> #include <asm/ptrace.h>
>> #include <asm/cacheflush.h>
>> @@ -42,10 +44,21 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
>> static void __kprobes
>> post_kprobe_handler(struct kprobe_ctlblk *, struct pt_regs *);
>>
>> +static int __kprobes patch_text(kprobe_opcode_t *addr, u32 opcode)
>> +{
>> +    void *addrs[1];
>> +    u32 insns[1];
>> +
>> +    addrs[0] = (void *)addr;
>> +    insns[0] = (u32)opcode;
>> +
>> +    return aarch64_insn_patch_text(addrs, insns, 1);
>> +}
>> +
>> static void __kprobes arch_prepare_ss_slot(struct kprobe *p)
>> {
>>    /* prepare insn slot */
>> -    p->ainsn.api.insn[0] = cpu_to_le32(p->opcode);
>> +    patch_text(p->ainsn.api.insn, p->opcode);
>>
>>    flush_icache_range((uintptr_t) (p->ainsn.api.insn),
>>               (uintptr_t) (p->ainsn.api.insn) +
>> @@ -118,15 +131,15 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
>>    return 0;
>> }
>>
>> -static int __kprobes patch_text(kprobe_opcode_t *addr, u32 opcode)
>> +void *alloc_insn_page(void)
>> {
>> -    void *addrs[1];
>> -    u32 insns[1];
>> +    void *page;
>>
>> -    addrs[0] = (void *)addr;
>> -    insns[0] = (u32)opcode;
>> +    page = vmalloc_exec(PAGE_SIZE);
>> +    if (page)
>> +        set_memory_ro((unsigned long)page, 1);
>>
>> -    return aarch64_insn_patch_text(addrs, insns, 1);
>> +    return page;
>> }
>>
>> /* arm kprobe: install breakpoint in text */
>> --
>> 2.19.1
>>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] arm64: kprobe: make page to RO mode when allocate it
  2018-10-30 11:38 [PATCH v2] arm64: kprobe: make page to RO mode when allocate it Anders Roxell
  2018-10-30 11:45 ` Will Deacon
  2018-10-30 11:49 ` Ard Biesheuvel
@ 2018-11-02 17:31 ` Catalin Marinas
  2 siblings, 0 replies; 7+ messages in thread
From: Catalin Marinas @ 2018-11-02 17:31 UTC (permalink / raw)
  To: Anders Roxell
  Cc: will.deacon, masami.hiramatsu, Arnd Bergmann, Ard Biesheuvel,
	linux-kernel, Laura Abbott, linux-arm-kernel

On Tue, Oct 30, 2018 at 12:38:50PM +0100, Anders Roxell wrote:
> Commit 1404d6f13e47 ("arm64: dump: Add checking for writable and exectuable pages")
> has successfully identified code that leaves a page with W+X
> permissions.
> 
> [    3.245140] arm64/mm: Found insecure W+X mapping at address (____ptrval____)/0xffff000000d90000
> [    3.245771] WARNING: CPU: 0 PID: 1 at ../arch/arm64/mm/dump.c:232 note_page+0x410/0x420
> [    3.246141] Modules linked in:
> [    3.246653] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.19.0-rc5-next-20180928-00001-ge70ae259b853-dirty #62
> [    3.247008] Hardware name: linux,dummy-virt (DT)
> [    3.247347] pstate: 80000005 (Nzcv daif -PAN -UAO)
> [    3.247623] pc : note_page+0x410/0x420
> [    3.247898] lr : note_page+0x410/0x420
> [    3.248071] sp : ffff00000804bcd0
> [    3.248254] x29: ffff00000804bcd0 x28: ffff000009274000
> [    3.248578] x27: ffff00000921a000 x26: ffff80007dfff000
> [    3.248845] x25: ffff0000093f5000 x24: ffff000009526f6a
> [    3.249109] x23: 0000000000000004 x22: ffff000000d91000
> [    3.249396] x21: ffff000000d90000 x20: 0000000000000000
> [    3.249661] x19: ffff00000804bde8 x18: 0000000000000400
> [    3.249924] x17: 0000000000000000 x16: 0000000000000000
> [    3.250271] x15: ffffffffffffffff x14: 295f5f5f5f6c6176
> [    3.250594] x13: 7274705f5f5f5f28 x12: 2073736572646461
> [    3.250941] x11: 20746120676e6970 x10: 70616d20582b5720
> [    3.251252] x9 : 6572756365736e69 x8 : 3039643030303030
> [    3.251519] x7 : 306666666678302f x6 : ffff0000095467b2
> [    3.251802] x5 : 0000000000000000 x4 : 0000000000000000
> [    3.252060] x3 : 0000000000000000 x2 : ffffffffffffffff
> [    3.252323] x1 : 4d151327adc50b00 x0 : 0000000000000000
> [    3.252664] Call trace:
> [    3.252953]  note_page+0x410/0x420
> [    3.253186]  walk_pgd+0x12c/0x238
> [    3.253417]  ptdump_check_wx+0x68/0xf8
> [    3.253637]  mark_rodata_ro+0x68/0x98
> [    3.253847]  kernel_init+0x38/0x160
> [    3.254103]  ret_from_fork+0x10/0x18
> 
> kprobes allocates a writable executable page with module_alloc() in
> order to store executable code.
> Reworked to that when allocate a page it sets mode RO. Inspired by
> commit 63fef14fc98a ("kprobes/x86: Make insn buffer always ROX and use text_poke()").
> 
> Cc: Laura Abbott <labbott@redhat.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Co-developed-by: Arnd Bergmann <arnd@arndb.de>
> Co-developed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>

Queued for 4.20 (and removed the signed-off-bys, added acks, removed
unnecessary casts). Thanks.

-- 
Catalin

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] arm64: kprobe: make page to RO mode when allocate it
  2018-10-30 14:10   ` Ard Biesheuvel
@ 2018-11-02 17:40     ` Catalin Marinas
  2018-11-02 18:29       ` Ard Biesheuvel
  0 siblings, 1 reply; 7+ messages in thread
From: Catalin Marinas @ 2018-11-02 17:40 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Anders Roxell, Arnd Bergmann, Masami Hiramatsu, Will Deacon,
	Linux Kernel Mailing List, Laura Abbott, linux-arm-kernel

On Tue, Oct 30, 2018 at 11:10:51AM -0300, Ard Biesheuvel wrote:
> On 30 October 2018 at 08:49, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >> On 30 Oct 2018, at 08:38, Anders Roxell <anders.roxell@linaro.org> wrote:
> >>
> >> Commit 1404d6f13e47 ("arm64: dump: Add checking for writable and exectuable pages")
> >> has successfully identified code that leaves a page with W+X
> >> permissions.
> >>
> >> [    3.245140] arm64/mm: Found insecure W+X mapping at address (____ptrval____)/0xffff000000d90000
> >> [    3.245771] WARNING: CPU: 0 PID: 1 at ../arch/arm64/mm/dump.c:232 note_page+0x410/0x420
> >> [    3.246141] Modules linked in:
> >> [    3.246653] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.19.0-rc5-next-20180928-00001-ge70ae259b853-dirty #62
> >> [    3.247008] Hardware name: linux,dummy-virt (DT)
> >> [    3.247347] pstate: 80000005 (Nzcv daif -PAN -UAO)
> >> [    3.247623] pc : note_page+0x410/0x420
> >> [    3.247898] lr : note_page+0x410/0x420
> >> [    3.248071] sp : ffff00000804bcd0
> >> [    3.248254] x29: ffff00000804bcd0 x28: ffff000009274000
> >> [    3.248578] x27: ffff00000921a000 x26: ffff80007dfff000
> >> [    3.248845] x25: ffff0000093f5000 x24: ffff000009526f6a
> >> [    3.249109] x23: 0000000000000004 x22: ffff000000d91000
> >> [    3.249396] x21: ffff000000d90000 x20: 0000000000000000
> >> [    3.249661] x19: ffff00000804bde8 x18: 0000000000000400
> >> [    3.249924] x17: 0000000000000000 x16: 0000000000000000
> >> [    3.250271] x15: ffffffffffffffff x14: 295f5f5f5f6c6176
> >> [    3.250594] x13: 7274705f5f5f5f28 x12: 2073736572646461
> >> [    3.250941] x11: 20746120676e6970 x10: 70616d20582b5720
> >> [    3.251252] x9 : 6572756365736e69 x8 : 3039643030303030
> >> [    3.251519] x7 : 306666666678302f x6 : ffff0000095467b2
> >> [    3.251802] x5 : 0000000000000000 x4 : 0000000000000000
> >> [    3.252060] x3 : 0000000000000000 x2 : ffffffffffffffff
> >> [    3.252323] x1 : 4d151327adc50b00 x0 : 0000000000000000
> >> [    3.252664] Call trace:
> >> [    3.252953]  note_page+0x410/0x420
> >> [    3.253186]  walk_pgd+0x12c/0x238
> >> [    3.253417]  ptdump_check_wx+0x68/0xf8
> >> [    3.253637]  mark_rodata_ro+0x68/0x98
> >> [    3.253847]  kernel_init+0x38/0x160
> >> [    3.254103]  ret_from_fork+0x10/0x18
> >>
> >> kprobes allocates a writable executable page with module_alloc() in
> >> order to store executable code.
> >> Reworked to that when allocate a page it sets mode RO. Inspired by
> >> commit 63fef14fc98a ("kprobes/x86: Make insn buffer always ROX and use text_poke()").
> >>
> >> Cc: Laura Abbott <labbott@redhat.com>
> >> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >> Co-developed-by: Arnd Bergmann <arnd@arndb.de>
> >> Co-developed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >
> > Please remove these SOBs, Arnd and I provided input to this patch
> > but you are the one sending it (sob does not assert authorship or
> > anything like that, it just asserts that the code in the patch was
> > made available under a compatible license)
> 
> As Anders points out in a private communication, the Documentation/
> explicitly requires signoffs for Co-developed-by credits. Perhaps we
> should enhance that document to clarify that that does not mean you
> can simply add signoffs on someone else's behalf.

I think I'll rename co-developed-by with suggested-by to keep things
simpler. Are you ok with this (or are you providing an explicit
signed-off-by)?

-- 
Catalin

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] arm64: kprobe: make page to RO mode when allocate it
  2018-11-02 17:40     ` Catalin Marinas
@ 2018-11-02 18:29       ` Ard Biesheuvel
  0 siblings, 0 replies; 7+ messages in thread
From: Ard Biesheuvel @ 2018-11-02 18:29 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Anders Roxell, Arnd Bergmann, Masami Hiramatsu, Will Deacon,
	Linux Kernel Mailing List, Laura Abbott, linux-arm-kernel

On 2 November 2018 at 18:40, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Tue, Oct 30, 2018 at 11:10:51AM -0300, Ard Biesheuvel wrote:
>> On 30 October 2018 at 08:49, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> >> On 30 Oct 2018, at 08:38, Anders Roxell <anders.roxell@linaro.org> wrote:
>> >>
>> >> Commit 1404d6f13e47 ("arm64: dump: Add checking for writable and exectuable pages")
>> >> has successfully identified code that leaves a page with W+X
>> >> permissions.
>> >>
>> >> [    3.245140] arm64/mm: Found insecure W+X mapping at address (____ptrval____)/0xffff000000d90000
>> >> [    3.245771] WARNING: CPU: 0 PID: 1 at ../arch/arm64/mm/dump.c:232 note_page+0x410/0x420
>> >> [    3.246141] Modules linked in:
>> >> [    3.246653] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.19.0-rc5-next-20180928-00001-ge70ae259b853-dirty #62
>> >> [    3.247008] Hardware name: linux,dummy-virt (DT)
>> >> [    3.247347] pstate: 80000005 (Nzcv daif -PAN -UAO)
>> >> [    3.247623] pc : note_page+0x410/0x420
>> >> [    3.247898] lr : note_page+0x410/0x420
>> >> [    3.248071] sp : ffff00000804bcd0
>> >> [    3.248254] x29: ffff00000804bcd0 x28: ffff000009274000
>> >> [    3.248578] x27: ffff00000921a000 x26: ffff80007dfff000
>> >> [    3.248845] x25: ffff0000093f5000 x24: ffff000009526f6a
>> >> [    3.249109] x23: 0000000000000004 x22: ffff000000d91000
>> >> [    3.249396] x21: ffff000000d90000 x20: 0000000000000000
>> >> [    3.249661] x19: ffff00000804bde8 x18: 0000000000000400
>> >> [    3.249924] x17: 0000000000000000 x16: 0000000000000000
>> >> [    3.250271] x15: ffffffffffffffff x14: 295f5f5f5f6c6176
>> >> [    3.250594] x13: 7274705f5f5f5f28 x12: 2073736572646461
>> >> [    3.250941] x11: 20746120676e6970 x10: 70616d20582b5720
>> >> [    3.251252] x9 : 6572756365736e69 x8 : 3039643030303030
>> >> [    3.251519] x7 : 306666666678302f x6 : ffff0000095467b2
>> >> [    3.251802] x5 : 0000000000000000 x4 : 0000000000000000
>> >> [    3.252060] x3 : 0000000000000000 x2 : ffffffffffffffff
>> >> [    3.252323] x1 : 4d151327adc50b00 x0 : 0000000000000000
>> >> [    3.252664] Call trace:
>> >> [    3.252953]  note_page+0x410/0x420
>> >> [    3.253186]  walk_pgd+0x12c/0x238
>> >> [    3.253417]  ptdump_check_wx+0x68/0xf8
>> >> [    3.253637]  mark_rodata_ro+0x68/0x98
>> >> [    3.253847]  kernel_init+0x38/0x160
>> >> [    3.254103]  ret_from_fork+0x10/0x18
>> >>
>> >> kprobes allocates a writable executable page with module_alloc() in
>> >> order to store executable code.
>> >> Reworked to that when allocate a page it sets mode RO. Inspired by
>> >> commit 63fef14fc98a ("kprobes/x86: Make insn buffer always ROX and use text_poke()").
>> >>
>> >> Cc: Laura Abbott <labbott@redhat.com>
>> >> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> >> Co-developed-by: Arnd Bergmann <arnd@arndb.de>
>> >> Co-developed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >
>> > Please remove these SOBs, Arnd and I provided input to this patch
>> > but you are the one sending it (sob does not assert authorship or
>> > anything like that, it just asserts that the code in the patch was
>> > made available under a compatible license)
>>
>> As Anders points out in a private communication, the Documentation/
>> explicitly requires signoffs for Co-developed-by credits. Perhaps we
>> should enhance that document to clarify that that does not mean you
>> can simply add signoffs on someone else's behalf.
>
> I think I'll rename co-developed-by with suggested-by to keep things
> simpler. Are you ok with this (or are you providing an explicit
> signed-off-by)?
>

Either is fine with me.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-11-02 18:29 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-30 11:38 [PATCH v2] arm64: kprobe: make page to RO mode when allocate it Anders Roxell
2018-10-30 11:45 ` Will Deacon
2018-10-30 11:49 ` Ard Biesheuvel
2018-10-30 14:10   ` Ard Biesheuvel
2018-11-02 17:40     ` Catalin Marinas
2018-11-02 18:29       ` Ard Biesheuvel
2018-11-02 17:31 ` Catalin Marinas

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).