Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1722547 > unrolled thread

Re: [PATCH v3 3/4] arm/syscalls: Optimize address limit check

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 3/4] arm/syscalls: Optimize address limit check Thomas Garnier <thgarnie@google.com> - 2017-08-29 16:40 +0200
    Re: [PATCH v3 3/4] arm/syscalls: Optimize address limit check Kees Cook <keescook@chromium.org> - 2017-08-29 22:00 +0200

#1722547 — Re: [PATCH v3 3/4] arm/syscalls: Optimize address limit check

On Tue, Aug 22, 2017 at 9:42 AM, Thomas Garnier <thgarnie@google.com> wrote:
> On Mon, Aug 14, 2017 at 2:37 PM, 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 report data corruption (stopping
>> the kernel or process based on configuration).
>>
>> The address limit check has to be done before any pending work because
>> they can reset the address limit and the process is killed using a
>> SIGKILL signal. 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>
>
> Any feedback?

CCing LW@karo-electronics.de who experienced the same issue this patch
proposal fix.

Russel: Any feedback?

>
>> ---
>>  arch/arm/kernel/entry-common.S | 11 +++++++++++
>>  arch/arm/kernel/signal.c       |  7 +++++++
>>  2 files changed, 18 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..b67ae12503f3 100644
>> --- a/arch/arm/kernel/signal.c
>> +++ b/arch/arm/kernel/signal.c
>> @@ -14,6 +14,7 @@
>>  #include <linux/uaccess.h>
>>  #include <linux/tracehook.h>
>>  #include <linux/uprobes.h>
>> +#include <linux/syscalls.h>
>>
>>  #include <asm/elf.h>
>>  #include <asm/cacheflush.h>
>> @@ -673,3 +674,9 @@ struct page *get_signal_page(void)
>>
>>         return page;
>>  }
>> +
>> +/* Defer to generic check */
>> +asmlinkage void addr_limit_check_failed(void)
>> +{
>> +       addr_limit_user_check();
>> +}
>> --
>> 2.14.1.480.gb18f417b89-goog
>>
>
>
>
> --
> Thomas



-- 
Thomas

[toc] | [next] | [standalone]

#1722795

On Tue, Aug 29, 2017 at 7:32 AM, Thomas Garnier <thgarnie@google.com> wrote:
> On Tue, Aug 22, 2017 at 9:42 AM, Thomas Garnier <thgarnie@google.com> wrote:
>> On Mon, Aug 14, 2017 at 2:37 PM, 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 report data corruption (stopping
>>> the kernel or process based on configuration).
>>>
>>> The address limit check has to be done before any pending work because
>>> they can reset the address limit and the process is killed using a
>>> SIGKILL signal. 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>
>>
>> Any feedback?
>
> CCing LW@karo-electronics.de who experienced the same issue this patch
> proposal fix.
>
> Russell: Any feedback?

These implement Russell's suggestion. An Ack here would be nice. :) I
can't throw these into the ARM patch tracker because they depend on
stuff in -next (and the commit that needs to be reverted is in tglx's
tree).

Regardless, these all test out correctly for me, so:

Reviewed-by: Kees Cook <keescook@chromium.org>
Tested-by: Kees Cook <keescook@chromium.org>

In a perfect world, these 4 patches should go together with the other
address limit check patches in tglx's tree. Thomas (Gleixner), can you
update your tree for the merge window? At the very least, we need to
revert 73ac5d6a2b6ac ("arm/syscalls: Check address limit on user-mode
return"), which has caused infinite loops in some cases. Better to
take all 4 patches in this series, though.

Thanks!

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web