[PATCH v2 1/3] Revert "arm/syscalls: Check address limit on user-mode return"

[PATCH v2 1/3] Revert "arm/syscalls: Check address limit on user-mode return"

[Thomas Garnier]

This reverts commit 73ac5d6a2b6ac3ae8d1e1818f3e9946f97489bc9.

The work pending loop can call set_fs after addr_limit_user_check
removed the _TIF_FSCHECK flag. This may happen at anytime based on how
ARM handles alignment exceptions. It leads to an infinite loop condition.

After discussion, it has been agreed that the generic approach is not
tailored to the ARM architecture and any fix might not be complete. This
patch will be replaced by an architecture specific implementation. The
work flag approach will be kept for other architectures.

Reported-by: Leonard Crestez <leonard.crestez@nxp.com>
Signed-off-by: Thomas Garnier <thgarnie@google.com>
---
 arch/arm/include/asm/thread_info.h | 15 ++++++---------
 arch/arm/include/asm/uaccess.h     |  2 --
 arch/arm/kernel/entry-common.S     |  9 ++-------
 arch/arm/kernel/signal.c           |  5 -----
 4 files changed, 8 insertions(+), 23 deletions(-)

diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index 1d468b527b7b..776757d1604a 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -139,11 +139,10 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
 #define TIF_NEED_RESCHED	1	/* rescheduling necessary */
 #define TIF_NOTIFY_RESUME	2	/* callback before returning to user */
 #define TIF_UPROBE		3	/* breakpointed or singlestepping */
-#define TIF_FSCHECK		4	/* Check FS is USER_DS on return */
-#define TIF_SYSCALL_TRACE	5	/* syscall trace active */
-#define TIF_SYSCALL_AUDIT	6	/* syscall auditing active */
-#define TIF_SYSCALL_TRACEPOINT	7	/* syscall tracepoint instrumentation */
-#define TIF_SECCOMP		8	/* seccomp syscall filtering active */
+#define TIF_SYSCALL_TRACE	4	/* syscall trace active */
+#define TIF_SYSCALL_AUDIT	5	/* syscall auditing active */
+#define TIF_SYSCALL_TRACEPOINT	6	/* syscall tracepoint instrumentation */
+#define TIF_SECCOMP		7	/* seccomp syscall filtering active */
 
 #define TIF_NOHZ		12	/* in adaptive nohz mode */
 #define TIF_USING_IWMMXT	17
@@ -154,7 +153,6 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
 #define _TIF_NEED_RESCHED	(1 << TIF_NEED_RESCHED)
 #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
 #define _TIF_UPROBE		(1 << TIF_UPROBE)
-#define _TIF_FSCHECK		(1 << TIF_FSCHECK)
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
 #define _TIF_SYSCALL_TRACEPOINT	(1 << TIF_SYSCALL_TRACEPOINT)
@@ -168,9 +166,8 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
 /*
  * Change these and you break ASM code in entry-common.S
  */
-#define _TIF_WORK_MASK		(_TIF_NEED_RESCHED | _TIF_SIGPENDING |	\
-				 _TIF_NOTIFY_RESUME | _TIF_UPROBE |	\
-				 _TIF_FSCHECK)
+#define _TIF_WORK_MASK		(_TIF_NEED_RESCHED | _TIF_SIGPENDING | \
+				 _TIF_NOTIFY_RESUME | _TIF_UPROBE)
 
 #endif /* __KERNEL__ */
 #endif /* __ASM_ARM_THREAD_INFO_H */
diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index 87936dd5d151..0bf2347495f1 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -70,8 +70,6 @@ static inline void set_fs(mm_segment_t fs)
 {
 	current_thread_info()->addr_limit = fs;
 	modify_domain(DOMAIN_KERNEL, fs ? DOMAIN_CLIENT : DOMAIN_MANAGER);
-	/* On user-mode return, check fs is correct */
-	set_thread_flag(TIF_FSCHECK);
 }
 
 #define segment_eq(a, b)	((a) == (b))
diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index ca3614dc6938..0b60adf4a5d9 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -49,9 +49,7 @@ ret_fast_syscall:
  UNWIND(.cantunwind	)
 	disable_irq_notrace			@ disable interrupts
 	ldr	r1, [tsk, #TI_FLAGS]		@ re-check for syscall tracing
-	tst	r1, #_TIF_SYSCALL_WORK
-	bne	fast_work_pending
-	tst	r1, #_TIF_WORK_MASK
+	tst	r1, #_TIF_SYSCALL_WORK | _TIF_WORK_MASK
 	bne	fast_work_pending
 
 	/* perform architecture specific actions before user return */
@@ -77,15 +75,12 @@ 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
-	tst	r1, #_TIF_SYSCALL_WORK
-	bne	fast_work_pending
-	tst	r1, #_TIF_WORK_MASK
+	tst	r1, #_TIF_SYSCALL_WORK | _TIF_WORK_MASK
 	beq	no_work_pending
  UNWIND(.fnend		)
 ENDPROC(ret_fast_syscall)
 
 	/* Slower path - fall through to work_pending */
-fast_work_pending:
 #endif
 
 	tst	r1, #_TIF_SYSCALL_WORK
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index e2de50bf8742..5814298ef0b7 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -14,7 +14,6 @@
 #include <linux/uaccess.h>
 #include <linux/tracehook.h>
 #include <linux/uprobes.h>
-#include <linux/syscalls.h>
 
 #include <asm/elf.h>
 #include <asm/cacheflush.h>
@@ -614,10 +613,6 @@ do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
 	 * Update the trace code with the current status.
 	 */
 	trace_hardirqs_off();
-
-	/* Check valid user FS if needed */
-	addr_limit_user_check();
-
 	do {
 		if (likely(thread_flags & _TIF_NEED_RESCHED)) {
 			schedule();
-- 
2.14.0.rc0.400.g1c36432dff-goog

[PATCH v2 2/3] arm/syscalls: Optimize address limit check

[Thomas Garnier]

Disable the generic address limit check in favor of an architecture
specific optimized implementation. The generic implementation using
pending work flags did not work well with ARM and alignment faults.

The address limit is checked on each syscall return path to user-mode
path as well as the irq user-mode return function. If the address limit
was changed, a function is called to stop the kernel with an explicit
message.

The address limit check has to be done before any pending work because
they can reset the address limit. For example the lkdtm address limit
check does not work because the signal to kill the process will reset
the user-mode address limit.

Signed-off-by: Thomas Garnier <thgarnie@google.com>
---
 arch/arm/kernel/entry-common.S | 11 +++++++++++
 arch/arm/kernel/signal.c       |  5 +++++
 2 files changed, 16 insertions(+)

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index 0b60adf4a5d9..99c908226065 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -12,6 +12,7 @@
 #include <asm/unistd.h>
 #include <asm/ftrace.h>
 #include <asm/unwind.h>
+#include <asm/memory.h>
 #ifdef CONFIG_AEABI
 #include <asm/unistd-oabi.h>
 #endif
@@ -48,10 +49,14 @@ ret_fast_syscall:
  UNWIND(.fnstart	)
  UNWIND(.cantunwind	)
 	disable_irq_notrace			@ disable interrupts
+	ldr	r2, [tsk, #TI_ADDR_LIMIT]
+	cmp	r2, #TASK_SIZE
+	blne	addr_limit_check_failed
 	ldr	r1, [tsk, #TI_FLAGS]		@ re-check for syscall tracing
 	tst	r1, #_TIF_SYSCALL_WORK | _TIF_WORK_MASK
 	bne	fast_work_pending
 
+
 	/* perform architecture specific actions before user return */
 	arch_ret_to_user r1, lr
 
@@ -74,6 +79,9 @@ ret_fast_syscall:
  UNWIND(.cantunwind	)
 	str	r0, [sp, #S_R0 + S_OFF]!	@ save returned r0
 	disable_irq_notrace			@ disable interrupts
+	ldr	r2, [tsk, #TI_ADDR_LIMIT]
+	cmp	r2, #TASK_SIZE
+	blne	addr_limit_check_failed
 	ldr	r1, [tsk, #TI_FLAGS]		@ re-check for syscall tracing
 	tst	r1, #_TIF_SYSCALL_WORK | _TIF_WORK_MASK
 	beq	no_work_pending
@@ -106,6 +114,9 @@ ENTRY(ret_to_user)
 ret_slow_syscall:
 	disable_irq_notrace			@ disable interrupts
 ENTRY(ret_to_user_from_irq)
+	ldr	r2, [tsk, #TI_ADDR_LIMIT]
+	cmp	r2, #TASK_SIZE
+	blne	addr_limit_check_failed
 	ldr	r1, [tsk, #TI_FLAGS]
 	tst	r1, #_TIF_WORK_MASK
 	bne	slow_work_pending
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index 5814298ef0b7..5769c15cff89 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -673,3 +673,8 @@ struct page *get_signal_page(void)
 
 	return page;
 }
+
+asmlinkage void addr_limit_check_failed(void)
+{
+	panic("Incorrect address limit while returning to user-mode.");
+}
-- 
2.14.0.rc0.400.g1c36432dff-goog

[PATCH v2 3/3] arm64/syscalls: Move address limit check in loop

[Thomas Garnier]

A bug was reported on ARM where set_fs might be called after it was
checked on the work pending function. ARM64 is not affected by this bug
but has a similar construct. In order to avoid any similar problems in
the future, the addr_limit_user_check function is moved at the beginning
of the loop.

Fixes: cf7de27ab351 ("arm64/syscalls: Check address limit on user-mode return")
Reported-by: Leonard Crestez <leonard.crestez@nxp.com>
Signed-off-by: Thomas Garnier <thgarnie@google.com>
---
 arch/arm64/kernel/signal.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index e3e3293d1123..8e2705983e1d 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -751,10 +751,10 @@ asmlinkage void do_notify_resume(struct pt_regs *regs,
 	 */
 	trace_hardirqs_off();
 
-	/* Check valid user FS if needed */
-	addr_limit_user_check();
-
 	do {
+		/* Check valid user FS if needed */
+		addr_limit_user_check();
+
 		if (thread_flags & _TIF_NEED_RESCHED) {
 			schedule();
 		} else {
-- 
2.14.0.rc0.400.g1c36432dff-goog

Re: [PATCH v2 2/3] arm/syscalls: Optimize address limit check

[Thomas Garnier]

On Wed, Jul 26, 2017 at 10:00 AM, Thomas Garnier <thgarnie@google.com> wrote:
> Disable the generic address limit check in favor of an architecture
> specific optimized implementation. The generic implementation using
> pending work flags did not work well with ARM and alignment faults.
>
> The address limit is checked on each syscall return path to user-mode
> path as well as the irq user-mode return function. If the address limit
> was changed, a function is called to stop the kernel with an explicit
> message.
>
> The address limit check has to be done before any pending work because
> they can reset the address limit. For example the lkdtm address limit
> check does not work because the signal to kill the process will reset
> the user-mode address limit.
>
> Signed-off-by: Thomas Garnier <thgarnie@google.com>
> ---

Russel: What do you think about this patch set?

>  arch/arm/kernel/entry-common.S | 11 +++++++++++
>  arch/arm/kernel/signal.c       |  5 +++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> index 0b60adf4a5d9..99c908226065 100644
> --- a/arch/arm/kernel/entry-common.S
> +++ b/arch/arm/kernel/entry-common.S
> @@ -12,6 +12,7 @@
>  #include <asm/unistd.h>
>  #include <asm/ftrace.h>
>  #include <asm/unwind.h>
> +#include <asm/memory.h>
>  #ifdef CONFIG_AEABI
>  #include <asm/unistd-oabi.h>
>  #endif
> @@ -48,10 +49,14 @@ ret_fast_syscall:
>   UNWIND(.fnstart       )
>   UNWIND(.cantunwind    )
>         disable_irq_notrace                     @ disable interrupts
> +       ldr     r2, [tsk, #TI_ADDR_LIMIT]
> +       cmp     r2, #TASK_SIZE
> +       blne    addr_limit_check_failed
>         ldr     r1, [tsk, #TI_FLAGS]            @ re-check for syscall tracing
>         tst     r1, #_TIF_SYSCALL_WORK | _TIF_WORK_MASK
>         bne     fast_work_pending
>
> +
>         /* perform architecture specific actions before user return */
>         arch_ret_to_user r1, lr
>
> @@ -74,6 +79,9 @@ ret_fast_syscall:
>   UNWIND(.cantunwind    )
>         str     r0, [sp, #S_R0 + S_OFF]!        @ save returned r0
>         disable_irq_notrace                     @ disable interrupts
> +       ldr     r2, [tsk, #TI_ADDR_LIMIT]
> +       cmp     r2, #TASK_SIZE
> +       blne    addr_limit_check_failed
>         ldr     r1, [tsk, #TI_FLAGS]            @ re-check for syscall tracing
>         tst     r1, #_TIF_SYSCALL_WORK | _TIF_WORK_MASK
>         beq     no_work_pending
> @@ -106,6 +114,9 @@ ENTRY(ret_to_user)
>  ret_slow_syscall:
>         disable_irq_notrace                     @ disable interrupts
>  ENTRY(ret_to_user_from_irq)
> +       ldr     r2, [tsk, #TI_ADDR_LIMIT]
> +       cmp     r2, #TASK_SIZE
> +       blne    addr_limit_check_failed
>         ldr     r1, [tsk, #TI_FLAGS]
>         tst     r1, #_TIF_WORK_MASK
>         bne     slow_work_pending
> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> index 5814298ef0b7..5769c15cff89 100644
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -673,3 +673,8 @@ struct page *get_signal_page(void)
>
>         return page;
>  }
> +
> +asmlinkage void addr_limit_check_failed(void)
> +{
> +       panic("Incorrect address limit while returning to user-mode.");
> +}
> --
> 2.14.0.rc0.400.g1c36432dff-goog
>



-- 
Thomas

Re: [PATCH v2 2/3] arm/syscalls: Optimize address limit check

[Kees Cook]

On Wed, Jul 26, 2017 at 10:00 AM, Thomas Garnier <thgarnie@google.com> wrote:
> Disable the generic address limit check in favor of an architecture
> specific optimized implementation. The generic implementation using
> pending work flags did not work well with ARM and alignment faults.
>
> The address limit is checked on each syscall return path to user-mode
> path as well as the irq user-mode return function. If the address limit
> was changed, a function is called to stop the kernel with an explicit
> message.
>
> The address limit check has to be done before any pending work because
> they can reset the address limit. For example the lkdtm address limit
> check does not work because the signal to kill the process will reset
> the user-mode address limit.
>
> Signed-off-by: Thomas Garnier <thgarnie@google.com>
> ---
>  arch/arm/kernel/entry-common.S | 11 +++++++++++
>  arch/arm/kernel/signal.c       |  5 +++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> index 0b60adf4a5d9..99c908226065 100644
> --- a/arch/arm/kernel/entry-common.S
> +++ b/arch/arm/kernel/entry-common.S
> @@ -12,6 +12,7 @@
>  #include <asm/unistd.h>
>  #include <asm/ftrace.h>
>  #include <asm/unwind.h>
> +#include <asm/memory.h>
>  #ifdef CONFIG_AEABI
>  #include <asm/unistd-oabi.h>
>  #endif
> @@ -48,10 +49,14 @@ ret_fast_syscall:
>   UNWIND(.fnstart       )
>   UNWIND(.cantunwind    )
>         disable_irq_notrace                     @ disable interrupts
> +       ldr     r2, [tsk, #TI_ADDR_LIMIT]
> +       cmp     r2, #TASK_SIZE
> +       blne    addr_limit_check_failed
>         ldr     r1, [tsk, #TI_FLAGS]            @ re-check for syscall tracing
>         tst     r1, #_TIF_SYSCALL_WORK | _TIF_WORK_MASK
>         bne     fast_work_pending
>
> +
>         /* perform architecture specific actions before user return */
>         arch_ret_to_user r1, lr
>
> @@ -74,6 +79,9 @@ ret_fast_syscall:
>   UNWIND(.cantunwind    )
>         str     r0, [sp, #S_R0 + S_OFF]!        @ save returned r0
>         disable_irq_notrace                     @ disable interrupts
> +       ldr     r2, [tsk, #TI_ADDR_LIMIT]
> +       cmp     r2, #TASK_SIZE
> +       blne    addr_limit_check_failed
>         ldr     r1, [tsk, #TI_FLAGS]            @ re-check for syscall tracing
>         tst     r1, #_TIF_SYSCALL_WORK | _TIF_WORK_MASK
>         beq     no_work_pending
> @@ -106,6 +114,9 @@ ENTRY(ret_to_user)
>  ret_slow_syscall:
>         disable_irq_notrace                     @ disable interrupts
>  ENTRY(ret_to_user_from_irq)
> +       ldr     r2, [tsk, #TI_ADDR_LIMIT]
> +       cmp     r2, #TASK_SIZE
> +       blne    addr_limit_check_failed
>         ldr     r1, [tsk, #TI_FLAGS]
>         tst     r1, #_TIF_WORK_MASK
>         bne     slow_work_pending
> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> index 5814298ef0b7..5769c15cff89 100644
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -673,3 +673,8 @@ struct page *get_signal_page(void)
>
>         return page;
>  }
> +
> +asmlinkage void addr_limit_check_failed(void)
> +{
> +       panic("Incorrect address limit while returning to user-mode.");
> +}

Instead of taking the entire system down, how about a WARN/kill combo
instead? If it's too late for "force_sig(SIGKILL, current)", then
likely we should perform a "do_group_exit(SIGKILL)".

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2 2/3] arm/syscalls: Optimize address limit check

[Thomas Garnier]

On Mon, Aug 7, 2017 at 10:35 AM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Jul 26, 2017 at 10:00 AM, Thomas Garnier <thgarnie@google.com> wrote:
>> Disable the generic address limit check in favor of an architecture
>> specific optimized implementation. The generic implementation using
>> pending work flags did not work well with ARM and alignment faults.
>>
>> The address limit is checked on each syscall return path to user-mode
>> path as well as the irq user-mode return function. If the address limit
>> was changed, a function is called to stop the kernel with an explicit
>> message.
>>
>> The address limit check has to be done before any pending work because
>> they can reset the address limit. For example the lkdtm address limit
>> check does not work because the signal to kill the process will reset
>> the user-mode address limit.
>>
>> Signed-off-by: Thomas Garnier <thgarnie@google.com>
>> ---
>>  arch/arm/kernel/entry-common.S | 11 +++++++++++
>>  arch/arm/kernel/signal.c       |  5 +++++
>>  2 files changed, 16 insertions(+)
>>
>> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
>> index 0b60adf4a5d9..99c908226065 100644
>> --- a/arch/arm/kernel/entry-common.S
>> +++ b/arch/arm/kernel/entry-common.S
>> @@ -12,6 +12,7 @@
>>  #include <asm/unistd.h>
>>  #include <asm/ftrace.h>
>>  #include <asm/unwind.h>
>> +#include <asm/memory.h>
>>  #ifdef CONFIG_AEABI
>>  #include <asm/unistd-oabi.h>
>>  #endif
>> @@ -48,10 +49,14 @@ ret_fast_syscall:
>>   UNWIND(.fnstart       )
>>   UNWIND(.cantunwind    )
>>         disable_irq_notrace                     @ disable interrupts
>> +       ldr     r2, [tsk, #TI_ADDR_LIMIT]
>> +       cmp     r2, #TASK_SIZE
>> +       blne    addr_limit_check_failed
>>         ldr     r1, [tsk, #TI_FLAGS]            @ re-check for syscall tracing
>>         tst     r1, #_TIF_SYSCALL_WORK | _TIF_WORK_MASK
>>         bne     fast_work_pending
>>
>> +
>>         /* perform architecture specific actions before user return */
>>         arch_ret_to_user r1, lr
>>
>> @@ -74,6 +79,9 @@ ret_fast_syscall:
>>   UNWIND(.cantunwind    )
>>         str     r0, [sp, #S_R0 + S_OFF]!        @ save returned r0
>>         disable_irq_notrace                     @ disable interrupts
>> +       ldr     r2, [tsk, #TI_ADDR_LIMIT]
>> +       cmp     r2, #TASK_SIZE
>> +       blne    addr_limit_check_failed
>>         ldr     r1, [tsk, #TI_FLAGS]            @ re-check for syscall tracing
>>         tst     r1, #_TIF_SYSCALL_WORK | _TIF_WORK_MASK
>>         beq     no_work_pending
>> @@ -106,6 +114,9 @@ ENTRY(ret_to_user)
>>  ret_slow_syscall:
>>         disable_irq_notrace                     @ disable interrupts
>>  ENTRY(ret_to_user_from_irq)
>> +       ldr     r2, [tsk, #TI_ADDR_LIMIT]
>> +       cmp     r2, #TASK_SIZE
>> +       blne    addr_limit_check_failed
>>         ldr     r1, [tsk, #TI_FLAGS]
>>         tst     r1, #_TIF_WORK_MASK
>>         bne     slow_work_pending
>> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
>> index 5814298ef0b7..5769c15cff89 100644
>> --- a/arch/arm/kernel/signal.c
>> +++ b/arch/arm/kernel/signal.c
>> @@ -673,3 +673,8 @@ struct page *get_signal_page(void)
>>
>>         return page;
>>  }
>> +
>> +asmlinkage void addr_limit_check_failed(void)
>> +{
>> +       panic("Incorrect address limit while returning to user-mode.");
>> +}
>
> Instead of taking the entire system down, how about a WARN/kill combo
> instead? If it's too late for "force_sig(SIGKILL, current)", then
> likely we should perform a "do_group_exit(SIGKILL)".

Sure, why not. I can also change the others architectures to move to a
do_group_exit(SIGKILL).

Before the next iteration, I want to know if Russel has any feedback
on this implementation, given the previous thread.

>
> -Kees
>
> --
> Kees Cook
> Pixel Security



-- 
Thomas

Re: [PATCH v2 2/3] arm/syscalls: Optimize address limit check

[Russell King - ARM Linux]

On Mon, Aug 07, 2017 at 10:42:14AM -0700, Thomas Garnier wrote:
> On Mon, Aug 7, 2017 at 10:35 AM, Kees Cook <keescook@chromium.org> wrote:
> > On Wed, Jul 26, 2017 at 10:00 AM, Thomas Garnier <thgarnie@google.com> wrote:
> >> Disable the generic address limit check in favor of an architecture
> >> specific optimized implementation. The generic implementation using
> >> pending work flags did not work well with ARM and alignment faults.
> >>
> >> The address limit is checked on each syscall return path to user-mode
> >> path as well as the irq user-mode return function. If the address limit
> >> was changed, a function is called to stop the kernel with an explicit
> >> message.
> >>
> >> The address limit check has to be done before any pending work because
> >> they can reset the address limit. For example the lkdtm address limit
> >> check does not work because the signal to kill the process will reset
> >> the user-mode address limit.
> >>
> >> Signed-off-by: Thomas Garnier <thgarnie@google.com>
> >> ---
> >>  arch/arm/kernel/entry-common.S | 11 +++++++++++
> >>  arch/arm/kernel/signal.c       |  5 +++++
> >>  2 files changed, 16 insertions(+)
> >>
> >> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> >> index 0b60adf4a5d9..99c908226065 100644
> >> --- a/arch/arm/kernel/entry-common.S
> >> +++ b/arch/arm/kernel/entry-common.S
> >> @@ -12,6 +12,7 @@
> >>  #include <asm/unistd.h>
> >>  #include <asm/ftrace.h>
> >>  #include <asm/unwind.h>
> >> +#include <asm/memory.h>
> >>  #ifdef CONFIG_AEABI
> >>  #include <asm/unistd-oabi.h>
> >>  #endif
> >> @@ -48,10 +49,14 @@ ret_fast_syscall:
> >>   UNWIND(.fnstart       )
> >>   UNWIND(.cantunwind    )
> >>         disable_irq_notrace                     @ disable interrupts
> >> +       ldr     r2, [tsk, #TI_ADDR_LIMIT]
> >> +       cmp     r2, #TASK_SIZE
> >> +       blne    addr_limit_check_failed
> >>         ldr     r1, [tsk, #TI_FLAGS]            @ re-check for syscall tracing
> >>         tst     r1, #_TIF_SYSCALL_WORK | _TIF_WORK_MASK
> >>         bne     fast_work_pending
> >>
> >> +
> >>         /* perform architecture specific actions before user return */
> >>         arch_ret_to_user r1, lr
> >>
> >> @@ -74,6 +79,9 @@ ret_fast_syscall:
> >>   UNWIND(.cantunwind    )
> >>         str     r0, [sp, #S_R0 + S_OFF]!        @ save returned r0
> >>         disable_irq_notrace                     @ disable interrupts
> >> +       ldr     r2, [tsk, #TI_ADDR_LIMIT]
> >> +       cmp     r2, #TASK_SIZE
> >> +       blne    addr_limit_check_failed
> >>         ldr     r1, [tsk, #TI_FLAGS]            @ re-check for syscall tracing
> >>         tst     r1, #_TIF_SYSCALL_WORK | _TIF_WORK_MASK
> >>         beq     no_work_pending
> >> @@ -106,6 +114,9 @@ ENTRY(ret_to_user)
> >>  ret_slow_syscall:
> >>         disable_irq_notrace                     @ disable interrupts
> >>  ENTRY(ret_to_user_from_irq)
> >> +       ldr     r2, [tsk, #TI_ADDR_LIMIT]
> >> +       cmp     r2, #TASK_SIZE
> >> +       blne    addr_limit_check_failed
> >>         ldr     r1, [tsk, #TI_FLAGS]
> >>         tst     r1, #_TIF_WORK_MASK
> >>         bne     slow_work_pending
> >
> > Instead of taking the entire system down, how about a WARN/kill combo
> > instead? If it's too late for "force_sig(SIGKILL, current)", then
> > likely we should perform a "do_group_exit(SIGKILL)".
> 
> Sure, why not. I can also change the others architectures to move to a
> do_group_exit(SIGKILL).
> 
> Before the next iteration, I want to know if Russel has any feedback
> on this implementation, given the previous thread.

It's better in so far as it avoids the problems previously highlighted.

However, it depends how efficient we want these paths to be - the
difference between your assembly and the assembly I've previously
supplied is that mine fills in any delay slots with some useful work
and avoids adding extra delay slots in this path.

Arguably, the system call exit path is as important as the system
call entry path for OS performance, so I think we should strive to
make it as efficient as possible - much as I already did when I
posted code on this topic previously.

I think that code can simply be adapted to call your C function
instead of the assembly "addr_limit_fail" label.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH v2 2/3] arm/syscalls: Optimize address limit check

[Thomas Garnier]

On Mon, Aug 7, 2017 at 10:55 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
>
> It's better in so far as it avoids the problems previously highlighted.
>
> However, it depends how efficient we want these paths to be - the
> difference between your assembly and the assembly I've previously
> supplied is that mine fills in any delay slots with some useful work
> and avoids adding extra delay slots in this path.

The previous assembly implementation we did was design as you
described but all checks were done after the pending work was managed.
I would like the address limit check to be done before, especially if
we move from panic to a SIGKILL approach.

>
> Arguably, the system call exit path is as important as the system
> call entry path for OS performance, so I think we should strive to
> make it as efficient as possible - much as I already did when I
> posted code on this topic previously.

How do you think it could improve while keeping the check before pending work?

>
> I think that code can simply be adapted to call your C function
> instead of the assembly "addr_limit_fail" label.

I don't use the label anymore on this version.

>
> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
> According to speedtest.net: 8.21Mbps down 510kbps up



-- 
Thomas