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

Groups > linux.kernel > #1640071 > unrolled thread

Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

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: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Thomas Garnier <thgarnie@google.com> - 2017-05-12 01:20 +0200
    Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-12 01:50 +0200
      Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Martin Schwidefsky <schwidefsky@de.ibm.com> - 2017-05-12 07:30 +0200
        Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-12 07:40 +0200
          Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Martin Schwidefsky <schwidefsky@de.ibm.com> - 2017-05-12 08:00 +0200
            Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-12 21:10 +0200
              Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-12 21:40 +0200
                Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-05-12 22:30 +0200
                  Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Peter Zijlstra <peterz@infradead.org> - 2017-05-12 22:40 +0200
                    Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-05-12 22:50 +0200
                      Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-12 23:10 +0200
                        Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-12 23:10 +0200
                    Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-13 09:30 +0200
                  Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-12 23:10 +0200
                    Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Daniel Micay <danielmicay@gmail.com> - 2017-05-12 23:20 +0200
                    Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-12 23:20 +0200
                      Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Daniel Micay <danielmicay@gmail.com> - 2017-05-12 23:30 +0200
                      Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-12 23:50 +0200
                        Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Rik van Riel <riel@redhat.com> - 2017-05-12 23:50 +0200
                          Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-13 01:00 +0200
                        Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-13 00:00 +0200
            Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-12 21:10 +0200
              Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-05-12 21:20 +0200
          Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-12 09:00 +0200
      Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Andy Lutomirski <luto@kernel.org> - 2017-05-12 08:20 +0200
      Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-12 09:00 +0200
        Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Thomas Garnier <thgarnie@google.com> - 2017-05-12 19:10 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1640780

