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

Commit Message

Thomas Garnier March 9, 2017, 1:24 a.m. UTC

Implement specific usage of verify_pre_usermode_state for user-mode
returns for arm64.
---
Based on next-20170308
---
 arch/arm64/Kconfig        |  1 +
 arch/arm64/kernel/entry.S | 15 +++++++++++++++
 2 files changed, 16 insertions(+)

Comments

Mark Rutland March 9, 2017, 12:23 p.m. UTC | #1

On Wed, Mar 08, 2017 at 05:24:56PM -0800, Thomas Garnier wrote:
> Implement specific usage of verify_pre_usermode_state for user-mode
> returns for arm64.
> ---
> Based on next-20170308
> ---
>  arch/arm64/Kconfig        |  1 +
>  arch/arm64/kernel/entry.S | 15 +++++++++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 896eba61e5ed..da54774838d8 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..eca392ae63e9 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -737,6 +737,19 @@ ENTRY(cpu_switch_to)
>  	ret
>  ENDPROC(cpu_switch_to)
>  
> +#ifdef CONFIG_BUG_ON_DATA_CORRUPTION
> +.macro VERIFY_PRE_USERMODE_STATE
> +	bl	verify_pre_usermode_state
> +.endm
> +#else

We generally stick to lower case for the arm64 assembly macros. If we
need this, we should stick to the existing convention.

