[PATCH] arm64: Fix text patching logic when using fixmap

[Kees Cook]

On Tue, Feb 24, 2015 at 8:30 AM, Marc Zyngier <marc.zyngier at arm.com> wrote:
> Patch 2f896d586610 ("arm64: use fixmap for text patching") changed
> the way we patch the kernel text, using a fixmap when the kernel or
> modules are flagged as read only.
>
> Unfortunately, a flaw in the logic makes it fall over when patching
> modules without CONFIG_DEBUG_SET_MODULE_RONX enabled:
>
> [...]
> [   32.032636] Call trace:
> [   32.032716] [<fffffe00003da0dc>] __copy_to_user+0x2c/0x60
> [   32.032837] [<fffffe0000099f08>] __aarch64_insn_write+0x94/0xf8
> [   32.033027] [<fffffe000009a0a0>] aarch64_insn_patch_text_nosync+0x18/0x58
> [   32.033200] [<fffffe000009c3ec>] ftrace_modify_code+0x58/0x84
> [   32.033363] [<fffffe000009c4e4>] ftrace_make_nop+0x3c/0x58
> [   32.033532] [<fffffe0000164420>] ftrace_process_locs+0x3d0/0x5c8
> [   32.033709] [<fffffe00001661cc>] ftrace_module_init+0x28/0x34
> [   32.033882] [<fffffe0000135148>] load_module+0xbb8/0xfc4
> [   32.034044] [<fffffe0000135714>] SyS_finit_module+0x94/0xc4
> [...]
>
> This is triggered by the use of virt_to_page() on a module address,
> which ends to pointing to Nowhereland if you're lucky, or corrupt
> your precious data if not.
>
> This patch fixes the logic by mimicking what is done on arm:
> - If we're patching a module and CONFIG_DEBUG_SET_MODULE_RONX is set,
>   use vmalloc_to_page().
> - If we're patching the kernel and CONFIG_DEBUG_RODATA is set,
>   use virt_to_page().
> - Otherwise, use the provided address, as we can write to it directly.
>
> Tested on 4.0-rc1 as a KVM guest.
>
> Reported-by: Richard W.M. Jones <rjones at redhat.com>
> Cc: Kees Cook <keescook at chromium.org>
> Cc: Mark Rutland <mark.rutland at arm.com>
> Cc: Laura Abbott <lauraa at codeaurora.org>
> Cc: Catalin Marinas <catalin.marinas at arm.com>
> Cc: Will Deacon <will.deacon at arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier at arm.com>

Good catch! Looks fine to me.

Reviewed-by: Kees Cook <keescook at chromium.org>

-Kees

> ---
>  arch/arm64/kernel/insn.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
> index 27d4864..c8eca88 100644
> --- a/arch/arm64/kernel/insn.c
> +++ b/arch/arm64/kernel/insn.c
> @@ -87,8 +87,10 @@ static void __kprobes *patch_map(void *addr, int fixmap)
>
>         if (module && IS_ENABLED(CONFIG_DEBUG_SET_MODULE_RONX))
>                 page = vmalloc_to_page(addr);
> -       else
> +       else if (!module && IS_ENABLED(CONFIG_DEBUG_RODATA))
>                 page = virt_to_page(addr);
> +       else
> +               return addr;
>
>         BUG_ON(!page);
>         set_fixmap(fixmap, page_to_phys(page));
> --
> 2.1.4
>



-- 
Kees Cook
Chrome OS Security