[PATCH v2 1/4] syscalls: Restore address limit after a syscall

[PATCH v2 1/4] syscalls: Restore address limit after a syscall

[Borislav Petkov]

On Wed, Mar 08, 2017 at 05:24:53PM -0800, Thomas Garnier wrote:
> @@ -191,6 +191,22 @@ extern struct trace_event_functions exit_syscall_print_funcs;
>  	SYSCALL_METADATA(sname, x, __VA_ARGS__)			\
>  	__SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
>  
> +asmlinkage void verify_pre_usermode_state(void);
> +
> +#ifndef CONFIG_ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE
> +static inline bool has_user_ds(void) {
> +	bool ret = segment_eq(get_fs(), USER_DS);
> +	// Prevent re-ordering the call

This is not the kernel comments style. Use /* */ instead.

> +	barrier();
> +	return ret;
> +}
> +#else
> +static inline bool has_user_ds(void) {
> +	return false;
> +}
> +#endif

... and then you could slim down the ifdeffery a bit:

static inline bool has_user_ds(void) {
	bool ret = false;

#ifndef CONFIG_ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE
	ret = segment_eq(get_fs(), USER_DS);
	/* Prevent re-ordering the call. */
	barrier();
#endif

	return ret;
}

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCH v2 1/4] syscalls: Restore address limit after a syscall

[Mark Rutland]

Hi,

On Wed, Mar 08, 2017 at 05:24:53PM -0800, Thomas Garnier wrote:
> This patch ensures a syscall does not return to user-mode with a kernel
> address limit. If that happened, a process can corrupt kernel-mode
> memory and elevate privileges.
> 
> For example, it would mitigation this bug:
> 
> - https://bugs.chromium.org/p/project-zero/issues/detail?id=990
> 
> If the CONFIG_BUG_ON_DATA_CORRUPTION option is enabled, an incorrect
> state will result in a BUG_ON.
> 
> The CONFIG_ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE option is also
> added so each architecture can optimize this change.

> +#ifndef CONFIG_ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE
> +static inline bool has_user_ds(void) {
> +	bool ret = segment_eq(get_fs(), USER_DS);
> +	// Prevent re-ordering the call
> +	barrier();

What ordering are we trying to ensure, that isn't otherwise given?

We expect get_fs() and set_fs() to be ordered w.r.t. each other and
w.r.t. uaccess uses, or we'd need barriers all over the place.

Given that, I can't see why we need a barrier here. So this needs a
better comment, at least.

> +	return ret;
> +}
> +#else
> +static inline bool has_user_ds(void) {
> +	return false;
> +}
> +#endif

It would be simpler to wrap the call entirely, e.g. have:

#ifdef CONFIG_WHATEVER
static inline void verify_pre_usermode_state(void)
{
	if (segment_eq(get_fs(), USER_DS))
		__verify_pre_usermode_state();
}
#else
static inline void verify_pre_usermode_state(void) { }
#endif

> @@ -199,7 +215,10 @@ extern struct trace_event_functions exit_syscall_print_funcs;
>  	asmlinkage long SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__));	\
>  	asmlinkage long SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__))	\
>  	{								\
> +		bool user_caller = has_user_ds();			\
>  		long ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__));	\
> +		if (user_caller)					\
> +			verify_pre_usermode_state();			\

... then we can unconditionally use verify_pre_usermode_state() here ... 

>  		__MAP(x,__SC_TEST,__VA_ARGS__);				\
>  		__PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__));	\
>  		return ret;						\

[...]

> +/* Called before coming back to user-mode */
> +asmlinkage void verify_pre_usermode_state(void)

... and we just prepend a couple of underscores here.

> +{
> +	if (CHECK_DATA_CORRUPTION(!segment_eq(get_fs(), USER_DS),
> +				  "incorrect get_fs() on user-mode return"))
> +		set_fs(USER_DS);
> +}

Thanks,
Mark.

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

[Mark Rutland]

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
> 

[PATCH v2 1/4] syscalls: Restore address limit after a syscall

[Russell King - ARM Linux]

