* [PATCH] arm64: Fix text patching logic when using fixmap
2015-02-24 16:30 [PATCH] arm64: Fix text patching logic when using fixmap Marc Zyngier
@ 2015-02-24 16:32 ` Kees Cook
2015-02-24 16:43 ` Mark Rutland
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Kees Cook @ 2015-02-24 16:32 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Feb 24, 2015 at 8:30 AM, Marc Zyngier <marc.zyngier@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@redhat.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Laura Abbott <lauraa@codeaurora.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Good catch! Looks fine to me.
Reviewed-by: Kees Cook <keescook@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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] arm64: Fix text patching logic when using fixmap
2015-02-24 16:30 [PATCH] arm64: Fix text patching logic when using fixmap Marc Zyngier
2015-02-24 16:32 ` Kees Cook
@ 2015-02-24 16:43 ` Mark Rutland
2015-02-24 18:11 ` Richard W.M. Jones
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Mark Rutland @ 2015-02-24 16:43 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Feb 24, 2015 at 04:30:21PM +0000, Marc Zyngier 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@redhat.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Laura Abbott <lauraa@codeaurora.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
This makes sense to me:
Acked-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> 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
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] arm64: Fix text patching logic when using fixmap
2015-02-24 16:30 [PATCH] arm64: Fix text patching logic when using fixmap Marc Zyngier
2015-02-24 16:32 ` Kees Cook
2015-02-24 16:43 ` Mark Rutland
@ 2015-02-24 18:11 ` Richard W.M. Jones
2015-02-24 23:58 ` Laura Abbott
2015-02-27 13:25 ` Jon Masters
4 siblings, 0 replies; 7+ messages in thread
From: Richard W.M. Jones @ 2015-02-24 18:11 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Feb 24, 2015 at 04:30:21PM +0000, Marc Zyngier 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@redhat.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Laura Abbott <lauraa@codeaurora.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Tested and works for me, thanks! Therefore:
Tested-by: Richard W.M. Jones <rjones@redhat.com>
Rich.
> 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
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] arm64: Fix text patching logic when using fixmap
2015-02-24 16:30 [PATCH] arm64: Fix text patching logic when using fixmap Marc Zyngier
` (2 preceding siblings ...)
2015-02-24 18:11 ` Richard W.M. Jones
@ 2015-02-24 23:58 ` Laura Abbott
2015-02-27 13:25 ` Jon Masters
4 siblings, 0 replies; 7+ messages in thread
From: Laura Abbott @ 2015-02-24 23:58 UTC (permalink / raw)
To: linux-arm-kernel
On 2/24/2015 8:30 AM, Marc Zyngier 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@redhat.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Laura Abbott <lauraa@codeaurora.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> 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));
>
Looks like I dropped this incorrectly between v6 and v7.
Acked-by: Laura Abbott <lauraa@codeaurora.org>
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] arm64: Fix text patching logic when using fixmap
2015-02-24 16:30 [PATCH] arm64: Fix text patching logic when using fixmap Marc Zyngier
` (3 preceding siblings ...)
2015-02-24 23:58 ` Laura Abbott
@ 2015-02-27 13:25 ` Jon Masters
2015-02-27 13:41 ` Jon Masters
4 siblings, 1 reply; 7+ messages in thread
From: Jon Masters @ 2015-02-27 13:25 UTC (permalink / raw)
To: linux-arm-kernel
On 02/24/2015 11:30 AM, Marc Zyngier 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@redhat.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Laura Abbott <lauraa@codeaurora.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
This fixes a crash on boot in the AHCI driver experienced by a test
Fedora Hosted arm64 kernel when running on various platforms.
Tested by me on AMD Seattle and confirmed fixing the issue.
Tested-by: Jon Masters <jcm@redhat.com>
> ---
> 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));
>
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] arm64: Fix text patching logic when using fixmap
2015-02-27 13:25 ` Jon Masters
@ 2015-02-27 13:41 ` Jon Masters
0 siblings, 0 replies; 7+ messages in thread
From: Jon Masters @ 2015-02-27 13:41 UTC (permalink / raw)
To: linux-arm-kernel
On 02/27/2015 08:25 AM, Jon Masters wrote:
> On 02/24/2015 11:30 AM, Marc Zyngier 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@redhat.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Laura Abbott <lauraa@codeaurora.org>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>
> This fixes a crash on boot in the AHCI driver experienced by a test
> Fedora Hosted arm64 kernel when running on various platforms.
>
> Tested by me on AMD Seattle and confirmed fixing the issue.
>
> Tested-by: Jon Masters <jcm@redhat.com>
(In case anyone cares also tested on APM Mustang and it fixes the same)
^ permalink raw reply [flat|nested] 7+ messages in thread