diff mbox

[v6,4/4] arm64/syscalls: Specific usage of verify_pre_usermode_state

Message ID 20170404174727.35478-4-thgarnie@google.com
State New, archived
Headers show

Commit Message

Thomas Garnier April 4, 2017, 5:47 p.m. UTC
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(+)

Comments

Catalin Marinas April 5, 2017, 2:22 p.m. UTC | #1
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?
Thomas Garnier April 5, 2017, 2:36 p.m. UTC | #2
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
Catalin Marinas April 5, 2017, 5:49 p.m. UTC | #3
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).
Thomas Garnier April 5, 2017, 6:14 p.m. UTC | #4
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
Catalin Marinas April 7, 2017, 4:11 p.m. UTC | #5
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 mbox

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.
  */