On Thu, Mar 09, 2017 at 12:09:55PM +0000, Mark Rutland wrote:
> Hi,
> 
> On Wed, Mar 08, 2017 at 05:24:53PM -0800, Thomas Garnier wrote:
> > This patch ensures a syscall does not return to user-mode with a kernel
> > address limit. If that happened, a process can corrupt kernel-mode
> > memory and elevate privileges.
> > 
> > For example, it would mitigation this bug:
> > 
> > - https://bugs.chromium.org/p/project-zero/issues/detail?id=990
> > 
> > If the CONFIG_BUG_ON_DATA_CORRUPTION option is enabled, an incorrect
> > state will result in a BUG_ON.
> > 
> > The CONFIG_ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE option is also
> > added so each architecture can optimize this change.
> 
> > +#ifndef CONFIG_ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE
> > +static inline bool has_user_ds(void) {
> > +	bool ret = segment_eq(get_fs(), USER_DS);
> > +	// Prevent re-ordering the call
> > +	barrier();
> 
> What ordering are we trying to ensure, that isn't otherwise given?
> 
> We expect get_fs() and set_fs() to be ordered w.r.t. each other and
> w.r.t. uaccess uses, or we'd need barriers all over the place.
> 
> Given that, I can't see why we need a barrier here. So this needs a
> better comment, at least.
> 
> > +	return ret;
> > +}
> > +#else
> > +static inline bool has_user_ds(void) {
> > +	return false;
> > +}
> > +#endif
> 
> It would be simpler to wrap the call entirely, e.g. have:
> 
> #ifdef CONFIG_WHATEVER
> static inline void verify_pre_usermode_state(void)
> {
> 	if (segment_eq(get_fs(), USER_DS))
> 		__verify_pre_usermode_state();
> }
> #else
> static inline void verify_pre_usermode_state(void) { }
> #endif

That's utterly pointless - you've missed a detail.

> > @@ -199,7 +215,10 @@ extern struct trace_event_functions exit_syscall_print_funcs;
> >  	asmlinkage long SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__));	\
> >  	asmlinkage long SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__))	\
> >  	{								\
> > +		bool user_caller = has_user_ds();			\
> >  		long ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__));	\
> > +		if (user_caller)					\
> > +			verify_pre_usermode_state();			\
> 
> ... then we can unconditionally use verify_pre_usermode_state() here ... 

Look at this closely.  has_user_ds() is called _before_ the syscall code
is invoked.  It's checking what conditions the syscall was entered from.
If the syscall was entered with the user segment selected, then we run
a check on the system state _after_ the syscall code has returned.

Putting both after the syscall code has returned is completely pointless -
it turns it into this code:

	if (segment_eq(get_fs(), USER_DS))
		if (CHECK_DATA_CORRUPTION(!segment_eq(get_fs(), USER_DS),
					  "incorrect get_fs() on user-mode return"))
			set_fs(USER_DS);

which is obviously bogus (it'll never fire.)

-- 
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 v2 1/4] syscalls: Restore address limit after a syscall

[Mark Rutland]

On Thu, Mar 09, 2017 at 01:44:56PM +0000, Russell King - ARM Linux wrote:
> On Thu, Mar 09, 2017 at 12:09:55PM +0000, Mark Rutland wrote:
> > On Wed, Mar 08, 2017 at 05:24:53PM -0800, Thomas Garnier wrote:

> > It would be simpler to wrap the call entirely, e.g. have:
> > 
> > #ifdef CONFIG_WHATEVER
> > static inline void verify_pre_usermode_state(void)
> > {
> > 	if (segment_eq(get_fs(), USER_DS))
> > 		__verify_pre_usermode_state();
> > }
> > #else
> > static inline void verify_pre_usermode_state(void) { }
> > #endif
> 
> That's utterly pointless - you've missed a detail.
> 
> > > @@ -199,7 +215,10 @@ extern struct trace_event_functions exit_syscall_print_funcs;
> > >  	asmlinkage long SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__));	\
> > >  	asmlinkage long SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__))	\
> > >  	{								\
> > > +		bool user_caller = has_user_ds();			\
> > >  		long ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__));	\
> > > +		if (user_caller)					\
> > > +			verify_pre_usermode_state();			\
> > 
> > ... then we can unconditionally use verify_pre_usermode_state() here ... 
> 
> Look at this closely.  has_user_ds() is called _before_ the syscall code
> is invoked.  It's checking what conditions the syscall was entered from.
> If the syscall was entered with the user segment selected, then we run
> a check on the system state _after_ the syscall code has returned.