On Fri, May 12, 2017 at 2:41 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Fri, May 12, 2017 at 02:17:19PM -0700, Kees Cook wrote:
>
>> Two things are at risk from stack exhaustion: thread_info (mainly
>> addr_limit) when on the stack (fixed by THREAD_INFO_IN_TASK), and
>
> Really?  Let's take a look at arm, for example:
>
> struct thread_info {
>         unsigned long           flags;          /* low level flags */
>         int                     preempt_count;  /* 0 => preemptable, <0 => bug */
>         mm_segment_t            addr_limit;     /* address limit */
>         struct task_struct      *task;          /* main task structure */
>
> and current() is defined as current_thread_info()->task.
>
> Seriously, look at these beasts.  Overwriting ->addr_limit is nowhere near
> the top threat.  If attacker can overwrite thread_info, you have lost.

I don't disagree, but the type of attack is different. If the attacker
overwrites task_struct pointer, then they need to have built an false
one, and that may be made difficult by PAN, or need to know more about
kernel memory layout (rather than only stack depth), etc. Attacking
addr_limit makes it very very easy to upgrade attack capabilities. I'm
not say thread_info shouldn't be moved off the stack.

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1640701

On Thu, May 11, 2017 at 10:54 PM, Martin Schwidefsky
<schwidefsky@de.ibm.com> wrote:
> On Thu, 11 May 2017 22:34:31 -0700
> Kees Cook <keescook@chromium.org> wrote:
>
>> On Thu, May 11, 2017 at 10:28 PM, Martin Schwidefsky
>> <schwidefsky@de.ibm.com> wrote:
>> > On Thu, 11 May 2017 16:44:07 -0700
>> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
>> >
>> >> On Thu, May 11, 2017 at 4:17 PM, Thomas Garnier <thgarnie@google.com> wrote:
>> >> >
>> >> > Ingo: Do you want the change as-is? Would you like it to be optional?
>> >> > What do you think?
>> >>
>> >> I'm not ingo, but I don't like that patch. It's in the wrong place -
>> >> that system call return code is too timing-critical to add address
>> >> limit checks.
>> >>
>> >> Now what I think you *could* do is:
>> >>
>> >>  - make "set_fs()" actually set a work flag in the current thread flags
>> >>
>> >>  - do the test in the slow-path (syscall_return_slowpath).
>> >>
>> >> Yes, yes, that ends up being architecture-specific, but it's fairly simple.
>> >>
>> >> And it only slows down the system calls that actually use "set_fs()".
>> >> Sure, it will slow those down a fair amount, but they are hopefully a
>> >> small subset of all cases.
>> >>
>> >> How does that sound to people?  Thats' where we currently do that
>> >>
>> >>         if (IS_ENABLED(CONFIG_PROVE_LOCKING) &&
>> >>             WARN(irqs_disabled(), "syscall %ld left IRQs disabled",
>> >> regs->orig_ax))
>> >>                 local_irq_enable();
>> >>
>> >> check too, which is a fairly similar issue.
>> >
>> > This is exactly what Heiko did for the s390 backend as a result of this
>> > discussion. See the _CIF_ASCE_SECONDARY bit in arch/s390/kernel/entry.S,
>> > for the hot patch the check for the bit is included in the general
>> > _CIF_WORK test. Only the slow patch gets a bit slower.
>> >
>> > git commit b5a882fcf146c87cb6b67c6df353e1c042b8773d
>> > "s390: restore address space when returning to user space".
>>
>> If I'm understanding this, it won't catch corruption of addr_limit
>> during fast-path syscalls, though (i.e. addr_limit changed without a
>> call to set_fs()). :( This addr_limit corruption is mostly only a risk
>> archs without THREAD_INFO_IN_TASK, but it would still be nice to catch
>> unbalanced set_fs() code, so I like the idea. I like getting rid of
>> addr_limit entirely even more, but that'll take some time. :)
>
> Well for s390 there is no addr_limit as we use two separate address space
> for kernel vs. user. The equivalent to the addr_limit corruption on a
> fast-path syscall would be changing CR7 outside of set_fs. This boils
> down to the question what we are protection against? Bad code with
> unbalanced set_fs or evil code that changes addr_limit/CR7 outside of
> set_fs

Yeah, the risk for "corrupted addr_limit" is mainly a concern for
archs with addr_limit on the kernel stack. If I'm reading things
correctly, that means, from the archs I've been paying closer
attention to, it's an issue for arm, mips, and powerpc:

arch/arm/include/asm/uaccess.h: current_thread_info()->addr_limit = fs;
arch/arm/include/asm/thread_info.h:             (current_stack_pointer
& ~(THREAD_SIZE - 1));

arch/mips/include/asm/uaccess.h:#define set_fs(x)
(current_thread_info()->addr_limit = (x))
arch/mips/kernel/process.c:        * task stacks at THREAD_SIZE - 32

arch/powerpc/include/asm/uaccess.h:#define set_fs(val)
(current->thread.fs = (val))
arch/powerpc/kernel/process.c:          struct pt_regs *regs =
task_stack_page(current) + THREAD_SIZE;

(s390 uses a register, x86 and arm64 implement THREAD_INFO_IN_TASK.)
Targeting addr_limit through arbitrary write attacks isn't too common
since ... it's an arbitrary write. The issue with addr_limit was that
it can live on the kernel stack, which meant all kinds of
stack-related bugs can lead to it getting stomped on.

So, two goals to protect addr_limit:

- get it off the stack to make the difficulty of corruption on par
with other sensitive things that would require an arbitrary write
flaw.

- detect/block unbalanced set_fs() calls.

If we can get the former addressed by the remaining architectures,
then that class of attack will go away. For the latter, it sounds like
Linus's slowpath-exit will work nicely.

To me it looks like he architectures with addr_limit still on the
stack would still benefit from always-check-addr_limit on syscall
exit, but that would be arch-specific anyway.

And then, of course, we've got the parallel task of just removing
set_fs() entirely. :)

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1640704

On Fri, May 12, 2017 at 12:01:59PM -0700, Kees Cook wrote:
> Yeah, the risk for "corrupted addr_limit" is mainly a concern for
> archs with addr_limit on the kernel stack. If I'm reading things
> correctly, that means, from the archs I've been paying closer
> attention to, it's an issue for arm, mips, and powerpc:

I'd first want to uninline everything in uaccess.h first that makes
use of access_ok() - which I think is something that needs to happen
anyway.

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

[toc] | [prev] | [next] | [standalone]

#1640231

* Kees Cook <keescook@chromium.org> wrote:

> > git commit b5a882fcf146c87cb6b67c6df353e1c042b8773d
> > "s390: restore address space when returning to user space".
> 
> If I'm understanding this, it won't catch corruption of addr_limit
> during fast-path syscalls, though (i.e. addr_limit changed without a
> call to set_fs()). :(

Nor does it, or the patch you propose, protect against against something 
corrupting task->mm pointer, or the task->*uid values, or any of the myriads of 
security relevant values stored in the task structure!

Making sure API (set_fs()) usage is bug-free and protecting against the effects of 
general data corruption are two unrelated things that should not mixed.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1640221

[resending because kernel.org seems to have mangled my SMTP
credentials.  I wonder if this is a common problem.]

On Thu, May 11, 2017 at 4:44 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, May 11, 2017 at 4:17 PM, Thomas Garnier <thgarnie@google.com> wrote:
>>
>> Ingo: Do you want the change as-is? Would you like it to be optional?
>> What do you think?
>
> I'm not ingo, but I don't like that patch. It's in the wrong place -
> that system call return code is too timing-critical to add address
> limit checks.
>
> Now what I think you *could* do is:
>
>  - make "set_fs()" actually set a work flag in the current thread flags
>
>  - do the test in the slow-path (syscall_return_slowpath).
>
> Yes, yes, that ends up being architecture-specific, but it's fairly simple.
>
> And it only slows down the system calls that actually use "set_fs()".
> Sure, it will slow those down a fair amount, but they are hopefully a
> small subset of all cases.
>
> How does that sound to people?  Thats' where we currently do that
>
>         if (IS_ENABLED(CONFIG_PROVE_LOCKING) &&
>             WARN(irqs_disabled(), "syscall %ld left IRQs disabled",
> regs->orig_ax))
>                 local_irq_enable();
>
> check too, which is a fairly similar issue.
>

I like this.  It wouldn't help the problem that I suspect is a major
part of the motivation for this patch: a stack overflow could
overwrite addr_limit.  But we fixed that for real already.

Slightly off-topic: I would *love* to see syscall_return_slowpath() or
similar moved or at least mostly moved into generic code.  Aside from
the fact that it used to be written in asm, there's nothing
fundamentally arch-specific about it.

>
> And it only slows down the system calls that actually use "set_fs()".
> Sure, it will slow those down a fair amount, but they are hopefully a
> small subset of all cases.

It won't even slow them down that much.  The slow path is reasonably
fast these days.

[toc] | [prev] | [next] | [standalone]

#1640230

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, May 11, 2017 at 4:17 PM, Thomas Garnier <thgarnie@google.com> wrote:
> >
> > Ingo: Do you want the change as-is? Would you like it to be optional?
> > What do you think?
> 
> I'm not ingo, but I don't like that patch. It's in the wrong place -
> that system call return code is too timing-critical to add address
> limit checks.
> 
> Now what I think you *could* do is:
> 
>  - make "set_fs()" actually set a work flag in the current thread flags
> 
>  - do the test in the slow-path (syscall_return_slowpath).
> 
> Yes, yes, that ends up being architecture-specific, but it's fairly simple.
> 
> And it only slows down the system calls that actually use "set_fs()".
> Sure, it will slow those down a fair amount, but they are hopefully a
> small subset of all cases.
> 
> How does that sound to people?  Thats' where we currently do that
> 
>         if (IS_ENABLED(CONFIG_PROVE_LOCKING) &&
>             WARN(irqs_disabled(), "syscall %ld left IRQs disabled",
> regs->orig_ax))
>                 local_irq_enable();
> 
> check too, which is a fairly similar issue.

I really like that idea and I'd be perfectly fine with that solution, because it 
puts the overhead where the problem comes from, and adds an extra incentive for 
code to move away from set_fs() facilities. Win-win.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1640624

On Thu, May 11, 2017 at 11:58 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>> On Thu, May 11, 2017 at 4:17 PM, Thomas Garnier <thgarnie@google.com> wrote:
>> >
>> > Ingo: Do you want the change as-is? Would you like it to be optional?
>> > What do you think?
>>
>> I'm not ingo, but I don't like that patch. It's in the wrong place -
>> that system call return code is too timing-critical to add address
>> limit checks.
>>
>> Now what I think you *could* do is:
>>
>>  - make "set_fs()" actually set a work flag in the current thread flags
>>
>>  - do the test in the slow-path (syscall_return_slowpath).
>>
>> Yes, yes, that ends up being architecture-specific, but it's fairly simple.
>>
>> And it only slows down the system calls that actually use "set_fs()".
>> Sure, it will slow those down a fair amount, but they are hopefully a
>> small subset of all cases.
>>
>> How does that sound to people?  Thats' where we currently do that
>>
>>         if (IS_ENABLED(CONFIG_PROVE_LOCKING) &&
>>             WARN(irqs_disabled(), "syscall %ld left IRQs disabled",
>> regs->orig_ax))
>>                 local_irq_enable();
>>
>> check too, which is a fairly similar issue.
>
> I really like that idea and I'd be perfectly fine with that solution, because it
> puts the overhead where the problem comes from, and adds an extra incentive for
> code to move away from set_fs() facilities. Win-win.

Great, I will adapt the patch for that.

>
> Thanks,
>
>         Ingo



-- 
Thomas

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web