* [PATCH 2/2] arm: use fixmap for text patching when text is RO
@ 2014-04-04 21:27 ` Rabin Vincent
0 siblings, 0 replies; 20+ messages in thread
From: Rabin Vincent @ 2014-04-04 21:27 UTC (permalink / raw)
To: linux-arm-kernel
Use fixmaps for text patching when the kernel text is read-only,
inspired by x86. This makes jump labels and kprobes work with the
currently available CONFIG_DEBUG_SET_MODULE_RONX and the upcoming
CONFIG_DEBUG_RODATA options.
Signed-off-by: Rabin Vincent <rabin@rab.in>
---
arch/arm/include/asm/fixmap.h | 3 ++
arch/arm/kernel/jump_label.c | 2 +-
arch/arm/kernel/patch.c | 66 ++++++++++++++++++++++++++++++++++++++-----
arch/arm/kernel/patch.h | 12 +++++++-
4 files changed, 74 insertions(+), 9 deletions(-)
diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
index 55ed076..79c1719 100644
--- a/arch/arm/include/asm/fixmap.h
+++ b/arch/arm/include/asm/fixmap.h
@@ -18,6 +18,9 @@
#define FIXADDR_TOP (FIXADDR_END - PAGE_SIZE)
enum fixed_addresses {
+ FIX_TEXT_POKE0,
+ FIX_TEXT_POKE1,
+
FIX_KMAP_BEGIN,
FIX_KMAP_END = (FIXADDR_TOP - FIXADDR_START) >> PAGE_SHIFT,
__end_of_fixed_addresses
diff --git a/arch/arm/kernel/jump_label.c b/arch/arm/kernel/jump_label.c
index 4ce4f78..afeeb9e 100644
--- a/arch/arm/kernel/jump_label.c
+++ b/arch/arm/kernel/jump_label.c
@@ -19,7 +19,7 @@ static void __arch_jump_label_transform(struct jump_entry *entry,
insn = arm_gen_nop();
if (is_static)
- __patch_text(addr, insn);
+ __patch_text_early(addr, insn);
else
patch_text(addr, insn);
}
diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c
index 07314af..761c5b6 100644
--- a/arch/arm/kernel/patch.c
+++ b/arch/arm/kernel/patch.c
@@ -1,8 +1,11 @@
#include <linux/kernel.h>
+#include <linux/spinlock.h>
#include <linux/kprobes.h>
+#include <linux/mm.h>
#include <linux/stop_machine.h>
#include <asm/cacheflush.h>
+#include <asm/fixmap.h>
#include <asm/smp_plat.h>
#include <asm/opcodes.h>
@@ -13,21 +16,67 @@ struct patch {
unsigned int insn;
};
-void __kprobes __patch_text(void *addr, unsigned int insn)
+static DEFINE_SPINLOCK(patch_lock);
+
+static void __kprobes *patch_map(void *addr, int fixmap, unsigned long *flags)
+{
+ unsigned int uintaddr = (uintptr_t) addr;
+ bool module = !core_kernel_text(uintaddr);
+ struct page *page;
+
+ if (module && IS_ENABLED(CONFIG_DEBUG_SET_MODULE_RONX)) {
+ page = vmalloc_to_page(addr);
+ } else if (!module && IS_ENABLED(CONFIG_ARM_KERNMEM_PERMS)) {
+ page = virt_to_page(addr);
+ } else {
+ return addr;
+ }
+
+ if (flags)
+ spin_lock_irqsave(&patch_lock, *flags);
+
+ set_fixmap(fixmap, page_to_phys(page));
+
+ return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK));
+}
+
+static void __kprobes patch_unmap(int fixmap, unsigned long *flags)
+{
+ clear_fixmap(fixmap);
+
+ if (flags)
+ spin_unlock_irqrestore(&patch_lock, *flags);
+}
+
+void __kprobes __patch_text_real(void *addr, unsigned int insn, bool remap)
{
bool thumb2 = IS_ENABLED(CONFIG_THUMB2_KERNEL);
+ unsigned int uintaddr = (uintptr_t) addr;
+ unsigned long flags;
+ void *waddr = addr;
int size;
+ if (remap)
+ waddr = patch_map(addr, FIX_TEXT_POKE0, &flags);
+
if (thumb2 && __opcode_is_thumb16(insn)) {
- *(u16 *)addr = __opcode_to_mem_thumb16(insn);
+ *(u16 *)waddr = __opcode_to_mem_thumb16(insn);
size = sizeof(u16);
- } else if (thumb2 && ((uintptr_t)addr & 2)) {
+ } else if (thumb2 && (uintaddr & 2)) {
+ bool twopage = (uintaddr & ~PAGE_MASK) == PAGE_SIZE - 2;
u16 first = __opcode_thumb32_first(insn);
u16 second = __opcode_thumb32_second(insn);
- u16 *addrh = addr;
+ u16 *addrh0 = waddr;
+ u16 *addrh1 = waddr + 2;
+
+ if (remap && twopage)
+ addrh1 = patch_map(addr + 2, FIX_TEXT_POKE1, NULL);
- addrh[0] = __opcode_to_mem_thumb16(first);
- addrh[1] = __opcode_to_mem_thumb16(second);
+ *addrh0 = __opcode_to_mem_thumb16(first);
+ *addrh1 = __opcode_to_mem_thumb16(second);
+
+ if (twopage && addrh1 != addr + 2)
+ patch_unmap(FIX_TEXT_POKE1, NULL);
size = sizeof(u32);
} else {
@@ -36,10 +85,13 @@ void __kprobes __patch_text(void *addr, unsigned int insn)
else
insn = __opcode_to_mem_arm(insn);
- *(u32 *)addr = insn;
+ *(u32 *)waddr = insn;
size = sizeof(u32);
}
+ if (waddr != addr)
+ patch_unmap(FIX_TEXT_POKE0, &flags);
+
flush_icache_range((uintptr_t)(addr),
(uintptr_t)(addr) + size);
}
diff --git a/arch/arm/kernel/patch.h b/arch/arm/kernel/patch.h
index b4731f2..77e054c 100644
--- a/arch/arm/kernel/patch.h
+++ b/arch/arm/kernel/patch.h
@@ -2,6 +2,16 @@
#define _ARM_KERNEL_PATCH_H
void patch_text(void *addr, unsigned int insn);
-void __patch_text(void *addr, unsigned int insn);
+void __patch_text_real(void *addr, unsigned int insn, bool remap);
+
+static inline void __patch_text(void *addr, unsigned int insn)
+{
+ __patch_text_real(addr, insn, true);
+}
+
+static inline void __patch_text_early(void *addr, unsigned int insn)
+{
+ __patch_text_real(addr, insn, false);
+}
#endif
--
1.9.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 2/2] arm: use fixmap for text patching when text is RO
2014-04-04 21:27 ` Rabin Vincent
@ 2014-04-07 13:57 ` Jon Medhurst (Tixy)
-1 siblings, 0 replies; 20+ messages in thread
From: Jon Medhurst (Tixy) @ 2014-04-07 13:57 UTC (permalink / raw)
To: Rabin Vincent; +Cc: linux-arm-kernel, linux-kernel, Kees Cook, Laura Abbott
On Fri, 2014-04-04 at 23:27 +0200, Rabin Vincent wrote:
> Use fixmaps for text patching when the kernel text is read-only,
> inspired by x86. This makes jump labels and kprobes work with the
> currently available CONFIG_DEBUG_SET_MODULE_RONX and the upcoming
> CONFIG_DEBUG_RODATA options.
>
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
> arch/arm/include/asm/fixmap.h | 3 ++
> arch/arm/kernel/jump_label.c | 2 +-
> arch/arm/kernel/patch.c | 66 ++++++++++++++++++++++++++++++++++++++-----
> arch/arm/kernel/patch.h | 12 +++++++-
> 4 files changed, 74 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
> index 55ed076..79c1719 100644
> --- a/arch/arm/include/asm/fixmap.h
> +++ b/arch/arm/include/asm/fixmap.h
> @@ -18,6 +18,9 @@
> #define FIXADDR_TOP (FIXADDR_END - PAGE_SIZE)
>
> enum fixed_addresses {
> + FIX_TEXT_POKE0,
> + FIX_TEXT_POKE1,
> +
> FIX_KMAP_BEGIN,
> FIX_KMAP_END = (FIXADDR_TOP - FIXADDR_START) >> PAGE_SHIFT,
> __end_of_fixed_addresses
> diff --git a/arch/arm/kernel/jump_label.c b/arch/arm/kernel/jump_label.c
> index 4ce4f78..afeeb9e 100644
> --- a/arch/arm/kernel/jump_label.c
> +++ b/arch/arm/kernel/jump_label.c
> @@ -19,7 +19,7 @@ static void __arch_jump_label_transform(struct jump_entry *entry,
> insn = arm_gen_nop();
>
> if (is_static)
> - __patch_text(addr, insn);
> + __patch_text_early(addr, insn);
> else
> patch_text(addr, insn);
> }
> diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c
> index 07314af..761c5b6 100644
> --- a/arch/arm/kernel/patch.c
> +++ b/arch/arm/kernel/patch.c
> @@ -1,8 +1,11 @@
> #include <linux/kernel.h>
> +#include <linux/spinlock.h>
> #include <linux/kprobes.h>
> +#include <linux/mm.h>
> #include <linux/stop_machine.h>
>
> #include <asm/cacheflush.h>
> +#include <asm/fixmap.h>
> #include <asm/smp_plat.h>
> #include <asm/opcodes.h>
>
> @@ -13,21 +16,67 @@ struct patch {
> unsigned int insn;
> };
>
> -void __kprobes __patch_text(void *addr, unsigned int insn)
> +static DEFINE_SPINLOCK(patch_lock);
> +
> +static void __kprobes *patch_map(void *addr, int fixmap, unsigned long *flags)
> +{
> + unsigned int uintaddr = (uintptr_t) addr;
> + bool module = !core_kernel_text(uintaddr);
> + struct page *page;
> +
> + if (module && IS_ENABLED(CONFIG_DEBUG_SET_MODULE_RONX)) {
> + page = vmalloc_to_page(addr);
> + } else if (!module && IS_ENABLED(CONFIG_ARM_KERNMEM_PERMS)) {
> + page = virt_to_page(addr);
> + } else {
> + return addr;
> + }
I can't help but think that it'd be more maintainable to just always use
fixmap, though that would obviously have some performance impact (not
sure that particularly matters for text patching) and it would expose
possible fixmap bugginess to more kernels (see my next comment...)
> +
> + if (flags)
> + spin_lock_irqsave(&patch_lock, *flags);
> +
> + set_fixmap(fixmap, page_to_phys(page));
> +
> + return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK));
How does fixmap cope with cache colouring? Looking at the implementation
it looks like it doesn't and so fixmap use on ARM is possibly buggy.
For the text patching case where we know there are no writeable mappings
[1] this should be OK if we used set_fixmap_nocache here, so long as we
also invalidated the dcache later for the proper virtual address.
[1] Can we know there are no writeable mappings though, the ftrace code
modifying patches from Kees Cook have there own way of modifying text
code permissions.
[...]
Rest of patch trimmed
--
Tixy
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH 2/2] arm: use fixmap for text patching when text is RO
@ 2014-04-07 13:57 ` Jon Medhurst (Tixy)
0 siblings, 0 replies; 20+ messages in thread
From: Jon Medhurst (Tixy) @ 2014-04-07 13:57 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 2014-04-04 at 23:27 +0200, Rabin Vincent wrote:
> Use fixmaps for text patching when the kernel text is read-only,
> inspired by x86. This makes jump labels and kprobes work with the
> currently available CONFIG_DEBUG_SET_MODULE_RONX and the upcoming
> CONFIG_DEBUG_RODATA options.
>
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
> arch/arm/include/asm/fixmap.h | 3 ++
> arch/arm/kernel/jump_label.c | 2 +-
> arch/arm/kernel/patch.c | 66 ++++++++++++++++++++++++++++++++++++++-----
> arch/arm/kernel/patch.h | 12 +++++++-
> 4 files changed, 74 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
> index 55ed076..79c1719 100644
> --- a/arch/arm/include/asm/fixmap.h
> +++ b/arch/arm/include/asm/fixmap.h
> @@ -18,6 +18,9 @@
> #define FIXADDR_TOP (FIXADDR_END - PAGE_SIZE)
>
> enum fixed_addresses {
> + FIX_TEXT_POKE0,
> + FIX_TEXT_POKE1,
> +
> FIX_KMAP_BEGIN,
> FIX_KMAP_END = (FIXADDR_TOP - FIXADDR_START) >> PAGE_SHIFT,
> __end_of_fixed_addresses
> diff --git a/arch/arm/kernel/jump_label.c b/arch/arm/kernel/jump_label.c
> index 4ce4f78..afeeb9e 100644
> --- a/arch/arm/kernel/jump_label.c
> +++ b/arch/arm/kernel/jump_label.c
> @@ -19,7 +19,7 @@ static void __arch_jump_label_transform(struct jump_entry *entry,
> insn = arm_gen_nop();
>
> if (is_static)
> - __patch_text(addr, insn);
> + __patch_text_early(addr, insn);
> else
> patch_text(addr, insn);
> }
> diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c
> index 07314af..761c5b6 100644
> --- a/arch/arm/kernel/patch.c
> +++ b/arch/arm/kernel/patch.c
> @@ -1,8 +1,11 @@
> #include <linux/kernel.h>
> +#include <linux/spinlock.h>
> #include <linux/kprobes.h>
> +#include <linux/mm.h>
> #include <linux/stop_machine.h>
>
> #include <asm/cacheflush.h>
> +#include <asm/fixmap.h>
> #include <asm/smp_plat.h>
> #include <asm/opcodes.h>
>
> @@ -13,21 +16,67 @@ struct patch {
> unsigned int insn;
> };
>
> -void __kprobes __patch_text(void *addr, unsigned int insn)
> +static DEFINE_SPINLOCK(patch_lock);
> +
> +static void __kprobes *patch_map(void *addr, int fixmap, unsigned long *flags)
> +{
> + unsigned int uintaddr = (uintptr_t) addr;
> + bool module = !core_kernel_text(uintaddr);
> + struct page *page;
> +
> + if (module && IS_ENABLED(CONFIG_DEBUG_SET_MODULE_RONX)) {
> + page = vmalloc_to_page(addr);
> + } else if (!module && IS_ENABLED(CONFIG_ARM_KERNMEM_PERMS)) {
> + page = virt_to_page(addr);
> + } else {
> + return addr;
> + }
I can't help but think that it'd be more maintainable to just always use
fixmap, though that would obviously have some performance impact (not
sure that particularly matters for text patching) and it would expose
possible fixmap bugginess to more kernels (see my next comment...)
> +
> + if (flags)
> + spin_lock_irqsave(&patch_lock, *flags);
> +
> + set_fixmap(fixmap, page_to_phys(page));
> +
> + return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK));
How does fixmap cope with cache colouring? Looking at the implementation
it looks like it doesn't and so fixmap use on ARM is possibly buggy.
For the text patching case where we know there are no writeable mappings
[1] this should be OK if we used set_fixmap_nocache here, so long as we
also invalidated the dcache later for the proper virtual address.
[1] Can we know there are no writeable mappings though, the ftrace code
modifying patches from Kees Cook have there own way of modifying text
code permissions.
[...]
Rest of patch trimmed
--
Tixy
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/2] arm: use fixmap for text patching when text is RO
2014-04-07 13:57 ` Jon Medhurst (Tixy)
@ 2014-04-07 18:04 ` Kees Cook
-1 siblings, 0 replies; 20+ messages in thread
From: Kees Cook @ 2014-04-07 18:04 UTC (permalink / raw)
To: Jon Medhurst (Tixy); +Cc: Rabin Vincent, linux-arm-kernel, LKML, Laura Abbott
On Mon, Apr 7, 2014 at 6:57 AM, Jon Medhurst (Tixy) <tixy@linaro.org> wrote:
> On Fri, 2014-04-04 at 23:27 +0200, Rabin Vincent wrote:
>> Use fixmaps for text patching when the kernel text is read-only,
>> inspired by x86. This makes jump labels and kprobes work with the
>> currently available CONFIG_DEBUG_SET_MODULE_RONX and the upcoming
>> CONFIG_DEBUG_RODATA options.
>>
>> Signed-off-by: Rabin Vincent <rabin@rab.in>
>> ---
>> arch/arm/include/asm/fixmap.h | 3 ++
>> arch/arm/kernel/jump_label.c | 2 +-
>> arch/arm/kernel/patch.c | 66 ++++++++++++++++++++++++++++++++++++++-----
>> arch/arm/kernel/patch.h | 12 +++++++-
>> 4 files changed, 74 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
>> index 55ed076..79c1719 100644
>> --- a/arch/arm/include/asm/fixmap.h
>> +++ b/arch/arm/include/asm/fixmap.h
>> @@ -18,6 +18,9 @@
>> #define FIXADDR_TOP (FIXADDR_END - PAGE_SIZE)
>>
>> enum fixed_addresses {
>> + FIX_TEXT_POKE0,
>> + FIX_TEXT_POKE1,
>> +
>> FIX_KMAP_BEGIN,
>> FIX_KMAP_END = (FIXADDR_TOP - FIXADDR_START) >> PAGE_SHIFT,
>> __end_of_fixed_addresses
>> diff --git a/arch/arm/kernel/jump_label.c b/arch/arm/kernel/jump_label.c
>> index 4ce4f78..afeeb9e 100644
>> --- a/arch/arm/kernel/jump_label.c
>> +++ b/arch/arm/kernel/jump_label.c
>> @@ -19,7 +19,7 @@ static void __arch_jump_label_transform(struct jump_entry *entry,
>> insn = arm_gen_nop();
>>
>> if (is_static)
>> - __patch_text(addr, insn);
>> + __patch_text_early(addr, insn);
>> else
>> patch_text(addr, insn);
>> }
>> diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c
>> index 07314af..761c5b6 100644
>> --- a/arch/arm/kernel/patch.c
>> +++ b/arch/arm/kernel/patch.c
>> @@ -1,8 +1,11 @@
>> #include <linux/kernel.h>
>> +#include <linux/spinlock.h>
>> #include <linux/kprobes.h>
>> +#include <linux/mm.h>
>> #include <linux/stop_machine.h>
>>
>> #include <asm/cacheflush.h>
>> +#include <asm/fixmap.h>
>> #include <asm/smp_plat.h>
>> #include <asm/opcodes.h>
>>
>> @@ -13,21 +16,67 @@ struct patch {
>> unsigned int insn;
>> };
>>
>> -void __kprobes __patch_text(void *addr, unsigned int insn)
>> +static DEFINE_SPINLOCK(patch_lock);
>> +
>> +static void __kprobes *patch_map(void *addr, int fixmap, unsigned long *flags)
>> +{
>> + unsigned int uintaddr = (uintptr_t) addr;
>> + bool module = !core_kernel_text(uintaddr);
>> + struct page *page;
>> +
>> + if (module && IS_ENABLED(CONFIG_DEBUG_SET_MODULE_RONX)) {
>> + page = vmalloc_to_page(addr);
>> + } else if (!module && IS_ENABLED(CONFIG_ARM_KERNMEM_PERMS)) {
I think this should probably be CONFIG_DEBUG_RODATA now that I've
moved RO exclusively into that config.
>> + page = virt_to_page(addr);
>> + } else {
>> + return addr;
>> + }
>
> I can't help but think that it'd be more maintainable to just always use
> fixmap, though that would obviously have some performance impact (not
> sure that particularly matters for text patching) and it would expose
> possible fixmap bugginess to more kernels (see my next comment...)
>
>> +
>> + if (flags)
>> + spin_lock_irqsave(&patch_lock, *flags);
>> +
>> + set_fixmap(fixmap, page_to_phys(page));
>> +
>> + return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK));
>
> How does fixmap cope with cache colouring? Looking at the implementation
> it looks like it doesn't and so fixmap use on ARM is possibly buggy.
>
> For the text patching case where we know there are no writeable mappings
> [1] this should be OK if we used set_fixmap_nocache here, so long as we
> also invalidated the dcache later for the proper virtual address.
>
> [1] Can we know there are no writeable mappings though, the ftrace code
> modifying patches from Kees Cook have there own way of modifying text
> code permissions.
I think performance becomes an issue for massive updates like ftrace
seems to do, so I got the sense it was good to have the "bulk" way to
do it, and then this tiny poking way to do it.
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH 2/2] arm: use fixmap for text patching when text is RO
@ 2014-04-07 18:04 ` Kees Cook
0 siblings, 0 replies; 20+ messages in thread
From: Kees Cook @ 2014-04-07 18:04 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Apr 7, 2014 at 6:57 AM, Jon Medhurst (Tixy) <tixy@linaro.org> wrote:
> On Fri, 2014-04-04 at 23:27 +0200, Rabin Vincent wrote:
>> Use fixmaps for text patching when the kernel text is read-only,
>> inspired by x86. This makes jump labels and kprobes work with the
>> currently available CONFIG_DEBUG_SET_MODULE_RONX and the upcoming
>> CONFIG_DEBUG_RODATA options.
>>
>> Signed-off-by: Rabin Vincent <rabin@rab.in>
>> ---
>> arch/arm/include/asm/fixmap.h | 3 ++
>> arch/arm/kernel/jump_label.c | 2 +-
>> arch/arm/kernel/patch.c | 66 ++++++++++++++++++++++++++++++++++++++-----
>> arch/arm/kernel/patch.h | 12 +++++++-
>> 4 files changed, 74 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
>> index 55ed076..79c1719 100644
>> --- a/arch/arm/include/asm/fixmap.h
>> +++ b/arch/arm/include/asm/fixmap.h
>> @@ -18,6 +18,9 @@
>> #define FIXADDR_TOP (FIXADDR_END - PAGE_SIZE)
>>
>> enum fixed_addresses {
>> + FIX_TEXT_POKE0,
>> + FIX_TEXT_POKE1,
>> +
>> FIX_KMAP_BEGIN,
>> FIX_KMAP_END = (FIXADDR_TOP - FIXADDR_START) >> PAGE_SHIFT,
>> __end_of_fixed_addresses
>> diff --git a/arch/arm/kernel/jump_label.c b/arch/arm/kernel/jump_label.c
>> index 4ce4f78..afeeb9e 100644
>> --- a/arch/arm/kernel/jump_label.c
>> +++ b/arch/arm/kernel/jump_label.c
>> @@ -19,7 +19,7 @@ static void __arch_jump_label_transform(struct jump_entry *entry,
>> insn = arm_gen_nop();
>>
>> if (is_static)
>> - __patch_text(addr, insn);
>> + __patch_text_early(addr, insn);
>> else
>> patch_text(addr, insn);
>> }
>> diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c
>> index 07314af..761c5b6 100644
>> --- a/arch/arm/kernel/patch.c
>> +++ b/arch/arm/kernel/patch.c
>> @@ -1,8 +1,11 @@
>> #include <linux/kernel.h>
>> +#include <linux/spinlock.h>
>> #include <linux/kprobes.h>
>> +#include <linux/mm.h>
>> #include <linux/stop_machine.h>
>>
>> #include <asm/cacheflush.h>
>> +#include <asm/fixmap.h>
>> #include <asm/smp_plat.h>
>> #include <asm/opcodes.h>
>>
>> @@ -13,21 +16,67 @@ struct patch {
>> unsigned int insn;
>> };
>>
>> -void __kprobes __patch_text(void *addr, unsigned int insn)
>> +static DEFINE_SPINLOCK(patch_lock);
>> +
>> +static void __kprobes *patch_map(void *addr, int fixmap, unsigned long *flags)
>> +{
>> + unsigned int uintaddr = (uintptr_t) addr;
>> + bool module = !core_kernel_text(uintaddr);
>> + struct page *page;
>> +
>> + if (module && IS_ENABLED(CONFIG_DEBUG_SET_MODULE_RONX)) {
>> + page = vmalloc_to_page(addr);
>> + } else if (!module && IS_ENABLED(CONFIG_ARM_KERNMEM_PERMS)) {
I think this should probably be CONFIG_DEBUG_RODATA now that I've
moved RO exclusively into that config.
>> + page = virt_to_page(addr);
>> + } else {
>> + return addr;
>> + }
>
> I can't help but think that it'd be more maintainable to just always use
> fixmap, though that would obviously have some performance impact (not
> sure that particularly matters for text patching) and it would expose
> possible fixmap bugginess to more kernels (see my next comment...)
>
>> +
>> + if (flags)
>> + spin_lock_irqsave(&patch_lock, *flags);
>> +
>> + set_fixmap(fixmap, page_to_phys(page));
>> +
>> + return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK));
>
> How does fixmap cope with cache colouring? Looking at the implementation
> it looks like it doesn't and so fixmap use on ARM is possibly buggy.
>
> For the text patching case where we know there are no writeable mappings
> [1] this should be OK if we used set_fixmap_nocache here, so long as we
> also invalidated the dcache later for the proper virtual address.
>
> [1] Can we know there are no writeable mappings though, the ftrace code
> modifying patches from Kees Cook have there own way of modifying text
> code permissions.
I think performance becomes an issue for massive updates like ftrace
seems to do, so I got the sense it was good to have the "bulk" way to
do it, and then this tiny poking way to do it.
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/2] arm: use fixmap for text patching when text is RO
2014-04-07 18:04 ` Kees Cook
@ 2014-04-08 9:31 ` Jon Medhurst (Tixy)
-1 siblings, 0 replies; 20+ messages in thread
From: Jon Medhurst (Tixy) @ 2014-04-08 9:31 UTC (permalink / raw)
To: Kees Cook; +Cc: Rabin Vincent, linux-arm-kernel, LKML, Laura Abbott
On Mon, 2014-04-07 at 11:04 -0700, Kees Cook wrote:
> On Mon, Apr 7, 2014 at 6:57 AM, Jon Medhurst (Tixy) <tixy@linaro.org> wrote:
> > On Fri, 2014-04-04 at 23:27 +0200, Rabin Vincent wrote:
> >> Use fixmaps for text patching when the kernel text is read-only,
> >> inspired by x86. This makes jump labels and kprobes work with the
> >> currently available CONFIG_DEBUG_SET_MODULE_RONX and the upcoming
> >> CONFIG_DEBUG_RODATA options.
> >>
> >> Signed-off-by: Rabin Vincent <rabin@rab.in>
> >> ---
> >> arch/arm/include/asm/fixmap.h | 3 ++
> >> arch/arm/kernel/jump_label.c | 2 +-
> >> arch/arm/kernel/patch.c | 66 ++++++++++++++++++++++++++++++++++++++-----
> >> arch/arm/kernel/patch.h | 12 +++++++-
> >> 4 files changed, 74 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
> >> index 55ed076..79c1719 100644
> >> --- a/arch/arm/include/asm/fixmap.h
> >> +++ b/arch/arm/include/asm/fixmap.h
> >> @@ -18,6 +18,9 @@
> >> #define FIXADDR_TOP (FIXADDR_END - PAGE_SIZE)
> >>
> >> enum fixed_addresses {
> >> + FIX_TEXT_POKE0,
> >> + FIX_TEXT_POKE1,
> >> +
> >> FIX_KMAP_BEGIN,
> >> FIX_KMAP_END = (FIXADDR_TOP - FIXADDR_START) >> PAGE_SHIFT,
> >> __end_of_fixed_addresses
> >> diff --git a/arch/arm/kernel/jump_label.c b/arch/arm/kernel/jump_label.c
> >> index 4ce4f78..afeeb9e 100644
> >> --- a/arch/arm/kernel/jump_label.c
> >> +++ b/arch/arm/kernel/jump_label.c
> >> @@ -19,7 +19,7 @@ static void __arch_jump_label_transform(struct jump_entry *entry,
> >> insn = arm_gen_nop();
> >>
> >> if (is_static)
> >> - __patch_text(addr, insn);
> >> + __patch_text_early(addr, insn);
> >> else
> >> patch_text(addr, insn);
> >> }
> >> diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c
> >> index 07314af..761c5b6 100644
> >> --- a/arch/arm/kernel/patch.c
> >> +++ b/arch/arm/kernel/patch.c
> >> @@ -1,8 +1,11 @@
> >> #include <linux/kernel.h>
> >> +#include <linux/spinlock.h>
> >> #include <linux/kprobes.h>
> >> +#include <linux/mm.h>
> >> #include <linux/stop_machine.h>
> >>
> >> #include <asm/cacheflush.h>
> >> +#include <asm/fixmap.h>
> >> #include <asm/smp_plat.h>
> >> #include <asm/opcodes.h>
> >>
> >> @@ -13,21 +16,67 @@ struct patch {
> >> unsigned int insn;
> >> };
> >>
> >> -void __kprobes __patch_text(void *addr, unsigned int insn)
> >> +static DEFINE_SPINLOCK(patch_lock);
> >> +
> >> +static void __kprobes *patch_map(void *addr, int fixmap, unsigned long *flags)
> >> +{
> >> + unsigned int uintaddr = (uintptr_t) addr;
> >> + bool module = !core_kernel_text(uintaddr);
> >> + struct page *page;
> >> +
> >> + if (module && IS_ENABLED(CONFIG_DEBUG_SET_MODULE_RONX)) {
> >> + page = vmalloc_to_page(addr);
> >> + } else if (!module && IS_ENABLED(CONFIG_ARM_KERNMEM_PERMS)) {
>
> I think this should probably be CONFIG_DEBUG_RODATA now that I've
> moved RO exclusively into that config.
>
> >> + page = virt_to_page(addr);
> >> + } else {
> >> + return addr;
> >> + }
> >
> > I can't help but think that it'd be more maintainable to just always use
> > fixmap, though that would obviously have some performance impact (not
> > sure that particularly matters for text patching) and it would expose
> > possible fixmap bugginess to more kernels (see my next comment...)
> >
> >> +
> >> + if (flags)
> >> + spin_lock_irqsave(&patch_lock, *flags);
> >> +
> >> + set_fixmap(fixmap, page_to_phys(page));
> >> +
> >> + return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK));
> >
> > How does fixmap cope with cache colouring? Looking at the implementation
> > it looks like it doesn't and so fixmap use on ARM is possibly buggy.
> >
> > For the text patching case where we know there are no writeable mappings
> > [1] this should be OK if we used set_fixmap_nocache here, so long as we
> > also invalidated the dcache later for the proper virtual address.
> >
> > [1] Can we know there are no writeable mappings though, the ftrace code
> > modifying patches from Kees Cook have there own way of modifying text
> > code permissions.
>
> I think performance becomes an issue for massive updates like ftrace
> seems to do, so I got the sense it was good to have the "bulk" way to
> do it, and then this tiny poking way to do it.
Probably performance is significant, but as safely modifying kernel code
is virtually impossible to get right I would think that it best to
concentrate on a single method for this.
There again, the whole concept of things like ftrace is fundamentally
flawed, with it being impossible to determine all the functions which
require notrace annotations in order to avoid kernel crashes and
corruption. So perhaps a little flakiness in kprobes and ftrace is
acceptable? (I hope these things aren't enabled on distro kernels and
usable by non-root processes...)
--
Tixy
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH 2/2] arm: use fixmap for text patching when text is RO
@ 2014-04-08 9:31 ` Jon Medhurst (Tixy)
0 siblings, 0 replies; 20+ messages in thread
From: Jon Medhurst (Tixy) @ 2014-04-08 9:31 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, 2014-04-07 at 11:04 -0700, Kees Cook wrote:
> On Mon, Apr 7, 2014 at 6:57 AM, Jon Medhurst (Tixy) <tixy@linaro.org> wrote:
> > On Fri, 2014-04-04 at 23:27 +0200, Rabin Vincent wrote:
> >> Use fixmaps for text patching when the kernel text is read-only,
> >> inspired by x86. This makes jump labels and kprobes work with the
> >> currently available CONFIG_DEBUG_SET_MODULE_RONX and the upcoming
> >> CONFIG_DEBUG_RODATA options.
> >>
> >> Signed-off-by: Rabin Vincent <rabin@rab.in>
> >> ---
> >> arch/arm/include/asm/fixmap.h | 3 ++
> >> arch/arm/kernel/jump_label.c | 2 +-
> >> arch/arm/kernel/patch.c | 66 ++++++++++++++++++++++++++++++++++++++-----
> >> arch/arm/kernel/patch.h | 12 +++++++-
> >> 4 files changed, 74 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
> >> index 55ed076..79c1719 100644
> >> --- a/arch/arm/include/asm/fixmap.h
> >> +++ b/arch/arm/include/asm/fixmap.h
> >> @@ -18,6 +18,9 @@
> >> #define FIXADDR_TOP (FIXADDR_END - PAGE_SIZE)
> >>
> >> enum fixed_addresses {
> >> + FIX_TEXT_POKE0,
> >> + FIX_TEXT_POKE1,
> >> +
> >> FIX_KMAP_BEGIN,
> >> FIX_KMAP_END = (FIXADDR_TOP - FIXADDR_START) >> PAGE_SHIFT,
> >> __end_of_fixed_addresses
> >> diff --git a/arch/arm/kernel/jump_label.c b/arch/arm/kernel/jump_label.c
> >> index 4ce4f78..afeeb9e 100644
> >> --- a/arch/arm/kernel/jump_label.c
> >> +++ b/arch/arm/kernel/jump_label.c
> >> @@ -19,7 +19,7 @@ static void __arch_jump_label_transform(struct jump_entry *entry,
> >> insn = arm_gen_nop();
> >>
> >> if (is_static)
> >> - __patch_text(addr, insn);
> >> + __patch_text_early(addr, insn);
> >> else
> >> patch_text(addr, insn);
> >> }
> >> diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c
> >> index 07314af..761c5b6 100644
> >> --- a/arch/arm/kernel/patch.c
> >> +++ b/arch/arm/kernel/patch.c
> >> @@ -1,8 +1,11 @@
> >> #include <linux/kernel.h>
> >> +#include <linux/spinlock.h>
> >> #include <linux/kprobes.h>
> >> +#include <linux/mm.h>
> >> #include <linux/stop_machine.h>
> >>
> >> #include <asm/cacheflush.h>
> >> +#include <asm/fixmap.h>
> >> #include <asm/smp_plat.h>
> >> #include <asm/opcodes.h>
> >>
> >> @@ -13,21 +16,67 @@ struct patch {
> >> unsigned int insn;
> >> };
> >>
> >> -void __kprobes __patch_text(void *addr, unsigned int insn)
> >> +static DEFINE_SPINLOCK(patch_lock);
> >> +
> >> +static void __kprobes *patch_map(void *addr, int fixmap, unsigned long *flags)
> >> +{
> >> + unsigned int uintaddr = (uintptr_t) addr;
> >> + bool module = !core_kernel_text(uintaddr);
> >> + struct page *page;
> >> +
> >> + if (module && IS_ENABLED(CONFIG_DEBUG_SET_MODULE_RONX)) {
> >> + page = vmalloc_to_page(addr);
> >> + } else if (!module && IS_ENABLED(CONFIG_ARM_KERNMEM_PERMS)) {
>
> I think this should probably be CONFIG_DEBUG_RODATA now that I've
> moved RO exclusively into that config.
>
> >> + page = virt_to_page(addr);
> >> + } else {
> >> + return addr;
> >> + }
> >
> > I can't help but think that it'd be more maintainable to just always use
> > fixmap, though that would obviously have some performance impact (not
> > sure that particularly matters for text patching) and it would expose
> > possible fixmap bugginess to more kernels (see my next comment...)
> >
> >> +
> >> + if (flags)
> >> + spin_lock_irqsave(&patch_lock, *flags);
> >> +
> >> + set_fixmap(fixmap, page_to_phys(page));
> >> +
> >> + return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK));
> >
> > How does fixmap cope with cache colouring? Looking at the implementation
> > it looks like it doesn't and so fixmap use on ARM is possibly buggy.
> >
> > For the text patching case where we know there are no writeable mappings
> > [1] this should be OK if we used set_fixmap_nocache here, so long as we
> > also invalidated the dcache later for the proper virtual address.
> >
> > [1] Can we know there are no writeable mappings though, the ftrace code
> > modifying patches from Kees Cook have there own way of modifying text
> > code permissions.
>
> I think performance becomes an issue for massive updates like ftrace
> seems to do, so I got the sense it was good to have the "bulk" way to
> do it, and then this tiny poking way to do it.
Probably performance is significant, but as safely modifying kernel code
is virtually impossible to get right I would think that it best to
concentrate on a single method for this.
There again, the whole concept of things like ftrace is fundamentally
flawed, with it being impossible to determine all the functions which
require notrace annotations in order to avoid kernel crashes and
corruption. So perhaps a little flakiness in kprobes and ftrace is
acceptable? (I hope these things aren't enabled on distro kernels and
usable by non-root processes...)
--
Tixy
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] arm: use fixmap for text patching when text is RO
2014-04-07 13:57 ` Jon Medhurst (Tixy)
@ 2014-04-10 19:34 ` Rabin Vincent
-1 siblings, 0 replies; 20+ messages in thread
From: Rabin Vincent @ 2014-04-10 19:34 UTC (permalink / raw)
To: Jon Medhurst (Tixy)
Cc: linux-arm-kernel, linux-kernel, Kees Cook, Laura Abbott
On Mon, Apr 07, 2014 at 02:57:51PM +0100, Jon Medhurst (Tixy) wrote:
> How does fixmap cope with cache colouring? Looking at the implementation
> it looks like it doesn't and so fixmap use on ARM is possibly buggy.
>
> For the text patching case where we know there are no writeable mappings
> [1] this should be OK if we used set_fixmap_nocache here, so long as we
> also invalidated the dcache later for the proper virtual address.
OK. The dcache invalidation for the proper virtual address is btw
already there via the call to flush_icache_range().
> [1] Can we know there are no writeable mappings though, the ftrace code
> modifying patches from Kees Cook have there own way of modifying text
> code permissions.
The ftrace patches does the modifications and the cache cleaning in
stop_machine(), so there should not be any dirty cache lines from those
writable mappings when we set up and write to these fixmaps. Do you
still see a problem?
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/2] arm: use fixmap for text patching when text is RO
@ 2014-04-10 19:34 ` Rabin Vincent
0 siblings, 0 replies; 20+ messages in thread
From: Rabin Vincent @ 2014-04-10 19:34 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Apr 07, 2014 at 02:57:51PM +0100, Jon Medhurst (Tixy) wrote:
> How does fixmap cope with cache colouring? Looking at the implementation
> it looks like it doesn't and so fixmap use on ARM is possibly buggy.
>
> For the text patching case where we know there are no writeable mappings
> [1] this should be OK if we used set_fixmap_nocache here, so long as we
> also invalidated the dcache later for the proper virtual address.
OK. The dcache invalidation for the proper virtual address is btw
already there via the call to flush_icache_range().
> [1] Can we know there are no writeable mappings though, the ftrace code
> modifying patches from Kees Cook have there own way of modifying text
> code permissions.
The ftrace patches does the modifications and the cache cleaning in
stop_machine(), so there should not be any dirty cache lines from those
writable mappings when we set up and write to these fixmaps. Do you
still see a problem?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] arm: use fixmap for text patching when text is RO
2014-04-04 21:27 ` Rabin Vincent
@ 2014-04-23 21:09 ` Kees Cook
-1 siblings, 0 replies; 20+ messages in thread
From: Kees Cook @ 2014-04-23 21:09 UTC (permalink / raw)
To: Rabin Vincent
Cc: linux-arm-kernel, LKML, Laura Abbott, Jon Medhurst, Doug Anderson
On Fri, Apr 4, 2014 at 2:27 PM, Rabin Vincent <rabin@rab.in> wrote:
> Use fixmaps for text patching when the kernel text is read-only,
> inspired by x86. This makes jump labels and kprobes work with the
> currently available CONFIG_DEBUG_SET_MODULE_RONX and the upcoming
> CONFIG_DEBUG_RODATA options.
>
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
> arch/arm/include/asm/fixmap.h | 3 ++
> arch/arm/kernel/jump_label.c | 2 +-
> arch/arm/kernel/patch.c | 66 ++++++++++++++++++++++++++++++++++++++-----
> arch/arm/kernel/patch.h | 12 +++++++-
> 4 files changed, 74 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
> index 55ed076..79c1719 100644
> --- a/arch/arm/include/asm/fixmap.h
> +++ b/arch/arm/include/asm/fixmap.h
> @@ -18,6 +18,9 @@
> #define FIXADDR_TOP (FIXADDR_END - PAGE_SIZE)
>
> enum fixed_addresses {
> + FIX_TEXT_POKE0,
> + FIX_TEXT_POKE1,
> +
> FIX_KMAP_BEGIN,
> FIX_KMAP_END = (FIXADDR_TOP - FIXADDR_START) >> PAGE_SHIFT,
> __end_of_fixed_addresses
> diff --git a/arch/arm/kernel/jump_label.c b/arch/arm/kernel/jump_label.c
> index 4ce4f78..afeeb9e 100644
> --- a/arch/arm/kernel/jump_label.c
> +++ b/arch/arm/kernel/jump_label.c
> @@ -19,7 +19,7 @@ static void __arch_jump_label_transform(struct jump_entry *entry,
> insn = arm_gen_nop();
>
> if (is_static)
> - __patch_text(addr, insn);
> + __patch_text_early(addr, insn);
> else
> patch_text(addr, insn);
> }
> diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c
> index 07314af..761c5b6 100644
> --- a/arch/arm/kernel/patch.c
> +++ b/arch/arm/kernel/patch.c
> @@ -1,8 +1,11 @@
> #include <linux/kernel.h>
> +#include <linux/spinlock.h>
> #include <linux/kprobes.h>
> +#include <linux/mm.h>
> #include <linux/stop_machine.h>
>
> #include <asm/cacheflush.h>
> +#include <asm/fixmap.h>
> #include <asm/smp_plat.h>
> #include <asm/opcodes.h>
>
> @@ -13,21 +16,67 @@ struct patch {
> unsigned int insn;
> };
>
> -void __kprobes __patch_text(void *addr, unsigned int insn)
> +static DEFINE_SPINLOCK(patch_lock);
> +
> +static void __kprobes *patch_map(void *addr, int fixmap, unsigned long *flags)
> +{
> + unsigned int uintaddr = (uintptr_t) addr;
> + bool module = !core_kernel_text(uintaddr);
> + struct page *page;
> +
> + if (module && IS_ENABLED(CONFIG_DEBUG_SET_MODULE_RONX)) {
> + page = vmalloc_to_page(addr);
> + } else if (!module && IS_ENABLED(CONFIG_ARM_KERNMEM_PERMS)) {
> + page = virt_to_page(addr);
> + } else {
> + return addr;
> + }
> +
> + if (flags)
> + spin_lock_irqsave(&patch_lock, *flags);
> +
> + set_fixmap(fixmap, page_to_phys(page));
> +
> + return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK));
> +}
> +
> +static void __kprobes patch_unmap(int fixmap, unsigned long *flags)
> +{
> + clear_fixmap(fixmap);
> +
> + if (flags)
> + spin_unlock_irqrestore(&patch_lock, *flags);
> +}
> +
> +void __kprobes __patch_text_real(void *addr, unsigned int insn, bool remap)
> {
> bool thumb2 = IS_ENABLED(CONFIG_THUMB2_KERNEL);
> + unsigned int uintaddr = (uintptr_t) addr;
> + unsigned long flags;
> + void *waddr = addr;
> int size;
>
> + if (remap)
> + waddr = patch_map(addr, FIX_TEXT_POKE0, &flags);
> +
> if (thumb2 && __opcode_is_thumb16(insn)) {
> - *(u16 *)addr = __opcode_to_mem_thumb16(insn);
> + *(u16 *)waddr = __opcode_to_mem_thumb16(insn);
> size = sizeof(u16);
> - } else if (thumb2 && ((uintptr_t)addr & 2)) {
> + } else if (thumb2 && (uintaddr & 2)) {
> + bool twopage = (uintaddr & ~PAGE_MASK) == PAGE_SIZE - 2;
> u16 first = __opcode_thumb32_first(insn);
> u16 second = __opcode_thumb32_second(insn);
> - u16 *addrh = addr;
> + u16 *addrh0 = waddr;
> + u16 *addrh1 = waddr + 2;
> +
> + if (remap && twopage)
> + addrh1 = patch_map(addr + 2, FIX_TEXT_POKE1, NULL);
>
> - addrh[0] = __opcode_to_mem_thumb16(first);
> - addrh[1] = __opcode_to_mem_thumb16(second);
> + *addrh0 = __opcode_to_mem_thumb16(first);
> + *addrh1 = __opcode_to_mem_thumb16(second);
> +
> + if (twopage && addrh1 != addr + 2)
> + patch_unmap(FIX_TEXT_POKE1, NULL);
>
> size = sizeof(u32);
> } else {
> @@ -36,10 +85,13 @@ void __kprobes __patch_text(void *addr, unsigned int insn)
> else
> insn = __opcode_to_mem_arm(insn);
>
> - *(u32 *)addr = insn;
> + *(u32 *)waddr = insn;
> size = sizeof(u32);
> }
>
> + if (waddr != addr)
> + patch_unmap(FIX_TEXT_POKE0, &flags);
> +
> flush_icache_range((uintptr_t)(addr),
> (uintptr_t)(addr) + size);
> }
> diff --git a/arch/arm/kernel/patch.h b/arch/arm/kernel/patch.h
> index b4731f2..77e054c 100644
> --- a/arch/arm/kernel/patch.h
> +++ b/arch/arm/kernel/patch.h
> @@ -2,6 +2,16 @@
> #define _ARM_KERNEL_PATCH_H
>
> void patch_text(void *addr, unsigned int insn);
> -void __patch_text(void *addr, unsigned int insn);
> +void __patch_text_real(void *addr, unsigned int insn, bool remap);
> +
> +static inline void __patch_text(void *addr, unsigned int insn)
> +{
> + __patch_text_real(addr, insn, true);
> +}
> +
> +static inline void __patch_text_early(void *addr, unsigned int insn)
> +{
> + __patch_text_real(addr, insn, false);
> +}
>
> #endif
> --
> 1.9.1
>
I think we can use this for the kgdb case too. Has anyone tried that
yet? Perhaps implementing text_poke() in terms of patch_map/unmap
calls would be best? Then the arm-specific kgdb hooks can use that,
since it does the icache flushing separately.
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH 2/2] arm: use fixmap for text patching when text is RO
@ 2014-04-23 21:09 ` Kees Cook
0 siblings, 0 replies; 20+ messages in thread
From: Kees Cook @ 2014-04-23 21:09 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Apr 4, 2014 at 2:27 PM, Rabin Vincent <rabin@rab.in> wrote:
> Use fixmaps for text patching when the kernel text is read-only,
> inspired by x86. This makes jump labels and kprobes work with the
> currently available CONFIG_DEBUG_SET_MODULE_RONX and the upcoming
> CONFIG_DEBUG_RODATA options.
>
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
> arch/arm/include/asm/fixmap.h | 3 ++
> arch/arm/kernel/jump_label.c | 2 +-
> arch/arm/kernel/patch.c | 66 ++++++++++++++++++++++++++++++++++++++-----
> arch/arm/kernel/patch.h | 12 +++++++-
> 4 files changed, 74 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
> index 55ed076..79c1719 100644
> --- a/arch/arm/include/asm/fixmap.h
> +++ b/arch/arm/include/asm/fixmap.h
> @@ -18,6 +18,9 @@
> #define FIXADDR_TOP (FIXADDR_END - PAGE_SIZE)
>
> enum fixed_addresses {
> + FIX_TEXT_POKE0,
> + FIX_TEXT_POKE1,
> +
> FIX_KMAP_BEGIN,
> FIX_KMAP_END = (FIXADDR_TOP - FIXADDR_START) >> PAGE_SHIFT,
> __end_of_fixed_addresses
> diff --git a/arch/arm/kernel/jump_label.c b/arch/arm/kernel/jump_label.c
> index 4ce4f78..afeeb9e 100644
> --- a/arch/arm/kernel/jump_label.c
> +++ b/arch/arm/kernel/jump_label.c
> @@ -19,7 +19,7 @@ static void __arch_jump_label_transform(struct jump_entry *entry,
> insn = arm_gen_nop();
>
> if (is_static)
> - __patch_text(addr, insn);
> + __patch_text_early(addr, insn);
> else
> patch_text(addr, insn);
> }
> diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c
> index 07314af..761c5b6 100644
> --- a/arch/arm/kernel/patch.c
> +++ b/arch/arm/kernel/patch.c
> @@ -1,8 +1,11 @@
> #include <linux/kernel.h>
> +#include <linux/spinlock.h>
> #include <linux/kprobes.h>
> +#include <linux/mm.h>
> #include <linux/stop_machine.h>
>
> #include <asm/cacheflush.h>
> +#include <asm/fixmap.h>
> #include <asm/smp_plat.h>
> #include <asm/opcodes.h>
>
> @@ -13,21 +16,67 @@ struct patch {
> unsigned int insn;
> };
>
> -void __kprobes __patch_text(void *addr, unsigned int insn)
> +static DEFINE_SPINLOCK(patch_lock);
> +
> +static void __kprobes *patch_map(void *addr, int fixmap, unsigned long *flags)
> +{
> + unsigned int uintaddr = (uintptr_t) addr;
> + bool module = !core_kernel_text(uintaddr);
> + struct page *page;
> +
> + if (module && IS_ENABLED(CONFIG_DEBUG_SET_MODULE_RONX)) {
> + page = vmalloc_to_page(addr);
> + } else if (!module && IS_ENABLED(CONFIG_ARM_KERNMEM_PERMS)) {
> + page = virt_to_page(addr);
> + } else {
> + return addr;
> + }
> +
> + if (flags)
> + spin_lock_irqsave(&patch_lock, *flags);
> +
> + set_fixmap(fixmap, page_to_phys(page));
> +
> + return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK));
> +}
> +
> +static void __kprobes patch_unmap(int fixmap, unsigned long *flags)
> +{
> + clear_fixmap(fixmap);
> +
> + if (flags)
> + spin_unlock_irqrestore(&patch_lock, *flags);
> +}
> +
> +void __kprobes __patch_text_real(void *addr, unsigned int insn, bool remap)
> {
> bool thumb2 = IS_ENABLED(CONFIG_THUMB2_KERNEL);
> + unsigned int uintaddr = (uintptr_t) addr;
> + unsigned long flags;
> + void *waddr = addr;
> int size;
>
> + if (remap)
> + waddr = patch_map(addr, FIX_TEXT_POKE0, &flags);
> +
> if (thumb2 && __opcode_is_thumb16(insn)) {
> - *(u16 *)addr = __opcode_to_mem_thumb16(insn);
> + *(u16 *)waddr = __opcode_to_mem_thumb16(insn);
> size = sizeof(u16);
> - } else if (thumb2 && ((uintptr_t)addr & 2)) {
> + } else if (thumb2 && (uintaddr & 2)) {
> + bool twopage = (uintaddr & ~PAGE_MASK) == PAGE_SIZE - 2;
> u16 first = __opcode_thumb32_first(insn);
> u16 second = __opcode_thumb32_second(insn);
> - u16 *addrh = addr;
> + u16 *addrh0 = waddr;
> + u16 *addrh1 = waddr + 2;
> +
> + if (remap && twopage)
> + addrh1 = patch_map(addr + 2, FIX_TEXT_POKE1, NULL);
>
> - addrh[0] = __opcode_to_mem_thumb16(first);
> - addrh[1] = __opcode_to_mem_thumb16(second);
> + *addrh0 = __opcode_to_mem_thumb16(first);
> + *addrh1 = __opcode_to_mem_thumb16(second);
> +
> + if (twopage && addrh1 != addr + 2)
> + patch_unmap(FIX_TEXT_POKE1, NULL);
>
> size = sizeof(u32);
> } else {
> @@ -36,10 +85,13 @@ void __kprobes __patch_text(void *addr, unsigned int insn)
> else
> insn = __opcode_to_mem_arm(insn);
>
> - *(u32 *)addr = insn;
> + *(u32 *)waddr = insn;
> size = sizeof(u32);
> }
>
> + if (waddr != addr)
> + patch_unmap(FIX_TEXT_POKE0, &flags);
> +
> flush_icache_range((uintptr_t)(addr),
> (uintptr_t)(addr) + size);
> }
> diff --git a/arch/arm/kernel/patch.h b/arch/arm/kernel/patch.h
> index b4731f2..77e054c 100644
> --- a/arch/arm/kernel/patch.h
> +++ b/arch/arm/kernel/patch.h
> @@ -2,6 +2,16 @@
> #define _ARM_KERNEL_PATCH_H
>
> void patch_text(void *addr, unsigned int insn);
> -void __patch_text(void *addr, unsigned int insn);
> +void __patch_text_real(void *addr, unsigned int insn, bool remap);
> +
> +static inline void __patch_text(void *addr, unsigned int insn)
> +{
> + __patch_text_real(addr, insn, true);
> +}
> +
> +static inline void __patch_text_early(void *addr, unsigned int insn)
> +{
> + __patch_text_real(addr, insn, false);
> +}
>
> #endif
> --
> 1.9.1
>
I think we can use this for the kgdb case too. Has anyone tried that
yet? Perhaps implementing text_poke() in terms of patch_map/unmap
calls would be best? Then the arm-specific kgdb hooks can use that,
since it does the icache flushing separately.
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/2] arm: use fixmap for text patching when text is RO
2014-04-23 21:09 ` Kees Cook
@ 2014-04-23 22:51 ` Doug Anderson
-1 siblings, 0 replies; 20+ messages in thread
From: Doug Anderson @ 2014-04-23 22:51 UTC (permalink / raw)
To: Kees Cook
Cc: Rabin Vincent, linux-arm-kernel, LKML, Laura Abbott, Jon Medhurst
Kees,
On Wed, Apr 23, 2014 at 2:09 PM, Kees Cook <keescook@chromium.org> wrote:
> I think we can use this for the kgdb case too. Has anyone tried that
> yet? Perhaps implementing text_poke() in terms of patch_map/unmap
> calls would be best? Then the arm-specific kgdb hooks can use that,
> since it does the icache flushing separately.
Yes. I just tried using these patches and it worked for me. :) I
didn't actually implement text_poke(), though.
Atop our 3.8 tree, in case anyone is curious
* https://chromium-review.googlesource.com/196670
* https://chromium-review.googlesource.com/196671
* https://chromium-review.googlesource.com/196672
* https://chromium-review.googlesource.com/196673
* https://chromium-review.googlesource.com/196674
Please include me on the CC list for spins of your patches...
-Doug
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv2 2/2] arm: use fixmap for text patching when text is RO
2014-04-04 21:27 ` Rabin Vincent
@ 2014-04-24 21:28 ` Rabin Vincent
-1 siblings, 0 replies; 20+ messages in thread
From: Rabin Vincent @ 2014-04-24 21:28 UTC (permalink / raw)
To: linux-arm-kernel
Cc: linux-kernel, Kees Cook, Laura Abbott, Jon Medhurst,
Doug Anderson, Rabin Vincent
Use fixmaps for text patching when the kernel text is read-only,
inspired by x86. This makes jump labels and kprobes work with the
currently available CONFIG_DEBUG_SET_MODULE_RONX and the upcoming
CONFIG_DEBUG_RODATA options.
Signed-off-by: Rabin Vincent <rabin@rab.in>
---
v2: flush the fixmap if necessary, with flush_kernel_vmap_range()
arch/arm/include/asm/fixmap.h | 3 ++
arch/arm/kernel/jump_label.c | 2 +-
arch/arm/kernel/patch.c | 71 ++++++++++++++++++++++++++++++++++++++-----
arch/arm/kernel/patch.h | 12 +++++++-
4 files changed, 79 insertions(+), 9 deletions(-)
diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
index 55ed076..79c1719 100644
--- a/arch/arm/include/asm/fixmap.h
+++ b/arch/arm/include/asm/fixmap.h
@@ -18,6 +18,9 @@
#define FIXADDR_TOP (FIXADDR_END - PAGE_SIZE)
enum fixed_addresses {
+ FIX_TEXT_POKE0,
+ FIX_TEXT_POKE1,
+
FIX_KMAP_BEGIN,
FIX_KMAP_END = (FIXADDR_TOP - FIXADDR_START) >> PAGE_SHIFT,
__end_of_fixed_addresses
diff --git a/arch/arm/kernel/jump_label.c b/arch/arm/kernel/jump_label.c
index 4ce4f78..afeeb9e 100644
--- a/arch/arm/kernel/jump_label.c
+++ b/arch/arm/kernel/jump_label.c
@@ -19,7 +19,7 @@ static void __arch_jump_label_transform(struct jump_entry *entry,
insn = arm_gen_nop();
if (is_static)
- __patch_text(addr, insn);
+ __patch_text_early(addr, insn);
else
patch_text(addr, insn);
}
diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c
index 07314af..3515b8d 100644
--- a/arch/arm/kernel/patch.c
+++ b/arch/arm/kernel/patch.c
@@ -1,8 +1,11 @@
#include <linux/kernel.h>
+#include <linux/spinlock.h>
#include <linux/kprobes.h>
+#include <linux/mm.h>
#include <linux/stop_machine.h>
#include <asm/cacheflush.h>
+#include <asm/fixmap.h>
#include <asm/smp_plat.h>
#include <asm/opcodes.h>
@@ -13,21 +16,70 @@ struct patch {
unsigned int insn;
};
-void __kprobes __patch_text(void *addr, unsigned int insn)
+static DEFINE_SPINLOCK(patch_lock);
+
+static void __kprobes *patch_map(void *addr, int fixmap, unsigned long *flags)
+{
+ unsigned int uintaddr = (uintptr_t) addr;
+ bool module = !core_kernel_text(uintaddr);
+ struct page *page;
+
+ if (module && IS_ENABLED(CONFIG_DEBUG_SET_MODULE_RONX)) {
+ page = vmalloc_to_page(addr);
+ } else if (!module && IS_ENABLED(CONFIG_ARM_KERNMEM_PERMS)) {
+ page = virt_to_page(addr);
+ } else {
+ return addr;
+ }
+
+ if (flags)
+ spin_lock_irqsave(&patch_lock, *flags);
+
+ set_fixmap(fixmap, page_to_phys(page));
+
+ return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK));
+}
+
+static void __kprobes patch_unmap(int fixmap, unsigned long *flags)
+{
+ clear_fixmap(fixmap);
+
+ if (flags)
+ spin_unlock_irqrestore(&patch_lock, *flags);
+}
+
+void __kprobes __patch_text_real(void *addr, unsigned int insn, bool remap)
{
bool thumb2 = IS_ENABLED(CONFIG_THUMB2_KERNEL);
+ unsigned int uintaddr = (uintptr_t) addr;
+ bool twopage = false;
+ unsigned long flags;
+ void *waddr = addr;
int size;
+ if (remap)
+ waddr = patch_map(addr, FIX_TEXT_POKE0, &flags);
+
if (thumb2 && __opcode_is_thumb16(insn)) {
- *(u16 *)addr = __opcode_to_mem_thumb16(insn);
+ *(u16 *)waddr = __opcode_to_mem_thumb16(insn);
size = sizeof(u16);
- } else if (thumb2 && ((uintptr_t)addr & 2)) {
+ } else if (thumb2 && (uintaddr & 2)) {
u16 first = __opcode_thumb32_first(insn);
u16 second = __opcode_thumb32_second(insn);
- u16 *addrh = addr;
+ u16 *addrh0 = waddr;
+ u16 *addrh1 = waddr + 2;
- addrh[0] = __opcode_to_mem_thumb16(first);
- addrh[1] = __opcode_to_mem_thumb16(second);
+ twopage = (uintaddr & ~PAGE_MASK) == PAGE_SIZE - 2;
+ if (twopage && remap)
+ addrh1 = patch_map(addr + 2, FIX_TEXT_POKE1, NULL);
+
+ *addrh0 = __opcode_to_mem_thumb16(first);
+ *addrh1 = __opcode_to_mem_thumb16(second);
+
+ if (twopage && addrh1 != addr + 2) {
+ flush_kernel_vmap_range(addrh1, 2);
+ patch_unmap(FIX_TEXT_POKE1, NULL);
+ }
size = sizeof(u32);
} else {
@@ -36,10 +88,15 @@ void __kprobes __patch_text(void *addr, unsigned int insn)
else
insn = __opcode_to_mem_arm(insn);
- *(u32 *)addr = insn;
+ *(u32 *)waddr = insn;
size = sizeof(u32);
}
+ if (waddr != addr) {
+ flush_kernel_vmap_range(waddr, twopage ? size / 2 : size);
+ patch_unmap(FIX_TEXT_POKE0, &flags);
+ }
+
flush_icache_range((uintptr_t)(addr),
(uintptr_t)(addr) + size);
}
diff --git a/arch/arm/kernel/patch.h b/arch/arm/kernel/patch.h
index b4731f2..77e054c 100644
--- a/arch/arm/kernel/patch.h
+++ b/arch/arm/kernel/patch.h
@@ -2,6 +2,16 @@
#define _ARM_KERNEL_PATCH_H
void patch_text(void *addr, unsigned int insn);
-void __patch_text(void *addr, unsigned int insn);
+void __patch_text_real(void *addr, unsigned int insn, bool remap);
+
+static inline void __patch_text(void *addr, unsigned int insn)
+{
+ __patch_text_real(addr, insn, true);
+}
+
+static inline void __patch_text_early(void *addr, unsigned int insn)
+{
+ __patch_text_real(addr, insn, false);
+}
#endif
--
1.9.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCHv2 2/2] arm: use fixmap for text patching when text is RO
@ 2014-04-24 21:28 ` Rabin Vincent
0 siblings, 0 replies; 20+ messages in thread
From: Rabin Vincent @ 2014-04-24 21:28 UTC (permalink / raw)
To: linux-arm-kernel
Use fixmaps for text patching when the kernel text is read-only,
inspired by x86. This makes jump labels and kprobes work with the
currently available CONFIG_DEBUG_SET_MODULE_RONX and the upcoming
CONFIG_DEBUG_RODATA options.
Signed-off-by: Rabin Vincent <rabin@rab.in>
---
v2: flush the fixmap if necessary, with flush_kernel_vmap_range()
arch/arm/include/asm/fixmap.h | 3 ++
arch/arm/kernel/jump_label.c | 2 +-
arch/arm/kernel/patch.c | 71 ++++++++++++++++++++++++++++++++++++++-----
arch/arm/kernel/patch.h | 12 +++++++-
4 files changed, 79 insertions(+), 9 deletions(-)
diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
index 55ed076..79c1719 100644
--- a/arch/arm/include/asm/fixmap.h
+++ b/arch/arm/include/asm/fixmap.h
@@ -18,6 +18,9 @@
#define FIXADDR_TOP (FIXADDR_END - PAGE_SIZE)
enum fixed_addresses {
+ FIX_TEXT_POKE0,
+ FIX_TEXT_POKE1,
+
FIX_KMAP_BEGIN,
FIX_KMAP_END = (FIXADDR_TOP - FIXADDR_START) >> PAGE_SHIFT,
__end_of_fixed_addresses
diff --git a/arch/arm/kernel/jump_label.c b/arch/arm/kernel/jump_label.c
index 4ce4f78..afeeb9e 100644
--- a/arch/arm/kernel/jump_label.c
+++ b/arch/arm/kernel/jump_label.c
@@ -19,7 +19,7 @@ static void __arch_jump_label_transform(struct jump_entry *entry,
insn = arm_gen_nop();
if (is_static)
- __patch_text(addr, insn);
+ __patch_text_early(addr, insn);
else
patch_text(addr, insn);
}
diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c
index 07314af..3515b8d 100644
--- a/arch/arm/kernel/patch.c
+++ b/arch/arm/kernel/patch.c
@@ -1,8 +1,11 @@
#include <linux/kernel.h>
+#include <linux/spinlock.h>
#include <linux/kprobes.h>
+#include <linux/mm.h>
#include <linux/stop_machine.h>
#include <asm/cacheflush.h>
+#include <asm/fixmap.h>
#include <asm/smp_plat.h>
#include <asm/opcodes.h>
@@ -13,21 +16,70 @@ struct patch {
unsigned int insn;
};
-void __kprobes __patch_text(void *addr, unsigned int insn)
+static DEFINE_SPINLOCK(patch_lock);
+
+static void __kprobes *patch_map(void *addr, int fixmap, unsigned long *flags)
+{
+ unsigned int uintaddr = (uintptr_t) addr;
+ bool module = !core_kernel_text(uintaddr);
+ struct page *page;
+
+ if (module && IS_ENABLED(CONFIG_DEBUG_SET_MODULE_RONX)) {
+ page = vmalloc_to_page(addr);
+ } else if (!module && IS_ENABLED(CONFIG_ARM_KERNMEM_PERMS)) {
+ page = virt_to_page(addr);
+ } else {
+ return addr;
+ }
+
+ if (flags)
+ spin_lock_irqsave(&patch_lock, *flags);
+
+ set_fixmap(fixmap, page_to_phys(page));
+
+ return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK));
+}
+
+static void __kprobes patch_unmap(int fixmap, unsigned long *flags)
+{
+ clear_fixmap(fixmap);
+
+ if (flags)
+ spin_unlock_irqrestore(&patch_lock, *flags);
+}
+
+void __kprobes __patch_text_real(void *addr, unsigned int insn, bool remap)
{
bool thumb2 = IS_ENABLED(CONFIG_THUMB2_KERNEL);
+ unsigned int uintaddr = (uintptr_t) addr;
+ bool twopage = false;
+ unsigned long flags;
+ void *waddr = addr;
int size;
+ if (remap)
+ waddr = patch_map(addr, FIX_TEXT_POKE0, &flags);
+
if (thumb2 && __opcode_is_thumb16(insn)) {
- *(u16 *)addr = __opcode_to_mem_thumb16(insn);
+ *(u16 *)waddr = __opcode_to_mem_thumb16(insn);
size = sizeof(u16);
- } else if (thumb2 && ((uintptr_t)addr & 2)) {
+ } else if (thumb2 && (uintaddr & 2)) {
u16 first = __opcode_thumb32_first(insn);
u16 second = __opcode_thumb32_second(insn);
- u16 *addrh = addr;
+ u16 *addrh0 = waddr;
+ u16 *addrh1 = waddr + 2;
- addrh[0] = __opcode_to_mem_thumb16(first);
- addrh[1] = __opcode_to_mem_thumb16(second);
+ twopage = (uintaddr & ~PAGE_MASK) == PAGE_SIZE - 2;
+ if (twopage && remap)
+ addrh1 = patch_map(addr + 2, FIX_TEXT_POKE1, NULL);
+
+ *addrh0 = __opcode_to_mem_thumb16(first);
+ *addrh1 = __opcode_to_mem_thumb16(second);
+
+ if (twopage && addrh1 != addr + 2) {
+ flush_kernel_vmap_range(addrh1, 2);
+ patch_unmap(FIX_TEXT_POKE1, NULL);
+ }
size = sizeof(u32);
} else {
@@ -36,10 +88,15 @@ void __kprobes __patch_text(void *addr, unsigned int insn)
else
insn = __opcode_to_mem_arm(insn);
- *(u32 *)addr = insn;
+ *(u32 *)waddr = insn;
size = sizeof(u32);
}
+ if (waddr != addr) {
+ flush_kernel_vmap_range(waddr, twopage ? size / 2 : size);
+ patch_unmap(FIX_TEXT_POKE0, &flags);
+ }
+
flush_icache_range((uintptr_t)(addr),
(uintptr_t)(addr) + size);
}
diff --git a/arch/arm/kernel/patch.h b/arch/arm/kernel/patch.h
index b4731f2..77e054c 100644
--- a/arch/arm/kernel/patch.h
+++ b/arch/arm/kernel/patch.h
@@ -2,6 +2,16 @@
#define _ARM_KERNEL_PATCH_H
void patch_text(void *addr, unsigned int insn);
-void __patch_text(void *addr, unsigned int insn);
+void __patch_text_real(void *addr, unsigned int insn, bool remap);
+
+static inline void __patch_text(void *addr, unsigned int insn)
+{
+ __patch_text_real(addr, insn, true);
+}
+
+static inline void __patch_text_early(void *addr, unsigned int insn)
+{
+ __patch_text_real(addr, insn, false);
+}
#endif
--
1.9.2
^ permalink raw reply related [flat|nested] 20+ messages in thread