Indeed; I clearly did not consider this correctly. 

Sorry for the noise.

Thanks,
Mark.

[PATCH v2 1/4] syscalls: Restore address limit after a syscall

[Thomas Garnier]

On Thu, Mar 9, 2017 at 12:42 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Mar 08, 2017 at 05:24:53PM -0800, Thomas Garnier wrote:
>> @@ -191,6 +191,22 @@ extern struct trace_event_functions exit_syscall_print_funcs;
>>       SYSCALL_METADATA(sname, x, __VA_ARGS__)                 \
>>       __SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
>>
>> +asmlinkage void verify_pre_usermode_state(void);
>> +
>> +#ifndef CONFIG_ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE
>> +static inline bool has_user_ds(void) {
>> +     bool ret = segment_eq(get_fs(), USER_DS);
>> +     // Prevent re-ordering the call
>
> This is not the kernel comments style. Use /* */ instead.
>
>> +     barrier();
>> +     return ret;
>> +}
>> +#else
>> +static inline bool has_user_ds(void) {
>> +     return false;
>> +}
>> +#endif
>
> ... and then you could slim down the ifdeffery a bit:
>
> static inline bool has_user_ds(void) {
>         bool ret = false;
>
> #ifndef CONFIG_ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE
>         ret = segment_eq(get_fs(), USER_DS);
>         /* Prevent re-ordering the call. */
>         barrier();
> #endif
>
>         return ret;
> }
>

I agree, cleaner. I will look to do this change on next iteration.

> --
> Regards/Gruss,
>     Boris.
>
> Good mailing practices for 400: avoid top-posting and trim the reply.



-- 
Thomas

[PATCH v2 1/4] syscalls: Restore address limit after a syscall

[Thomas Garnier]

On Thu, Mar 9, 2017 at 4:09 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> On Wed, Mar 08, 2017 at 05:24:53PM -0800, Thomas Garnier wrote:
>> This patch ensures a syscall does not return to user-mode with a kernel
>> address limit. If that happened, a process can corrupt kernel-mode
>> memory and elevate privileges.
>>
>> For example, it would mitigation this bug:
>>
>> - https://bugs.chromium.org/p/project-zero/issues/detail?id=990
>>
>> If the CONFIG_BUG_ON_DATA_CORRUPTION option is enabled, an incorrect
>> state will result in a BUG_ON.
>>
>> The CONFIG_ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE option is also
>> added so each architecture can optimize this change.
>
>> +#ifndef CONFIG_ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE
>> +static inline bool has_user_ds(void) {
>> +     bool ret = segment_eq(get_fs(), USER_DS);
>> +     // Prevent re-ordering the call
>> +     barrier();
>
> What ordering are we trying to ensure, that isn't otherwise given?
>
> We expect get_fs() and set_fs() to be ordered w.r.t. each other and
> w.r.t. uaccess uses, or we'd need barriers all over the place.
>
> Given that, I can't see why we need a barrier here. So this needs a
> better comment, at least.
>

I was half sure of that so that's why I added the barrier. If it is
not needed then I can remove it. Thanks!

>> +     return ret;
>> +}
>> +#else
>> +static inline bool has_user_ds(void) {
>> +     return false;
>> +}
>> +#endif
>
> It would be simpler to wrap the call entirely, e.g. have:
>
> #ifdef CONFIG_WHATEVER
> static inline void verify_pre_usermode_state(void)
> {
>         if (segment_eq(get_fs(), USER_DS))
>                 __verify_pre_usermode_state();
> }
> #else
> static inline void verify_pre_usermode_state(void) { }
> #endif
>
>> @@ -199,7 +215,10 @@ extern struct trace_event_functions exit_syscall_print_funcs;
>>       asmlinkage long SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__));      \
>>       asmlinkage long SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__))       \
>>       {                                                               \
>> +             bool user_caller = has_user_ds();                       \
>>               long ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__));  \
>> +             if (user_caller)                                        \
>> +                     verify_pre_usermode_state();                    \
>
> ... then we can unconditionally use verify_pre_usermode_state() here ...

