[5/5] arm: Enable IRQs before attempting to read user space in __und_usr

The Undef abort handler in the kernel reads the undefined instruction
from user space. If the page table was modified from another CPU, the
user access could fail and do_page_fault() will be executed with
interrupts disabled. This can potentially deadlock on ARM11MPCore or on
Cortex-A15 with erratum 798181 workaround enabled (both implying IPI for
TLB maintenance with page table lock held).

This patch enables the IRQs in __und_usr before attempting to read the
instruction from user space.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Ryan Mallon <rmallon@gmail.com>
---
 arch/arm/kernel/entry-armv.S       | 11 +++++++----
 arch/arm/kernel/iwmmxt.S           |  2 +-
 arch/arm/mach-ep93xx/crunch-bits.S |  2 +-
 arch/arm/vfp/entry.S               |  3 +--
 4 files changed, 10 insertions(+), 8 deletions(-)

Hi Catalin,

On Thu, Mar 13, 2014 at 11:45 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> The Undef abort handler in the kernel reads the undefined instruction
> from user space. If the page table was modified from another CPU, the
> user access could fail and do_page_fault() will be executed with
> interrupts disabled. This can potentially deadlock on ARM11MPCore or on
> Cortex-A15 with erratum 798181 workaround enabled (both implying IPI for
> TLB maintenance with page table lock held).
>
> This patch enables the IRQs in __und_usr before attempting to read the
> instruction from user space.
>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Hartley Sweeten <hsweeten@visionengravers.com>
> Cc: Ryan Mallon <rmallon@gmail.com>
> ---
>  arch/arm/kernel/entry-armv.S       | 11 +++++++----
>  arch/arm/kernel/iwmmxt.S           |  2 +-
>  arch/arm/mach-ep93xx/crunch-bits.S |  2 +-
>  arch/arm/vfp/entry.S               |  3 +--
>  4 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
> index 1879e8dd2acc..5fc897cf409b 100644
> --- a/arch/arm/kernel/entry-armv.S
> +++ b/arch/arm/kernel/entry-armv.S
> @@ -413,6 +413,11 @@ __und_usr:
>         @
>         adr     r9, BSYM(ret_from_exception)
>
> +       @ IRQs must be enabled before attempting to read the instruction from
> +       @ user space since that could cause a page/translation fault if the
> +       @ page table was modified by another CPU.
> +       enable_irq
> +
>         tst     r3, #PSR_T_BIT                  @ Thumb mode?
>         bne     __und_usr_thumb
>         sub     r4, r2, #4                      @ ARM instr at LR - 4

As the data abort is now handled in normal way, can we remove the
fixup handler for ldrt?

/*
 * The out of line fixup for the ldrt instructions above.
 */
        .pushsection .fixup, "ax"
        .align  2
4:      mov     pc, r9
        .popsection
        .pushsection __ex_table,"a"
        .long   1b, 4b
#if CONFIG_ARM_THUMB && __LINUX_ARM_ARCH__ >= 6 && CONFIG_CPU_V7
        .long   2b, 4b
        .long   3b, 4b
#endif
        .popsection


Thanks,
Arun

