| Message ID | 1416606645-25633-7-git-send-email-lauraa@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Laura,
It looks like "early" means before we've set up the strict page
permissions, though as far as I can tell the fixmap will already be
available before we do any patching. Could we not always use the fixmap
for patching? Then we only need the patch_{map,unmap} additions, and not
the changes to distinguish the early cases.
From testing on Juno with defconfig, all of the early patches were
avoidable NOP -> NOP changes as part of static key initialisation, which
I think we can skip similarly to x86 (I'll send a patch shortly). All other
patches were not early and went via the fixmap.
Even with the avoidable NOP -> NOP patching I did not see a noticeable
boot time difference from forcing the use of the fixmap.
Thanks,
Mark.
On Fri, Nov 21, 2014 at 09:50:43PM +0000, Laura Abbott wrote:
> When kernel text is marked as read only, it cannot be modified directly.
> Use a fixmap to modify the text instead in a similar manner to
> x86 and arm.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Tested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> ---
> arch/arm64/include/asm/fixmap.h | 1 +
> arch/arm64/include/asm/insn.h | 2 ++
> arch/arm64/kernel/insn.c | 72 +++++++++++++++++++++++++++++++++++++++--
> arch/arm64/kernel/jump_label.c | 2 +-
> 4 files changed, 73 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/include/asm/fixmap.h b/arch/arm64/include/asm/fixmap.h
> index db26a2f2..2cd4b0d 100644
> --- a/arch/arm64/include/asm/fixmap.h
> +++ b/arch/arm64/include/asm/fixmap.h
> @@ -48,6 +48,7 @@ enum fixed_addresses {
>
> FIX_BTMAP_END = __end_of_permanent_fixed_addresses,
> FIX_BTMAP_BEGIN = FIX_BTMAP_END + TOTAL_FIX_BTMAPS - 1,
> + FIX_TEXT_POKE0,
> __end_of_fixed_addresses
> };
>
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index 56a9e63..f66853b 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -282,6 +282,7 @@ bool aarch64_insn_is_nop(u32 insn);
>
> int aarch64_insn_read(void *addr, u32 *insnp);
> int aarch64_insn_write(void *addr, u32 insn);
> +int aarch64_insn_write_early(void *addr, u32 insn);
> enum aarch64_insn_encoding_class aarch64_get_insn_class(u32 insn);
> u32 aarch64_insn_encode_immediate(enum aarch64_insn_imm_type type,
> u32 insn, u64 imm);
> @@ -352,6 +353,7 @@ u32 aarch64_insn_gen_logical_shifted_reg(enum aarch64_insn_register dst,
> bool aarch64_insn_hotpatch_safe(u32 old_insn, u32 new_insn);
>
> int aarch64_insn_patch_text_nosync(void *addr, u32 insn);
> +int __aarch64_insn_patch_text_nosync(void *addr, u32 insn, bool early);
> int aarch64_insn_patch_text_sync(void *addrs[], u32 insns[], int cnt);
> int aarch64_insn_patch_text(void *addrs[], u32 insns[], int cnt);
> #endif /* __ASSEMBLY__ */
> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
> index 8cd27fe..b2cad38 100644
> --- a/arch/arm64/kernel/insn.c
> +++ b/arch/arm64/kernel/insn.c
> @@ -19,12 +19,15 @@
> #include <linux/bitops.h>
> #include <linux/compiler.h>
> #include <linux/kernel.h>
> +#include <linux/mm.h>
> #include <linux/smp.h>
> +#include <linux/spinlock.h>
> #include <linux/stop_machine.h>
> #include <linux/uaccess.h>
>
> #include <asm/cacheflush.h>
> #include <asm/debug-monitors.h>
> +#include <asm/fixmap.h>
> #include <asm/insn.h>
>
> #define AARCH64_INSN_SF_BIT BIT(31)
> @@ -72,6 +75,36 @@ bool __kprobes aarch64_insn_is_nop(u32 insn)
> }
> }
>
> +static DEFINE_SPINLOCK(patch_lock);
> +
> +static void __kprobes *patch_map(void *addr, int fixmap, unsigned long *flags)
> +{
> + unsigned long 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_DEBUG_RODATA))
> + 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);
> +}
> /*
> * In ARMv8-A, A64 instructions have a fixed length of 32 bits and are always
> * little-endian.
> @@ -88,10 +121,34 @@ int __kprobes aarch64_insn_read(void *addr, u32 *insnp)
> return ret;
> }
>
> +static int __kprobes __aarch64_insn_write(void *addr, u32 insn, bool patch)
> +{
> + void *waddr = addr;
> + unsigned long flags;
> + int ret;
> +
> + if (patch)
> + waddr = patch_map(addr, FIX_TEXT_POKE0, &flags);
> +
> + ret = probe_kernel_write(waddr, &insn, AARCH64_INSN_SIZE);
> +
> + if (waddr != addr)
> + patch_unmap(FIX_TEXT_POKE0, &flags);
> +
> + return ret;
> +}
> +
> int __kprobes aarch64_insn_write(void *addr, u32 insn)
> {
> insn = cpu_to_le32(insn);
> - return probe_kernel_write(addr, &insn, AARCH64_INSN_SIZE);
> + return __aarch64_insn_write(addr, insn, true);
> +}
> +
> +int __kprobes aarch64_insn_write_early(void *addr, u32 insn)
> +{
> + insn = cpu_to_le32(insn);
> + return __aarch64_insn_write(addr, insn, false);
> +
> }
>
> static bool __kprobes __aarch64_insn_hotpatch_safe(u32 insn)
> @@ -124,7 +181,7 @@ bool __kprobes aarch64_insn_hotpatch_safe(u32 old_insn, u32 new_insn)
> __aarch64_insn_hotpatch_safe(new_insn);
> }
>
> -int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn)
> +int __kprobes __aarch64_insn_patch_text_nosync(void *addr, u32 insn, bool early)
> {
> u32 *tp = addr;
> int ret;
> @@ -133,7 +190,11 @@ int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn)
> if ((uintptr_t)tp & 0x3)
> return -EINVAL;
>
> - ret = aarch64_insn_write(tp, insn);
> + if (early)
> + ret = aarch64_insn_write_early(tp, insn);
> + else
> + ret = aarch64_insn_write(tp, insn);
> +
> if (ret == 0)
> flush_icache_range((uintptr_t)tp,
> (uintptr_t)tp + AARCH64_INSN_SIZE);
> @@ -141,6 +202,11 @@ int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn)
> return ret;
> }
>
> +int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn)
> +{
> + return __aarch64_insn_patch_text_nosync(addr, insn, false);
> +}
> +
> struct aarch64_insn_patch {
> void **text_addrs;
> u32 *new_insns;
> diff --git a/arch/arm64/kernel/jump_label.c b/arch/arm64/kernel/jump_label.c
> index 263a166..9ac30bb 100644
> --- a/arch/arm64/kernel/jump_label.c
> +++ b/arch/arm64/kernel/jump_label.c
> @@ -38,7 +38,7 @@ static void __arch_jump_label_transform(struct jump_entry *entry,
> }
>
> if (is_static)
> - aarch64_insn_patch_text_nosync(addr, insn);
> + __aarch64_insn_patch_text_nosync(addr, insn, true);
> else
> aarch64_insn_patch_text(&addr, &insn, 1);
> }
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
>
>
On 11/25/2014 9:04 AM, Mark Rutland wrote: > Hi Laura, > > It looks like "early" means before we've set up the strict page > permissions, though as far as I can tell the fixmap will already be > available before we do any patching. Could we not always use the fixmap > for patching? Then we only need the patch_{map,unmap} additions, and not > the changes to distinguish the early cases. > > From testing on Juno with defconfig, all of the early patches were > avoidable NOP -> NOP changes as part of static key initialisation, which > I think we can skip similarly to x86 (I'll send a patch shortly). All other > patches were not early and went via the fixmap. > > Even with the avoidable NOP -> NOP patching I did not see a noticeable > boot time difference from forcing the use of the fixmap. > I was basing it off of the arm version which needed the early option. If arm64 doesn't need it I'll drop it. > Thanks, > Mark. > Thanks, Laura
On Tue, Nov 25, 2014 at 06:54:14PM +0000, Laura Abbott wrote: > On 11/25/2014 9:04 AM, Mark Rutland wrote: > > Hi Laura, > > > > It looks like "early" means before we've set up the strict page > > permissions, though as far as I can tell the fixmap will already be > > available before we do any patching. Could we not always use the fixmap > > for patching? Then we only need the patch_{map,unmap} additions, and not > > the changes to distinguish the early cases. > > > > From testing on Juno with defconfig, all of the early patches were > > avoidable NOP -> NOP changes as part of static key initialisation, which > > I think we can skip similarly to x86 (I'll send a patch shortly). All other > > patches were not early and went via the fixmap. > > > > Even with the avoidable NOP -> NOP patching I did not see a noticeable > > boot time difference from forcing the use of the fixmap. > > > > I was basing it off of the arm version which needed the early option. > If arm64 doesn't need it I'll drop it. Given that it only determines whether or not to use the fixmap, and we can always use the fixmap, I think we can drop it. Thanks, Mark.
On Wed, Nov 26, 2014 at 04:18:02PM +0000, Mark Rutland wrote: > On Tue, Nov 25, 2014 at 06:54:14PM +0000, Laura Abbott wrote: > > On 11/25/2014 9:04 AM, Mark Rutland wrote: > > > Hi Laura, > > > > > > It looks like "early" means before we've set up the strict page > > > permissions, though as far as I can tell the fixmap will already be > > > available before we do any patching. Could we not always use the fixmap > > > for patching? Then we only need the patch_{map,unmap} additions, and not > > > the changes to distinguish the early cases. > > > > > > From testing on Juno with defconfig, all of the early patches were > > > avoidable NOP -> NOP changes as part of static key initialisation, which > > > I think we can skip similarly to x86 (I'll send a patch shortly). All other > > > patches were not early and went via the fixmap. > > > > > > Even with the avoidable NOP -> NOP patching I did not see a noticeable > > > boot time difference from forcing the use of the fixmap. > > > > > > > I was basing it off of the arm version which needed the early option. > > If arm64 doesn't need it I'll drop it. > > Given that it only determines whether or not to use the fixmap, and we > can always use the fixmap, I think we can drop it. BTW, do we ever expect a write to the temporary POKE0 fixmap to fail? If not (I don't think it would fail), we can just override the __probe_kernel_write() to use set_fixmap() (maybe only when this feature is enabled).
diff --git a/arch/arm64/include/asm/fixmap.h b/arch/arm64/include/asm/fixmap.h index db26a2f2..2cd4b0d 100644 --- a/arch/arm64/include/asm/fixmap.h +++ b/arch/arm64/include/asm/fixmap.h @@ -48,6 +48,7 @@ enum fixed_addresses { FIX_BTMAP_END = __end_of_permanent_fixed_addresses, FIX_BTMAP_BEGIN = FIX_BTMAP_END + TOTAL_FIX_BTMAPS - 1, + FIX_TEXT_POKE0, __end_of_fixed_addresses }; diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h index 56a9e63..f66853b 100644 --- a/arch/arm64/include/asm/insn.h +++ b/arch/arm64/include/asm/insn.h @@ -282,6 +282,7 @@ bool aarch64_insn_is_nop(u32 insn); int aarch64_insn_read(void *addr, u32 *insnp); int aarch64_insn_write(void *addr, u32 insn); +int aarch64_insn_write_early(void *addr, u32 insn); enum aarch64_insn_encoding_class aarch64_get_insn_class(u32 insn); u32 aarch64_insn_encode_immediate(enum aarch64_insn_imm_type type, u32 insn, u64 imm); @@ -352,6 +353,7 @@ u32 aarch64_insn_gen_logical_shifted_reg(enum aarch64_insn_register dst, bool aarch64_insn_hotpatch_safe(u32 old_insn, u32 new_insn); int aarch64_insn_patch_text_nosync(void *addr, u32 insn); +int __aarch64_insn_patch_text_nosync(void *addr, u32 insn, bool early); int aarch64_insn_patch_text_sync(void *addrs[], u32 insns[], int cnt); int aarch64_insn_patch_text(void *addrs[], u32 insns[], int cnt); #endif /* __ASSEMBLY__ */ diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c index 8cd27fe..b2cad38 100644 --- a/arch/arm64/kernel/insn.c +++ b/arch/arm64/kernel/insn.c @@ -19,12 +19,15 @@ #include <linux/bitops.h> #include <linux/compiler.h> #include <linux/kernel.h> +#include <linux/mm.h> #include <linux/smp.h> +#include <linux/spinlock.h> #include <linux/stop_machine.h> #include <linux/uaccess.h> #include <asm/cacheflush.h> #include <asm/debug-monitors.h> +#include <asm/fixmap.h> #include <asm/insn.h> #define AARCH64_INSN_SF_BIT BIT(31) @@ -72,6 +75,36 @@ bool __kprobes aarch64_insn_is_nop(u32 insn) } } +static DEFINE_SPINLOCK(patch_lock); + +static void __kprobes *patch_map(void *addr, int fixmap, unsigned long *flags) +{ + unsigned long 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_DEBUG_RODATA)) + 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); +} /* * In ARMv8-A, A64 instructions have a fixed length of 32 bits and are always * little-endian. @@ -88,10 +121,34 @@ int __kprobes aarch64_insn_read(void *addr, u32 *insnp) return ret; } +static int __kprobes __aarch64_insn_write(void *addr, u32 insn, bool patch) +{ + void *waddr = addr; + unsigned long flags; + int ret; + + if (patch) + waddr = patch_map(addr, FIX_TEXT_POKE0, &flags); + + ret = probe_kernel_write(waddr, &insn, AARCH64_INSN_SIZE); + + if (waddr != addr) + patch_unmap(FIX_TEXT_POKE0, &flags); + + return ret; +} + int __kprobes aarch64_insn_write(void *addr, u32 insn) { insn = cpu_to_le32(insn); - return probe_kernel_write(addr, &insn, AARCH64_INSN_SIZE); + return __aarch64_insn_write(addr, insn, true); +} + +int __kprobes aarch64_insn_write_early(void *addr, u32 insn) +{ + insn = cpu_to_le32(insn); + return __aarch64_insn_write(addr, insn, false); + } static bool __kprobes __aarch64_insn_hotpatch_safe(u32 insn) @@ -124,7 +181,7 @@ bool __kprobes aarch64_insn_hotpatch_safe(u32 old_insn, u32 new_insn) __aarch64_insn_hotpatch_safe(new_insn); } -int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn) +int __kprobes __aarch64_insn_patch_text_nosync(void *addr, u32 insn, bool early) { u32 *tp = addr; int ret; @@ -133,7 +190,11 @@ int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn) if ((uintptr_t)tp & 0x3) return -EINVAL; - ret = aarch64_insn_write(tp, insn); + if (early) + ret = aarch64_insn_write_early(tp, insn); + else + ret = aarch64_insn_write(tp, insn); + if (ret == 0) flush_icache_range((uintptr_t)tp, (uintptr_t)tp + AARCH64_INSN_SIZE); @@ -141,6 +202,11 @@ int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn) return ret; } +int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn) +{ + return __aarch64_insn_patch_text_nosync(addr, insn, false); +} + struct aarch64_insn_patch { void **text_addrs; u32 *new_insns; diff --git a/arch/arm64/kernel/jump_label.c b/arch/arm64/kernel/jump_label.c index 263a166..9ac30bb 100644 --- a/arch/arm64/kernel/jump_label.c +++ b/arch/arm64/kernel/jump_label.c @@ -38,7 +38,7 @@ static void __arch_jump_label_transform(struct jump_entry *entry, } if (is_static) - aarch64_insn_patch_text_nosync(addr, insn); + __aarch64_insn_patch_text_nosync(addr, insn, true); else aarch64_insn_patch_text(&addr, &insn, 1); }