Not sure I understood that point. The goal is to see if get_fs was
changed, that's why I check before the syscall and I want to ensure
the call is not shuffled after the syscall, therefore the original
barrier.

>
>>               __MAP(x,__SC_TEST,__VA_ARGS__);                         \
>>               __PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__));       \
>>               return ret;                                             \
>
> [...]
>
>> +/* Called before coming back to user-mode */
>> +asmlinkage void verify_pre_usermode_state(void)
>
> ... and we just prepend a couple of underscores here.
>
>> +{
>> +     if (CHECK_DATA_CORRUPTION(!segment_eq(get_fs(), USER_DS),
>> +                               "incorrect get_fs() on user-mode return"))
>> +             set_fs(USER_DS);
>> +}
>
> Thanks,
> Mark.



-- 
Thomas

[PATCH v2 1/4] syscalls: Restore address limit after a syscall

[Thomas Garnier]

On Thu, Mar 9, 2017 at 7:21 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Mar 09, 2017 at 01:44:56PM +0000, Russell King - ARM Linux wrote:
>> On Thu, Mar 09, 2017 at 12:09:55PM +0000, Mark Rutland wrote:
>> > On Wed, Mar 08, 2017 at 05:24:53PM -0800, Thomas Garnier wrote:
>
>> > It would be simpler to wrap the call entirely, e.g. have:
>> >
>> > #ifdef CONFIG_WHATEVER
>> > static inline void verify_pre_usermode_state(void)
>> > {
>> >     if (segment_eq(get_fs(), USER_DS))
>> >             __verify_pre_usermode_state();
>> > }
>> > #else
>> > static inline void verify_pre_usermode_state(void) { }
>> > #endif
>>
>> That's utterly pointless - you've missed a detail.
>>
>> > > @@ -199,7 +215,10 @@ extern struct trace_event_functions exit_syscall_print_funcs;
>> > >   asmlinkage long SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__));      \
>> > >   asmlinkage long SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__))       \
>> > >   {                                                               \
>> > > +         bool user_caller = has_user_ds();                       \
>> > >           long ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__));  \
>> > > +         if (user_caller)                                        \
>> > > +                 verify_pre_usermode_state();                    \
>> >
>> > ... then we can unconditionally use verify_pre_usermode_state() here ...
>>
>> Look at this closely.  has_user_ds() is called _before_ the syscall code
>> is invoked.  It's checking what conditions the syscall was entered from.
>> If the syscall was entered with the user segment selected, then we run
>> a check on the system state _after_ the syscall code has returned.
>
> Indeed; I clearly did not consider this correctly.
>
> Sorry for the noise.
>

No problem, I missed that reply so discard my question on the email
few seconds ago.

> Thanks,
> Mark.



-- 
Thomas

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

[Thomas Garnier]

On Thu, Mar 9, 2017 at 4:23 AM, Mark Rutland <mark.rutland@arm.com> 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!
-- 
Thomas

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

[Mark Rutland]

On Thu, Mar 09, 2017 at 07:56:49AM -0800, Thomas Garnier wrote:
> On Thu, Mar 9, 2017 at 4:23 AM, Mark Rutland <mark.rutland@arm.com> 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.

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

[Thomas Garnier]

On Thu, Mar 9, 2017 at 8:05 AM, Mark Rutland <mark.rutland@arm.com> 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 <mark.rutland@arm.com> 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.



-- 
Thomas

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

[Russell King - ARM Linux]