> +/* Similar to set_fs(USER_DS) in verify_pre_usermode_state without a warning. */
> +.macro VERIFY_PRE_USERMODE_STATE
> +	mov	x1, #TASK_SIZE_64
> +	str	x1, [tsk, #TSK_TI_ADDR_LIMIT]
> +.endm

We need arm64's set_fs() to configure UAO, too, so this is much weaker
than set_fs(), and will leave __{get,put}_user and
__copy_{to,from}_user() able to access kernel memory.

We don't currently have an asm helper to clear UAO, and unconditionally
poking that on exception return is liable to be somewhat expensive.

Also, given we're only trying to catch this in syscalls, I'm afraid I
don't see what we gain by doing this in the entry assembly.

Thanks,
Mark.

> +#endif
> +
> +
>  /*
>   * This is the fast syscall return path.  We do as little as possible here,
>   * and this includes saving x0 back into the kernel stack.
> @@ -744,6 +757,7 @@ ENDPROC(cpu_switch_to)
>  ret_fast_syscall:
>  	disable_irq				// disable interrupts
>  	str	x0, [sp, #S_X0]			// returned x0
> +	VERIFY_PRE_USERMODE_STATE
>  	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 +785,7 @@ work_pending:
>   */
>  ret_to_user:
>  	disable_irq				// disable interrupts
> +	VERIFY_PRE_USERMODE_STATE
>  	ldr	x1, [tsk, #TSK_TI_FLAGS]
>  	and	x2, x1, #_TIF_WORK_MASK
>  	cbnz	x2, work_pending
> -- 
> 2.12.0.246.ga2ecc84866-goog
>

Thomas Garnier March 9, 2017, 3:56 p.m. UTC | #2

On Thu, Mar 9, 2017 at 4:23 AM, Mark Rutland <[e-mail]> wrote:
> We generally stick to lower case for the arm64 assembly macros. If we
> need this, we should stick to the existing convention.
>
>> +/* Similar to set_fs(USER_DS) in verify_pre_usermode_state without a warning. */
>> +.macro VERIFY_PRE_USERMODE_STATE
>> +     mov     x1, #TASK_SIZE_64
>> +     str     x1, [tsk, #TSK_TI_ADDR_LIMIT]
>> +.endm
>
> We need arm64's set_fs() to configure UAO, too, so this is much weaker
> than set_fs(), and will leave __{get,put}_user and
> __copy_{to,from}_user() able to access kernel memory.
>
> We don't currently have an asm helper to clear UAO, and unconditionally
> poking that on exception return is liable to be somewhat expensive.
>
> Also, given we're only trying to catch this in syscalls, I'm afraid I
> don't see what we gain by doing this in the entry assembly.
>

I optimized all architectures from the arm (32-bit) discussion. I will
come back to a simple bl to the verify function. Thanks!

Mark Rutland March 9, 2017, 4:05 p.m. UTC | #3

On Thu, Mar 09, 2017 at 07:56:49AM -0800, Thomas Garnier wrote:
> On Thu, Mar 9, 2017 at 4:23 AM, Mark Rutland <[e-mail]> wrote:
> > We generally stick to lower case for the arm64 assembly macros. If we
> > need this, we should stick to the existing convention.
> >
> >> +/* Similar to set_fs(USER_DS) in verify_pre_usermode_state without a warning. */
> >> +.macro VERIFY_PRE_USERMODE_STATE
> >> +     mov     x1, #TASK_SIZE_64
> >> +     str     x1, [tsk, #TSK_TI_ADDR_LIMIT]
> >> +.endm
> >
> > We need arm64's set_fs() to configure UAO, too, so this is much weaker
> > than set_fs(), and will leave __{get,put}_user and
> > __copy_{to,from}_user() able to access kernel memory.
> >
> > We don't currently have an asm helper to clear UAO, and unconditionally
> > poking that on exception return is liable to be somewhat expensive.
> >
> > Also, given we're only trying to catch this in syscalls, I'm afraid I
> > don't see what we gain by doing this in the entry assembly.
> 
> I optimized all architectures from the arm (32-bit) discussion. I will
> come back to a simple bl to the verify function. Thanks!

What I was trying to ask was do we need to touch the assembly at all
here?

Are we trying to protect the non-syscall cases by doing this in
assembly? If so, it'd be worth calling out in the commit message.

If so, we could add the necessary helper to clear UAO.

If not, doing this in the entry assembly only saves the small overhead
of reading and comparing the addr_limit for in-kernel use of the
syscalls (e.g. in the compat wrappers), and we may as well rely on the
common !ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE implementation.

Thanks,
Mark.

Thomas Garnier March 9, 2017, 4:19 p.m. UTC | #4

On Thu, Mar 9, 2017 at 8:05 AM, Mark Rutland <[e-mail]> wrote:
> On Thu, Mar 09, 2017 at 07:56:49AM -0800, Thomas Garnier wrote:
>> On Thu, Mar 9, 2017 at 4:23 AM, Mark Rutland <[e-mail]> wrote:
>> > We generally stick to lower case for the arm64 assembly macros. If we
>> > need this, we should stick to the existing convention.
>> >
>> >> +/* Similar to set_fs(USER_DS) in verify_pre_usermode_state without a warning. */
>> >> +.macro VERIFY_PRE_USERMODE_STATE
>> >> +     mov     x1, #TASK_SIZE_64
>> >> +     str     x1, [tsk, #TSK_TI_ADDR_LIMIT]
>> >> +.endm
>> >
>> > We need arm64's set_fs() to configure UAO, too, so this is much weaker
>> > than set_fs(), and will leave __{get,put}_user and
>> > __copy_{to,from}_user() able to access kernel memory.
>> >
>> > We don't currently have an asm helper to clear UAO, and unconditionally
>> > poking that on exception return is liable to be somewhat expensive.
>> >
>> > Also, given we're only trying to catch this in syscalls, I'm afraid I
>> > don't see what we gain by doing this in the entry assembly.
>>
>> I optimized all architectures from the arm (32-bit) discussion. I will
>> come back to a simple bl to the verify function. Thanks!
>
> What I was trying to ask was do we need to touch the assembly at all
> here?

You don't but he generic solution add code to every single syscall.

> Are we trying to protect the non-syscall cases by doing this in
> assembly? If so, it'd be worth calling out in the commit message.

It is an added benefit but not required.

> If so, we could add the necessary helper to clear UAO.

I can look at set_fs and fix it on the next iteraiton.

> If not, doing this in the entry assembly only saves the small overhead
> of reading and comparing the addr_limit for in-kernel use of the
> syscalls (e.g. in the compat wrappers), and we may as well rely on the
> common !ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE implementation.

You also don't have the code added for each syscall and a call.

>
> Thanks,
> Mark.

Russell King (Oracle) March 9, 2017, 4:26 p.m. UTC | #5

On Thu, Mar 09, 2017 at 07:56:49AM -0800, Thomas Garnier wrote:
> On Thu, Mar 9, 2017 at 4:23 AM, Mark Rutland <[e-mail]> wrote:
> > We generally stick to lower case for the arm64 assembly macros. If we
> > need this, we should stick to the existing convention.
> >
> >> +/* Similar to set_fs(USER_DS) in verify_pre_usermode_state without a warning. */
> >> +.macro VERIFY_PRE_USERMODE_STATE
> >> +     mov     x1, #TASK_SIZE_64
> >> +     str     x1, [tsk, #TSK_TI_ADDR_LIMIT]
> >> +.endm
> >
> > We need arm64's set_fs() to configure UAO, too, so this is much weaker
> > than set_fs(), and will leave __{get,put}_user and
> > __copy_{to,from}_user() able to access kernel memory.
> >
> > We don't currently have an asm helper to clear UAO, and unconditionally
> > poking that on exception return is liable to be somewhat expensive.
> >
> > Also, given we're only trying to catch this in syscalls, I'm afraid I
> > don't see what we gain by doing this in the entry assembly.
> >
> 
> I optimized all architectures from the arm (32-bit) discussion. I will
> come back to a simple bl to the verify function. Thanks!

I wouldn't call what you've done on ARM an "optimisation", because my
comment about making the fast path worthless still stands.

Thomas Garnier March 9, 2017, 4:35 p.m. UTC | #6

On Thu, Mar 9, 2017 at 8:26 AM, Russell King - ARM Linux
<[e-mail]> wrote:
> On Thu, Mar 09, 2017 at 07:56:49AM -0800, Thomas Garnier wrote:
>> On Thu, Mar 9, 2017 at 4:23 AM, Mark Rutland <[e-mail]> wrote:
>> > We generally stick to lower case for the arm64 assembly macros. If we
>> > need this, we should stick to the existing convention.
>> >
>> >> +/* Similar to set_fs(USER_DS) in verify_pre_usermode_state without a warning. */
>> >> +.macro VERIFY_PRE_USERMODE_STATE
>> >> +     mov     x1, #TASK_SIZE_64
>> >> +     str     x1, [tsk, #TSK_TI_ADDR_LIMIT]
>> >> +.endm
>> >
>> > We need arm64's set_fs() to configure UAO, too, so this is much weaker
>> > than set_fs(), and will leave __{get,put}_user and
>> > __copy_{to,from}_user() able to access kernel memory.
>> >
>> > We don't currently have an asm helper to clear UAO, and unconditionally
>> > poking that on exception return is liable to be somewhat expensive.
>> >
>> > Also, given we're only trying to catch this in syscalls, I'm afraid I
>> > don't see what we gain by doing this in the entry assembly.
>> >
>>
>> I optimized all architectures from the arm (32-bit) discussion. I will
>> come back to a simple bl to the verify function. Thanks!
>
> I wouldn't call what you've done on ARM an "optimisation", because my
> comment about making the fast path worthless still stands.

Why does it still stands on the latest proposal?

>
> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.

Patch

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 896eba61e5ed..da54774838d8 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..eca392ae63e9 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -737,6 +737,19 @@  ENTRY(cpu_switch_to)
 	ret
 ENDPROC(cpu_switch_to)
 
+#ifdef CONFIG_BUG_ON_DATA_CORRUPTION
+.macro VERIFY_PRE_USERMODE_STATE
+	bl	verify_pre_usermode_state
+.endm
+#else
+/* Similar to set_fs(USER_DS) in verify_pre_usermode_state without a warning. */
+.macro VERIFY_PRE_USERMODE_STATE
+	mov	x1, #TASK_SIZE_64
+	str	x1, [tsk, #TSK_TI_ADDR_LIMIT]
+.endm
+#endif
+
+
 /*
  * This is the fast syscall return path.  We do as little as possible here,
  * and this includes saving x0 back into the kernel stack.
@@ -744,6 +757,7 @@  ENDPROC(cpu_switch_to)
 ret_fast_syscall:
 	disable_irq				// disable interrupts
 	str	x0, [sp, #S_X0]			// returned x0
+	VERIFY_PRE_USERMODE_STATE
 	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 +785,7 @@  work_pending:
  */
 ret_to_user:
 	disable_irq				// disable interrupts
+	VERIFY_PRE_USERMODE_STATE
 	ldr	x1, [tsk, #TSK_TI_FLAGS]
 	and	x2, x1, #_TIF_WORK_MASK
 	cbnz	x2, work_pending