> @@ -517,7 +522,7 @@ ENDPROC(__und_usr)
>   *  r9  = normal "successful" return address
>   *  r10 = this threads thread_info structure
>   *  lr  = unrecognised instruction return address
> - * IRQs disabled, FIQs enabled.
> + * IRQs enabled, FIQs enabled.
>   */
>         @
>         @ Fall-through from Thumb-2 __und_usr
> @@ -624,7 +629,6 @@ call_fpe:
>  #endif
>
>  do_fpe:
> -       enable_irq
>         ldr     r4, .LCfp
>         add     r10, r10, #TI_FPSTATE           @ r10 = workspace
>         ldr     pc, [r4]                        @ Call FP module USR entry point
> @@ -652,8 +656,7 @@ __und_usr_fault_32:
>         b       1f
>  __und_usr_fault_16:
>         mov     r1, #2
> -1:     enable_irq
> -       mov     r0, sp
> +1:     mov     r0, sp
>         adr     lr, BSYM(ret_from_exception)
>         b       __und_fault
>  ENDPROC(__und_usr_fault_32)
> diff --git a/arch/arm/kernel/iwmmxt.S b/arch/arm/kernel/iwmmxt.S
> index c52f3e225aeb..da3117054712 100644
> --- a/arch/arm/kernel/iwmmxt.S
> +++ b/arch/arm/kernel/iwmmxt.S
> @@ -61,7 +61,7 @@
>   * r9  = ret_from_exception
>   * lr  = undefined instr exit
>   *
> - * called from prefetch exception handler with interrupts disabled
> + * called from prefetch exception handler with interrupts enabled
>   */
>
>  ENTRY(iwmmxt_task_enable)
> diff --git a/arch/arm/mach-ep93xx/crunch-bits.S b/arch/arm/mach-ep93xx/crunch-bits.S
> index 890c5df2b4fe..85e765534003 100644
> --- a/arch/arm/mach-ep93xx/crunch-bits.S
> +++ b/arch/arm/mach-ep93xx/crunch-bits.S
> @@ -62,7 +62,7 @@
>   * r9  = ret_from_exception
>   * lr  = undefined instr exit
>   *
> - * called from prefetch exception handler with interrupts disabled
> + * called from prefetch exception handler with interrupts enabled
>   */
>  ENTRY(crunch_task_enable)
>         inc_preempt_count r10, r3
> diff --git a/arch/arm/vfp/entry.S b/arch/arm/vfp/entry.S
> index f0759e70fb86..fe6ca574d093 100644
> --- a/arch/arm/vfp/entry.S
> +++ b/arch/arm/vfp/entry.S
> @@ -22,11 +22,10 @@
>  @  r9  = normal "successful" return address
>  @  r10 = this threads thread_info structure
>  @  lr  = unrecognised instruction return address
> -@  IRQs disabled.
> +@  IRQs enabled.
>  @
>  ENTRY(do_vfp)
>         inc_preempt_count r10, r4
> -       enable_irq
>         ldr     r4, .LCvfp
>         ldr     r11, [r10, #TI_CPU]     @ CPU number
>         add     r10, r10, #TI_VFPSTATE  @ r10 = workspace
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Apr 01, 2014 at 05:54:15PM +0530, Arun KS wrote:
> As the data abort is now handled in normal way, can we remove the
> fixup handler for ldrt?

No, because all instructions which access userspace, and therefore may
fault, must be given a fixup handler so the kernel knows what to do
should it not be able to supply the page.

Omitting that marking means the only other option in that case is for
the kernel to oops.

Hi Arun,

On Fri, Apr 04, 2014 at 12:29:04PM +0100, Arun KS wrote:
> On Tue, Apr 1, 2014 at 8:18 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Tue, Apr 01, 2014 at 05:54:15PM +0530, Arun KS wrote:
> >> As the data abort is now handled in normal way, can we remove the
> >> fixup handler for ldrt?
> >
> > No, because all instructions which access userspace, and therefore may
> > fault, must be given a fixup handler so the kernel knows what to do
> > should it not be able to supply the page.
> >
> > Omitting that marking means the only other option in that case is for
> > the kernel to oops.
> 
> I have a concern regarding the fixup handler.
> Fixup handler returns to the next instruction which has caused the
> undef execption, rather than going to the same instruction.
> 
> ARM ARM says that after undefined exception, the pc will be pointing
> to the next instruction.
> ie +4 offset in case of ARM and +2 in case of Thumb.
> 
> And there is no correction offset passed to vector_stub in case of
> undef exception.
> 
> File: arch/arm/kernel/entry-armv.S +1085
> vector_stub     und, UND_MODE
> 
> During an undefined exception, in normal scenario(ie when ldrt
> instruction does not cause an abort) after resorting the context in
> VFP hardware, the PC is modified as show below before jumping to
> ret_from_exception which is in r9.
> 
> File: arch/arm/vfp/vfphw.S +169
> @ The context stored in the VFP hardware is up to date with this thread
> vfp_hw_state_valid:
>         tst     r1, #FPEXC_EX
>         bne     process_exception       @ might as well handle the pending
>                                         @ exception before retrying branch
>                                         @ out before setting an FPEXC that
>                                         @ stops us reading stuff
>         VFPFMXR FPEXC, r1               @ Restore FPEXC last
>         sub     r2, r2, #4              @ Retry current instruction - if Thumb
>         str     r2, [sp, #S_PC]         @ mode it's two 16-bit instructions,
>                                         @ else it's one 32-bit instruction, so
>                                         @ always subtract 4 from the following
>                                         @ instruction address.
> 
> But if ldrt results in an abort, we reach the fixup handler and return
> to ret_from_execption without correcting the pc.

If ldrt results in an abort, with my patches for enabling the IRQ we
should not execute the fixup handler. The in-kernel page fault would
handled and the ldrt instruction re-executed.

I think we could still get to the fixup handler if one thread is
triggering the undef while another thread (different CPU) is munmap'ing
the page (very unlikely case though). The fault caused by ldrt in
__und_usr wouldn't be fully handled, falling back to the fixup.

> From d72e15d92c1016ce3b1c7c7da01ca60cf21340d5 Mon Sep 17 00:00:00 2001
> From: Arun KS <getarunks@gmail.com>
> Date: Fri, 4 Apr 2014 16:42:58 +0530
> Subject: Modify fixup handler to re-execute the original instruction
> 
> Signed-off-by: Danesh Petigara <dpetigara@broadcom.com>
> Signed-off-by: Vinayak Menon <vinayakm.list@gmail.com>

Some more text here is useful, like what you explained above, just add
it to the commit log.

> ---
>  arch/arm/kernel/entry-armv.S |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
> index 1879e8d..e801c7d 100644
> --- a/arch/arm/kernel/entry-armv.S
> +++ b/arch/arm/kernel/entry-armv.S
> @@ -484,7 +484,8 @@ ENDPROC(__und_usr)
>   */
>         .pushsection .fixup, "ax"
>         .align  2
> -4:     mov     pc, r9
> +4:     str     r4, [sp, #S_PC]
> +       mov     pc, r9
>         .popsection
>         .pushsection __ex_table,"a"
>         .long   1b, 4b

I think this patch should cover the above case and a subsequent
re-execution of the address in user space would trigger the prefetch
abort.

On its own, I think the above patch would also work without my other
patches. But the effect is that on ldrt fault we re-execute the user
instruction hoping that it will trigger the prefetch abort, fix it up in
the kernel and return to that instruction again. It may be worth as
cc: stable (assuming that anyone tests it properly).

Otherwise:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>