On Thu, Mar 09, 2017 at 07:56:49AM -0800, Thomas Garnier wrote:
> On Thu, Mar 9, 2017 at 4:23 AM, Mark Rutland <mark.rutland@arm.com> 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.

-- 
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 v2 4/4] arm64/syscalls: Specific usage of verify_pre_usermode_state

[Thomas Garnier]

On Thu, Mar 9, 2017 at 8:26 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> 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 <mark.rutland@arm.com> 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.



-- 
Thomas

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

[Russell King - ARM Linux]

On Thu, Mar 09, 2017 at 08:35:30AM -0800, Thomas Garnier wrote:
> On Thu, Mar 9, 2017 at 8:26 AM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > 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?

It's still having to needlessly save stuff when there's nothing wrong.
Remember, syscalls are a fast path, so the minimum we can do is good.
Calling into C functions is not ideal, because they will tend to be
_very_ expensive compared to hand crafted assembly, especially for
something like this.

It's possible to check the address limit in just three instructions,
which is way less than will be incurred by calling a C function.

Note: This patch is completely untested.

 arch/arm/kernel/entry-common.S | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index eb5cd77bf1d8..6b43c6d73117 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -41,8 +41,11 @@
  UNWIND(.cantunwind	)
 	disable_irq_notrace			@ disable interrupts
 	ldr	r1, [tsk, #TI_FLAGS]		@ re-check for syscall tracing
+	ldr	r2, [tsk, #TI_ADDR_LIMIT]
 	tst	r1, #_TIF_SYSCALL_WORK | _TIF_WORK_MASK
 	bne	fast_work_pending
+	cmp	r2, #TASK_SIZE
+	blgt	addr_limit_fail
 
 	/* perform architecture specific actions before user return */
 	arch_ret_to_user r1, lr
@@ -67,6 +70,7 @@ ENDPROC(ret_fast_syscall)
 	str	r0, [sp, #S_R0 + S_OFF]!	@ save returned r0
 	disable_irq_notrace			@ disable interrupts
 	ldr	r1, [tsk, #TI_FLAGS]		@ re-check for syscall tracing
+	ldr	r2, [tsk, #TI_ADDR_LIMIT]
 	tst	r1, #_TIF_SYSCALL_WORK | _TIF_WORK_MASK
 	beq	no_work_pending
  UNWIND(.fnend		)
@@ -82,6 +86,7 @@ ENDPROC(ret_fast_syscall)
 	mov	r2, why				@ 'syscall'
 	bl	do_work_pending
 	cmp	r0, #0
+	ldreq	r2, [tsk, #TI_ADDR_LIMIT]
 	beq	no_work_pending
 	movlt	scno, #(__NR_restart_syscall - __NR_SYSCALL_BASE)
 	ldmia	sp, {r0 - r6}			@ have to reload r0 - r6
@@ -99,9 +104,12 @@ ENTRY(ret_to_user)
 	disable_irq_notrace			@ disable interrupts
 ENTRY(ret_to_user_from_irq)
 	ldr	r1, [tsk, #TI_FLAGS]
+	ldr	r2, [tsk, #TI_ADDR_LIMIT]
 	tst	r1, #_TIF_WORK_MASK
 	bne	slow_work_pending
 no_work_pending:
+	cmp	r2, #TASK_SIZE
+	blgt	addr_limit_fail
 	asm_trace_hardirqs_on save = 0
 
 	/* perform architecture specific actions before user return */
@@ -125,6 +133,16 @@ ENTRY(ret_from_fork)
 	b	ret_slow_syscall
 ENDPROC(ret_from_fork)
 
+addr_limit_fail:
+#ifdef CONFIG_BUG_ON_DATA_CORRUPTION
+	stmfd	sp!, {r0, lr}
+	bl	verify_pre_usermode_state
+	ldmfd	sp!, {r0, lr}
+#endif
+	mov	r2, #TASK_SIZE
+	str	r2, [tsk, #TI_ADDR_LIMIT]
+	ret	lr
+
 /*=============================================================================
  * SWI handler
  *-----------------------------------------------------------------------------


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