| Message ID | 20170404174727.35478-4-thgarnie@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 04, 2017 at 10:47:27AM -0700, Thomas Garnier wrote: > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S > index 43512d4d7df2..6d598e7051c3 100644 > --- a/arch/arm64/kernel/entry.S > +++ b/arch/arm64/kernel/entry.S > @@ -744,6 +744,10 @@ ENDPROC(cpu_switch_to) > ret_fast_syscall: > disable_irq // disable interrupts > str x0, [sp, #S_X0] // returned x0 > + ldr x2, [tsk, #TSK_TI_ADDR_LIMIT] // check addr limit change > + mov x1, #TASK_SIZE_64 > + cmp x2, x1 > + b.ne addr_limit_fail KERNEL_DS is set to the maximum address (-1UL), so it would be easier to check against this here and avoid a "mov". Even simpler if you'd check against bit 63 of the address for KERNEL_DS: ldr x1, [tsk, TSK_TI_ADDR_LIMIT] // check addr limit change tbnz x1, #63, addr_limit_fail // KERNEL_DS is -1UL > ldr x1, [tsk, #TSK_TI_FLAGS] // re-check for syscall tracing > and x2, x1, #_TIF_SYSCALL_WORK > cbnz x2, ret_fast_syscall_trace > @@ -771,6 +775,11 @@ work_pending: > */ > ret_to_user: > disable_irq // disable interrupts > + ldr x2, [tsk, #TSK_TI_ADDR_LIMIT] // check addr limit change > + mov x1, #TASK_SIZE_64 > + cmp x2, x1 > + b.ne addr_limit_fail Same here. > + > ldr x1, [tsk, #TSK_TI_FLAGS] > and x2, x1, #_TIF_WORK_MASK > cbnz x2, work_pending > @@ -779,6 +788,12 @@ finish_ret_to_user: > kernel_exit 0 > ENDPROC(ret_to_user) > > +addr_limit_fail: > + stp x0, lr, [sp,#-16]! > + bl asm_verify_pre_usermode_state > + ldp x0, lr, [sp],#16 > + ret lr Where is this supposed to return? What is the value of lr when branching to addr_limit_fail?
On Wed, Apr 5, 2017 at 7:22 AM, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Tue, Apr 04, 2017 at 10:47:27AM -0700, Thomas Garnier wrote: >> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S >> index 43512d4d7df2..6d598e7051c3 100644 >> --- a/arch/arm64/kernel/entry.S >> +++ b/arch/arm64/kernel/entry.S >> @@ -744,6 +744,10 @@ ENDPROC(cpu_switch_to) >> ret_fast_syscall: >> disable_irq // disable interrupts >> str x0, [sp, #S_X0] // returned x0 >> + ldr x2, [tsk, #TSK_TI_ADDR_LIMIT] // check addr limit change >> + mov x1, #TASK_SIZE_64 >> + cmp x2, x1 >> + b.ne addr_limit_fail > > KERNEL_DS is set to the maximum address (-1UL), so it would be easier to > check against this here and avoid a "mov". Even simpler if you'd check > against bit 63 of the address for KERNEL_DS: We also want to catch corruption so checking the 63 bit make sense. I will look for this change in the next iteration. > > ldr x1, [tsk, TSK_TI_ADDR_LIMIT] // check addr limit change > tbnz x1, #63, addr_limit_fail // KERNEL_DS is -1UL > >> ldr x1, [tsk, #TSK_TI_FLAGS] // re-check for syscall tracing >> and x2, x1, #_TIF_SYSCALL_WORK >> cbnz x2, ret_fast_syscall_trace >> @@ -771,6 +775,11 @@ work_pending: >> */ >> ret_to_user: >> disable_irq // disable interrupts >> + ldr x2, [tsk, #TSK_TI_ADDR_LIMIT] // check addr limit change >> + mov x1, #TASK_SIZE_64 >> + cmp x2, x1 >> + b.ne addr_limit_fail > > Same here. > >> + >> ldr x1, [tsk, #TSK_TI_FLAGS] >> and x2, x1, #_TIF_WORK_MASK >> cbnz x2, work_pending >> @@ -779,6 +788,12 @@ finish_ret_to_user: >> kernel_exit 0 >> ENDPROC(ret_to_user) >> >> +addr_limit_fail: >> + stp x0, lr, [sp,#-16]! >> + bl asm_verify_pre_usermode_state >> + ldp x0, lr, [sp],#16 >> + ret lr > > Where is this supposed to return? What is the value of lr when branching > to addr_limit_fail? It is not supposed to return. Do you think I should remove stp, ldp, ret and jut add a brk 0x100 or jmp/call a break/bug function? > > -- > Catalin
On Wed, Apr 05, 2017 at 07:36:17AM -0700, Thomas Garnier wrote: > On Wed, Apr 5, 2017 at 7:22 AM, Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Tue, Apr 04, 2017 at 10:47:27AM -0700, Thomas Garnier wrote: > >> + > >> ldr x1, [tsk, #TSK_TI_FLAGS] > >> and x2, x1, #_TIF_WORK_MASK > >> cbnz x2, work_pending > >> @@ -779,6 +788,12 @@ finish_ret_to_user: > >> kernel_exit 0 > >> ENDPROC(ret_to_user) > >> > >> +addr_limit_fail: > >> + stp x0, lr, [sp,#-16]! > >> + bl asm_verify_pre_usermode_state > >> + ldp x0, lr, [sp],#16 > >> + ret lr > > > > Where is this supposed to return? What is the value of lr when branching > > to addr_limit_fail? > > It is not supposed to return. Do you think I should remove stp, ldp, > ret and jut add a brk 0x100 or jmp/call a break/bug function? Can you not just make addr_limit_fail a C function which never returns (similar to what we to with bad_mode() on arm64)? Since addr_limit_fail is only called when the segment is not the right one, I don't really see why you need another call to asm_verify_pre_usermode_state() to do a similar check again. Just panic in addr_limit_fail (unless I misunderstood what you are trying to achieve).
On Wed, Apr 5, 2017 at 10:49 AM, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Wed, Apr 05, 2017 at 07:36:17AM -0700, Thomas Garnier wrote: >> On Wed, Apr 5, 2017 at 7:22 AM, Catalin Marinas <catalin.marinas@arm.com> wrote: >> > On Tue, Apr 04, 2017 at 10:47:27AM -0700, Thomas Garnier wrote: >> >> + >> >> ldr x1, [tsk, #TSK_TI_FLAGS] >> >> and x2, x1, #_TIF_WORK_MASK >> >> cbnz x2, work_pending >> >> @@ -779,6 +788,12 @@ finish_ret_to_user: >> >> kernel_exit 0 >> >> ENDPROC(ret_to_user) >> >> >> >> +addr_limit_fail: >> >> + stp x0, lr, [sp,#-16]! >> >> + bl asm_verify_pre_usermode_state >> >> + ldp x0, lr, [sp],#16 >> >> + ret lr >> > >> > Where is this supposed to return? What is the value of lr when branching >> > to addr_limit_fail? >> >> It is not supposed to return. Do you think I should remove stp, ldp, >> ret and jut add a brk 0x100 or jmp/call a break/bug function? > > Can you not just make addr_limit_fail a C function which never returns > (similar to what we to with bad_mode() on arm64)? Since addr_limit_fail > is only called when the segment is not the right one, I don't really see > why you need another call to asm_verify_pre_usermode_state() to do a > similar check again. Just panic in addr_limit_fail (unless I > misunderstood what you are trying to achieve). Calling asm_verify_pre_usermode_state has the advantage of having a clear BUG_ON for the error (versus a panic description). What do you think about replacing asm_verify_pre_usermode_state by a "address_limit_fail" function that still calls verify_pre_usermode_state but panic afterwards (because it should never return)? The assembly code would be easier to understand and in case of error the BUG_ON is clear for the user. > > -- > Catalin
On Wed, Apr 05, 2017 at 11:14:34AM -0700, Thomas Garnier wrote: > On Wed, Apr 5, 2017 at 10:49 AM, Catalin Marinas > <catalin.marinas@arm.com> wrote: > > On Wed, Apr 05, 2017 at 07:36:17AM -0700, Thomas Garnier wrote: > >> On Wed, Apr 5, 2017 at 7:22 AM, Catalin Marinas <catalin.marinas@arm.com> wrote: > >> > On Tue, Apr 04, 2017 at 10:47:27AM -0700, Thomas Garnier wrote: > >> >> + > >> >> ldr x1, [tsk, #TSK_TI_FLAGS] > >> >> and x2, x1, #_TIF_WORK_MASK > >> >> cbnz x2, work_pending > >> >> @@ -779,6 +788,12 @@ finish_ret_to_user: > >> >> kernel_exit 0 > >> >> ENDPROC(ret_to_user) > >> >> > >> >> +addr_limit_fail: > >> >> + stp x0, lr, [sp,#-16]! > >> >> + bl asm_verify_pre_usermode_state > >> >> + ldp x0, lr, [sp],#16 > >> >> + ret lr > >> > > >> > Where is this supposed to return? What is the value of lr when branching > >> > to addr_limit_fail? > >> > >> It is not supposed to return. Do you think I should remove stp, ldp, > >> ret and jut add a brk 0x100 or jmp/call a break/bug function? > > > > Can you not just make addr_limit_fail a C function which never returns > > (similar to what we to with bad_mode() on arm64)? Since addr_limit_fail > > is only called when the segment is not the right one, I don't really see > > why you need another call to asm_verify_pre_usermode_state() to do a > > similar check again. Just panic in addr_limit_fail (unless I > > misunderstood what you are trying to achieve). > > Calling asm_verify_pre_usermode_state has the advantage of having a > clear BUG_ON for the error (versus a panic description). > > What do you think about replacing asm_verify_pre_usermode_state by a > "address_limit_fail" function that still calls > verify_pre_usermode_state but panic afterwards (because it should > never return)? > > The assembly code would be easier to understand and in case of error > the BUG_ON is clear for the user. It looks fine to me, though I'd have to see the patch.
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index f2b0b528037d..0e86d87259f4 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -24,6 +24,7 @@ config ARM64 select ARCH_WANT_COMPAT_IPC_PARSE_VERSION select ARCH_WANT_FRAME_POINTERS select ARCH_HAS_UBSAN_SANITIZE_ALL + select ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE select ARM_AMBA select ARM_ARCH_TIMER select ARM_GIC diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index 43512d4d7df2..6d598e7051c3 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -744,6 +744,10 @@ ENDPROC(cpu_switch_to) ret_fast_syscall: disable_irq // disable interrupts str x0, [sp, #S_X0] // returned x0 + ldr x2, [tsk, #TSK_TI_ADDR_LIMIT] // check addr limit change + mov x1, #TASK_SIZE_64 + cmp x2, x1 + b.ne addr_limit_fail ldr x1, [tsk, #TSK_TI_FLAGS] // re-check for syscall tracing and x2, x1, #_TIF_SYSCALL_WORK cbnz x2, ret_fast_syscall_trace @@ -771,6 +775,11 @@ work_pending: */ ret_to_user: disable_irq // disable interrupts + ldr x2, [tsk, #TSK_TI_ADDR_LIMIT] // check addr limit change + mov x1, #TASK_SIZE_64 + cmp x2, x1 + b.ne addr_limit_fail + ldr x1, [tsk, #TSK_TI_FLAGS] and x2, x1, #_TIF_WORK_MASK cbnz x2, work_pending @@ -779,6 +788,12 @@ finish_ret_to_user: kernel_exit 0 ENDPROC(ret_to_user) +addr_limit_fail: + stp x0, lr, [sp,#-16]! + bl asm_verify_pre_usermode_state + ldp x0, lr, [sp],#16 + ret lr + /* * This is how we return from a fork. */
Implement specific usage of verify_pre_usermode_state for user-mode returns for arm64. Signed-off-by: Thomas Garnier <thgarnie@google.com> --- Based on next-20170404 --- arch/arm64/Kconfig | 1 + arch/arm64/kernel/entry.S | 15 +++++++++++++++ 2 files changed, 16 insertions(+)