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

Groups > linux.kernel > #1632979 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Thomas Garnier <thgarnie@google.com> - 2017-04-28 17:40 +0200
    Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Thomas Garnier <thgarnie@google.com> - 2017-05-06 00:20 +0200
      Re: [PATCH v9 1/4] syscalls: Verify address limit before returning  to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-08 09:40 +0200
        Re: [PATCH v9 1/4] syscalls: Verify address limit before returning  to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-08 10:00 +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-08 17: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-08 17:30 +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-08 22:00 +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-09 09:00 +0200
                Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Greg KH <greg@kroah.com> - 2017-05-09 13:20 +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-09 16:40 +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 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
                Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-09 18:40 +0200
        Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Greg KH <greg@kroah.com> - 2017-05-08 14:50 +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-09 08:50 +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-09 11: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-09 15: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-09 15: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-09 18: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-09 19:00 +0200
                      Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Andy Lutomirski <luto@amacapital.net> - 2017-05-10 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-10 01:40 +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-10 04: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-10 09:20 +0200
                          Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Borislav Petkov <bp@alien8.de> - 2017-05-11 13:30 +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-10 08: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-10 04:20 +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-10 04: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-10 05:20 +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-10 05: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-10 05:50 +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-10 09:00 +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-10 09:00 +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-10 09:30 +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-10 09:40 +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-10 09:00 +0200
                    Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Arnd Bergmann <arnd@arndb.de> - 2017-05-10 09:30 +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-10 09:40 +0200
                Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Brian Gerst <brgerst@gmail.com> - 2017-05-09 18:10 +0200
                Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Arnd Bergmann <arnd@arndb.de> - 2017-05-10 09:40 +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-10 10: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-10 10:20 +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-11 02: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:10 +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 09:20 +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-12 09:40 +0200
                    Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Arnd Bergmann <arnd@arndb.de> - 2017-05-12 09:50 +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-12 10:20 +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 10:20 +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 10:20 +0200
                        Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address  limit before returning to user-mode Arnd Bergmann <arnd@arndb.de> - 2017-05-12 10:30 +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-13 01:30 +0200
        Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-08 15:20 +0200
          Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Jann Horn <jannh@google.com> - 2017-05-08 16:10 +0200
            Re: [PATCH v9 1/4] syscalls: Verify address limit before returning  to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-08 22:50 +0200
              Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Andy Lutomirski <luto@kernel.org> - 2017-05-13 01:20 +0200
          Re: [PATCH v9 1/4] syscalls: Verify address limit before returning  to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-08 16:10 +0200
            Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-08 17:30 +0200
              Re: [PATCH v9 1/4] syscalls: Verify address limit before returning  to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-09 08:40 +0200

Page 4 of 5 — ← Prev page 1 2 3 [4] 5  Next page →

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

On Wed, May 10, 2017 at 03:45:24AM +0100, Al Viro wrote:
> FWIW, some parts of that queue are obviously sane; it's the conversions of
> kernel_write() and friends to ->read_iter/->write_iter() that are non-starters.

And that part is the main point!

> That stuff is used in too many situations; we can't guarantee that all of
> them will be for files that have those.

That's why this series handles ITER_KVEC for this case, which is all
that's really needed for kernel_read/write.  If you insiste the bvec
and pipe cases are handled as well that couod be added fairly easily.

> As for default_file_splice_read(), I seriously suspect that with your change
> we could as well just make it return -EINVAL and be done with that; places
> that have ->read_iter() tend to have explicit ->splice_read() and it looks
> like the ones that do not should simply use generic_file_read_iter().
> I hadn't checked that, but there's not a lot of those:

Making ->splice_read to default to the ->read_iter based implementation
and returning -EINVAL if neither that nor an explicit ->splice_read
is provided is useful, but wasn't the aim of this series.  Similar
on the write side.

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

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

On Tue, May 9, 2017 at 6:03 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, May 09, 2017 at 06:02:50AM -0700, Christoph Hellwig wrote:
>> On Tue, May 09, 2017 at 06:00:01AM -0700, Andy Lutomirski wrote:
>> > fs/splice.c has some, ahem, interesting uses that have been the source
>> > of nasty exploits in the past.  Converting them to use iov_iter
>> > properly would be really, really nice.  Christoph, I don't suppose
>> > you'd like to do that?
>>
>> I can take care of all the fs code including this one.
>
> I spent the afternoon hacking up where I'd like this to head.  It's
> completely untested as of now:
>
>         http://git.infradead.org/users/hch/vfs.git/shortlog/refs/heads/setfs-elimination

My older time64_t syscall series has the side-effect of doing something
like this to the time-related compat handlers in kernel/compat.c. If nobody
else has started looking at removing set_fs from those, I can extract
the relevant parts from my series.

      Arnd

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

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

On Wed, May 10, 2017 at 09:28:48AM +0200, Arnd Bergmann wrote:
> My older time64_t syscall series has the side-effect of doing something
> like this to the time-related compat handlers in kernel/compat.c. If nobody
> else has started looking at removing set_fs from those, I can extract
> the relevant parts from my series.

That would be great.

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

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

On Tue, May 9, 2017 at 9:00 AM, Andy Lutomirski <luto@kernel.org> wrote:
> On Tue, May 9, 2017 at 1:56 AM, Christoph Hellwig <hch@infradead.org> wrote:
>> On Tue, May 09, 2017 at 08:45:22AM +0200, Ingo Molnar wrote:
>>> We only have ~115 code blocks in the kernel that set/restore KERNEL_DS, it would
>>> be a pity to add a runtime check to every system call ...
>>
>> I think we should simply strive to remove all of them that aren't
>> in core scheduler / arch code.  Basically evetyytime we do the
>>
>>         oldfs = get_fs();
>>         set_fs(KERNEL_DS);
>>         ..
>>         set_fs(oldfs);
>>
>> trick we're doing something wrong, and there should always be better
>> ways to archive it.  E.g. using iov_iter with a ITER_KVEC type
>> consistently would already remove most of them.
>
> How about trying to remove all of them?  If we could actually get rid
> of all of them, we could drop the arch support, and we'd get faster,
> simpler, shorter uaccess code throughout the kernel.
>
> The ones in kernel/compat.c are generally garbage.  They should be
> using compat_alloc_user_space().  Ditto for kernel/power/user.c.

compat_alloc_user_space() is a hack that should go away too.  It ends
up copying the data three times.

The more efficient solution to this is to have a core syscall function
that only accesses kernel memory, and then have two front-end
functions (native and compat) that do the actual reads and writes to
userspace, with conversion in the compat case.

--
Brian Gerst

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

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

On Tue, May 9, 2017 at 3:00 PM, Andy Lutomirski <luto@kernel.org> wrote:
> On Tue, May 9, 2017 at 1:56 AM, Christoph Hellwig <hch@infradead.org> wrote:
>> On Tue, May 09, 2017 at 08:45:22AM +0200, Ingo Molnar wrote:
>>> We only have ~115 code blocks in the kernel that set/restore KERNEL_DS, it would
>>> be a pity to add a runtime check to every system call ...
>>
>> I think we should simply strive to remove all of them that aren't
>> in core scheduler / arch code.  Basically evetyytime we do the
>>
>>         oldfs = get_fs();
>>         set_fs(KERNEL_DS);
>>         ..
>>         set_fs(oldfs);
>>
>> trick we're doing something wrong, and there should always be better
>> ways to archive it.  E.g. using iov_iter with a ITER_KVEC type
>> consistently would already remove most of them.
>
> How about trying to remove all of them?  If we could actually get rid
> of all of them, we could drop the arch support, and we'd get faster,
> simpler, shorter uaccess code throughout the kernel.
>
> The ones in kernel/compat.c are generally garbage.  They should be
> using compat_alloc_user_space().  Ditto for kernel/power/user.c.

compat_alloc_user_space() has some problems too, it adds
complexity to a rarely-tested code path and can add some noticeable
overhead in cases where user space access is slow because of
extra checks.

It's clearly better than set_fs(), but the way I prefer to convert the
code is to avoid both and instead move compat handlers next to
the native code, and splitting out the common code between native
and compat mode into a helper that takes a regular kernel pointer.

I think that's what both Al has done in the past on compat_ioctl()
and select() and what Christoph does in his latest series, but
it seems worth pointing out for others that decide to help out here.

     Arnd

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

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

On Wed, May 10, 2017 at 09:37:04AM +0200, Arnd Bergmann wrote:

> > How about trying to remove all of them?  If we could actually get rid
> > of all of them, we could drop the arch support, and we'd get faster,
> > simpler, shorter uaccess code throughout the kernel.

BTW, not all get_user() under KERNEL_DS are plain loads.  There is an
exception - probe_kernel_read().

> > The ones in kernel/compat.c are generally garbage.  They should be
> > using compat_alloc_user_space().  Ditto for kernel/power/user.c.
> 
> compat_alloc_user_space() has some problems too, it adds
> complexity to a rarely-tested code path and can add some noticeable
> overhead in cases where user space access is slow because of
> extra checks.
> 
> It's clearly better than set_fs(), but the way I prefer to convert the
> code is to avoid both and instead move compat handlers next to
> the native code, and splitting out the common code between native
> and compat mode into a helper that takes a regular kernel pointer.
> 
> I think that's what both Al has done in the past on compat_ioctl()
> and select() and what Christoph does in his latest series, but
> it seems worth pointing out for others that decide to help out here.

Folks, reducing the amount of places where we play with set_fs() is certainly
a good thing.  Getting rid of them completely is something entirely different;
I have tried to plot out patch series in this direction many times during the
last 5 years or so, but it's not going to be easy.  Tomorrow I can start
posting my notes in that direction (and there are tons of those, unfortunately
mixed with git grep results, highly unprintable personal comments, etc.);
just let me grab some sleep first...

BTW, slow userland access is not just due to extra checks; access_ok(),
in particular, is pretty much noise.  The real PITA comes from the things
like STAC/CLAC on recent x86.  Or hardware overhead of cross-address-space
block copy insn (e.g. on s390, where it's optimized for multi-cacheline
blocks).  Or things like uml, where it's a matter of walking the page
tables for each sodding __get_user().  It's not always just a matter of
address space limit...

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

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

On Wed, May 10, 2017 at 09:08:41AM +0100, Al Viro wrote:
> On Wed, May 10, 2017 at 09:37:04AM +0200, Arnd Bergmann wrote:
> 
> > > How about trying to remove all of them?  If we could actually get rid
> > > of all of them, we could drop the arch support, and we'd get faster,
> > > simpler, shorter uaccess code throughout the kernel.
> 
> BTW, not all get_user() under KERNEL_DS are plain loads.  There is an
> exception - probe_kernel_read().

And various calls that looks like opencoded versions, e.g. drivers/dio
or the ELF loader.

But in the long run we'll just need a separate primitive for that,
but that can wait until the set_fs calls outside the core code are
gone.

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

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

On Wed, May 10, 2017 at 1:14 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, May 10, 2017 at 09:08:41AM +0100, Al Viro wrote:
>> On Wed, May 10, 2017 at 09:37:04AM +0200, Arnd Bergmann wrote:
>>
>> > > How about trying to remove all of them?  If we could actually get rid
>> > > of all of them, we could drop the arch support, and we'd get faster,
>> > > simpler, shorter uaccess code throughout the kernel.
>>
>> BTW, not all get_user() under KERNEL_DS are plain loads.  There is an
>> exception - probe_kernel_read().
>
> And various calls that looks like opencoded versions, e.g. drivers/dio
> or the ELF loader.
>
> But in the long run we'll just need a separate primitive for that,
> but that can wait until the set_fs calls outside the core code are
> gone.

I suspect that, on most arches, the primitive is called
__copy_from_user().  We could make the generic code do that except
where overridden.

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

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

* Andy Lutomirski <luto@kernel.org> wrote:

> On Tue, May 9, 2017 at 1:56 AM, Christoph Hellwig <hch@infradead.org> wrote:
> > On Tue, May 09, 2017 at 08:45:22AM +0200, Ingo Molnar wrote:
> >> We only have ~115 code blocks in the kernel that set/restore KERNEL_DS, it would
> >> be a pity to add a runtime check to every system call ...
> >
> > I think we should simply strive to remove all of them that aren't
> > in core scheduler / arch code.  Basically evetyytime we do the
> >
> >         oldfs = get_fs();
> >         set_fs(KERNEL_DS);
> >         ..
> >         set_fs(oldfs);
> >
> > trick we're doing something wrong, and there should always be better
> > ways to archive it.  E.g. using iov_iter with a ITER_KVEC type
> > consistently would already remove most of them.
> 
> How about trying to remove all of them?  If we could actually get rid
> of all of them, we could drop the arch support, and we'd get faster,
> simpler, shorter uaccess code throughout the kernel.

I'm all for that!

Thanks,

	Ingo

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

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

On Fri, May 12, 2017 at 09:00:12AM +0200, Ingo Molnar wrote:

> > How about trying to remove all of them?  If we could actually get rid
> > of all of them, we could drop the arch support, and we'd get faster,
> > simpler, shorter uaccess code throughout the kernel.
> 
> I'm all for that!

Oh, for...  Ingo, do you really want to go through all ->write() and ->read()
instances, converting all of them to iov_iter?  Or, better yet, deal with
the patch flood from Nick Krause sock puppet brigade?

Folks, seriously, have you even looked through that zoo?  I have, and it's
really, really not fun.  Sure, we can say "fuck 'em, no need to allow
splice() on random crap".  Would be perfectly reasonable, expect that
it's not the only place doing kernel_write() and its ilk...

And converting everything to ->read_iter()/->write_iter() means an insane
amount of code churn, not to mention coping with random bogosities in
semantics.  ->read() and ->write() are going to stay around, pretty
much indefinitely.

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

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

On Fri, May 12, 2017 at 08:15:49AM +0100, Al Viro wrote:
> And converting everything to ->read_iter()/->write_iter() means an insane
> amount of code churn, not to mention coping with random bogosities in
> semantics.  ->read() and ->write() are going to stay around, pretty
> much indefinitely.

But I don't think kernel users of them have to.  I've been digging
through the calllers and will send an analysis to the list in a bit.

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

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

On Fri, May 12, 2017 at 9:15 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Fri, May 12, 2017 at 09:00:12AM +0200, Ingo Molnar wrote:
>
>> > How about trying to remove all of them?  If we could actually get rid
>> > of all of them, we could drop the arch support, and we'd get faster,
>> > simpler, shorter uaccess code throughout the kernel.
>>
>> I'm all for that!
>
> Oh, for...  Ingo, do you really want to go through all ->write() and ->read()
> instances, converting all of them to iov_iter?  Or, better yet, deal with
> the patch flood from Nick Krause sock puppet brigade?

How realistic and how useful would it be to first completely eliminate
the ones that are in loadable modules and then wrapping the definition
in #ifndef MODULE (or even make it an extern function)?

This should be a fairly complete list of the modular users:

drivers/block/drbd/drbd_main.c: set_fs(KERNEL_DS);
drivers/input/serio/hp_sdc.c:   set_fs(KERNEL_DS);
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:          set_fs(KERNEL_DS);
drivers/misc/lkdtm_bugs.c:      set_fs(KERNEL_DS);
drivers/s390/crypto/pkey_api.c: set_fs(KERNEL_DS);
drivers/staging/comedi/drivers/serial2002.c:    set_fs(KERNEL_DS);
drivers/staging/lustre/lnet/libcfs/tracefile.c: set_fs(get_ds());
drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:
 set_fs(KERNEL_DS);
drivers/staging/rtl8723bs/os_dep/osdep_service.c:               oldfs
= get_fs(); set_fs(get_ds());
drivers/usb/gadget/function/f_mass_storage.c:   set_fs(get_ds());
drivers/usb/gadget/function/u_uac1.c:   set_fs(KERNEL_DS);
drivers/vhost/vhost.c:  set_fs(USER_DS);
drivers/video/fbdev/core/fbmem.c:       set_fs(KERNEL_DS);
drivers/video/fbdev/hpfb.c:     set_fs(KERNEL_DS);
fs/autofs4/waitq.c:     set_fs(KERNEL_DS);
fs/binfmt_aout.c:       set_fs(KERNEL_DS);
fs/binfmt_elf.c:                set_fs(USER_DS);
fs/binfmt_elf_fdpic.c:  set_fs(KERNEL_DS);
fs/btrfs/send.c:        set_fs(KERNEL_DS);
fs/ext4/ioctl.c:                set_fs(KERNEL_DS);
fs/nfsd/vfs.c:  set_fs(KERNEL_DS);
net/9p/trans_fd.c:      set_fs(get_ds());
net/ipv6/addrconf.c:                    set_fs(KERNEL_DS);
net/ipv6/exthdrs.c:     set_fs(KERNEL_DS);
net/sunrpc/svcsock.c:   oldfs = get_fs(); set_fs(KERNEL_DS);
sound/core/oss/pcm_oss.c:       set_fs(get_ds());
sound/core/pcm_native.c:        set_fs(get_ds());
sound/drivers/opl3/opl3_oss.c:  set_fs(get_ds());
sound/oss/dmabuf.c:     set_fs(get_ds());
sound/oss/swarm_cs4297a.c:                set_fs(KERNEL_DS);
sound/pci/emu10k1/emufx.c:      set_fs(get_ds());
sound/pci/hda/hda_codec.c:              set_fs(get_ds());

     Arnd

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

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

On Fri, May 12, 2017 at 09:43:40AM +0200, Arnd Bergmann wrote:
> How realistic and how useful would it be to first completely eliminate
> the ones that are in loadable modules and then wrapping the definition
> in #ifndef MODULE (or even make it an extern function)?

Should be fairly doable and might be a nice step towards cleaning the
mess up.  In fact with my seres a large part of those are gone, and
most of the remaining handler are ioctl handlers or what seems like
opencoded versions of probe_kernel_read.

But it won't help against exploits modifying addr_limit manually.

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

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

On Fri, May 12, 2017 at 01:11:26AM -0700, Christoph Hellwig wrote:

> But it won't help against exploits modifying addr_limit manually.

Or the ones setting current->cred to that of init.  Your point being?

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

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

On Fri, May 12, 2017 at 09:43:40AM +0200, Arnd Bergmann wrote:

> How realistic and how useful would it be to first completely eliminate
> the ones that are in loadable modules and then wrapping the definition
> in #ifndef MODULE (or even make it an extern function)?

Eliminate _what_?  ->read() and ->write() instances?

> This should be a fairly complete list of the modular users:
> 
> drivers/block/drbd/drbd_main.c: set_fs(KERNEL_DS);

Ah, set_fs()...  Sure, many of those can be killed off.  Wouldn't be
a bad idea, but I don't understand what difference does modular/built-in
make here...

This one: AFAICS doesn't give a damn about set_fs() at all.

> drivers/input/serio/hp_sdc.c:   set_fs(KERNEL_DS);

Open-coded probe_kernel_read(), apparently.

> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:          set_fs(KERNEL_DS);

massive compat ioctl crap.

> drivers/misc/lkdtm_bugs.c:      set_fs(KERNEL_DS);

insane.

> drivers/s390/crypto/pkey_api.c: set_fs(KERNEL_DS);

No idea.

> drivers/staging/comedi/drivers/serial2002.c:    set_fs(KERNEL_DS);

Open-coded kernel_write(); to some character device, no less...  And similar
for kernel_read(), apparently.

> drivers/staging/lustre/lnet/libcfs/tracefile.c: set_fs(get_ds());

Fuck knows; kernel_write() might do it.  Depends upon what it's writing
to.

You've missed other places in lustre, BTW - including the ioctls on
sockets, etc.

> drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:
>  set_fs(KERNEL_DS);

Compat ioctl crap, again.

> drivers/staging/rtl8723bs/os_dep/osdep_service.c:               oldfs
> = get_fs(); set_fs(get_ds());

Oh, lovely - reading an arbitrary (as in, specified by pathname) file.
Firmware (mis)handling?

> drivers/usb/gadget/function/f_mass_storage.c:   set_fs(get_ds());

No idea.

> drivers/usb/gadget/function/u_uac1.c:   set_fs(KERNEL_DS);

kernel_write(), by the look of it.  Or something similar.

> drivers/vhost/vhost.c:  set_fs(USER_DS);

kernel thread doing use_mm()

> drivers/video/fbdev/core/fbmem.c:       set_fs(KERNEL_DS);

compat ioctl.

> drivers/video/fbdev/hpfb.c:     set_fs(KERNEL_DS);

probe_kernel_read()

> fs/autofs4/waitq.c:     set_fs(KERNEL_DS);

kernel_write()

> fs/binfmt_aout.c:       set_fs(KERNEL_DS);
> fs/binfmt_elf.c:                set_fs(USER_DS);
> fs/binfmt_elf_fdpic.c:  set_fs(KERNEL_DS);

coredump stuff.

> fs/btrfs/send.c:        set_fs(KERNEL_DS);
kernel_write()

Anyway, what's special about modules?  IDGI...

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

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

On Fri, May 12, 2017 at 10:11 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> Anyway, what's special about modules?  IDGI...

One of the arguments that came up earlier was code in external modules
being mostly unaudited, sometimes without any source code available
at all but still used in devices.

If modules can't do set_fs() any more, this could eliminate bugs with
unpaired set_fs in those modules.

Limiting factors of course are:

- embedded systems that ship come with their own kernels (as opposed
  to using whatever users have, or relying on binary distros) can just
  make it available to modules again, by reverting the patch

- As Christoph said, they could have an open-coded set_fs in the
  driver

- Whatever other method a clueless driver write might come up with
  isn't necessarily better than set_fs().

     Arnd

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

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

On Fri, May 12, 2017 at 12:15 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:

> Folks, seriously, have you even looked through that zoo?  I have, and it's
> really, really not fun.  Sure, we can say "fuck 'em, no need to allow
> splice() on random crap".  Would be perfectly reasonable, expect that
> it's not the only place doing kernel_write() and its ilk...

Can you clarify this?  I think we really may be able to do exactly
this.  From Christoph's list, there are only two things that need
kernel_read/kernel_write to user-supplied fds that may come from a
variety of sources: splice and exec.  If you're execing a chardev from
a crappy driver, something is seriously wrong.  And returning -EINVAL
from splice() to or from files that use ->read and ->write seems find
(and splice(2) even documents -EINVAL as meaning that the target
doesn't support splicing).

--Andy

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

#1637440

On Mon, May 8, 2017 at 12:33 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> (added more Cc:s)
>
> * Thomas Garnier <thgarnie@google.com> wrote:
>
>> On Fri, Apr 28, 2017 at 8:32 AM, Thomas Garnier <thgarnie@google.com> wrote:
>> > Ensure that a syscall does not return to user-mode with a kernel address
>> > limit. If that happens, a process can corrupt kernel-mode memory and
>> > elevate privileges [1].
>> >
>> > The CONFIG_ADDR_LIMIT_CHECK option disables the generic check so each
>> > architecture can create optimized versions. This option is enabled by
>> > default on s390 because a similar feature already exists.
>> >
>> > [1] https://bugs.chromium.org/p/project-zero/issues/detail?id=990
>> >
>> > Signed-off-by: Thomas Garnier <thgarnie@google.com>
>> > Tested-by: Kees Cook <keescook@chromium.org>
>>
>> Ingo: Do you want to take the set?
>
> Yeah, so now I'm questioning the whole premise of the feature, sorry :-/
>
> A big disavantage is that the "security check" will add 2-5 instructions to the
> system call fast path. Every one of them, and essentially forever. Just to handle
> a CVE that was caused by a buggy touch-screen driver helper function leaking
> KERNEL_DS and which was fixed long ago ...
>
> And yes, I realize that there were other such bugs and that such bugs might occur
> in the future - but why not push the overhead of the security check to the kernel
> build phase? I.e. I'm wondering how well we could do static analysis during kernel
> build - would a limited mode of Sparse be good enough for that? Or we could add a
> new static checker to tools/, built from first principles and used primarily for
> extended syntactical checking.

Static analysis is just not going to cover all cases. We've had
vulnerabilities where interrupt handlers left KERNEL_DS set, for
example. If there are performance concerns, let's put this behind a
CONFIG. 2-5 instructions is not an issue for most people that want
this coverage.

> For example I'd consider it a good practice to mandate that if a kernel function
> sets KERNEL_DS then it must restore it as well. Any function that does not do
> that, or is too complex for the static analysis to prove correctness for sure
> should be considered buggy!
>
> Are there any common kernel APIs outside set_fs() that set KERNEL_DS
> intentionally? The overwhelming pattern ought to be:
>
>         orig_fs = get_fs();
>         set_fs(KERNEL_DS);
>         ...
>         set_fs(orig_fs);
>
> ... and even a relatively simple static analysis tool ought to be able to see
> through that.

This pattern was, in fact, what the interrupt handler bug escaped
from. We have to build proactive defenses, and this check has a clear
defensive advantage. It's a noble goal to improve the static analyzers
and simplify the source, but we have too much history to prove that
this just isn't enough. This instruction cost of this is extremely
small, too. Until we can eliminate set_fs(), we need to add this
check.


> I'd even suggest we do it not like Sparse builds are done today, but in a more
> integrated fashion: do static analysis as part of a typical kernel defconfig build
> and not tolerate warnings but go for a 'zero warnings' policy like Linus uses for
> modconfig builds.
>
> _That_ solution I'd feel very, very good about - it would be so much better than
> any runtime checks...

I'm not opposed to this, but there will be push-back on "making the
build slower", and it still won't catch everything. Bug-finding is
different from making a bug class just unexploitable at all. As we've
done before, it's the difference between trying to find format string
attacks vs just removing %n from the format parser.

> Not to mention that such an integrated static analysis facility would allow many
> other things to be checked during build time, which we couldn't possibly check
> runtime.

Absolutely! But it's orthogonal to proactive runtime exploit blocking.
We've got one that works and defends against an entire class of
vulnerability for very low cost. It it's truly too costly for default,
let's put it behind a CONFIG and see who wants it. (Most distros, I
suspect, will enable it, just like hardened usercopy which is much
more expensive than this.)

-Kees

-- 
Kees Cook
Pixel Security

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

#1637458

On Mon, May 8, 2017 at 4:02 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Kees Cook <keescook@chromium.org> wrote:
>
>> > And yes, I realize that there were other such bugs and that such bugs might
>> > occur in the future - but why not push the overhead of the security check to
>> > the kernel build phase? I.e. I'm wondering how well we could do static
>> > analysis during kernel build - would a limited mode of Sparse be good enough
>> > for that? Or we could add a new static checker to tools/, built from first
>> > principles and used primarily for extended syntactical checking.
>>
>> Static analysis is just not going to cover all cases. We've had vulnerabilities
>> where interrupt handlers left KERNEL_DS set, for example. [...]
>
> Got any commit ID of that bug - was it because a function executed by the
> interrupt handler leaked KERNEL_DS?

I think Kees might be talking about
https://bugs.chromium.org/p/project-zero/issues/detail?id=822, fixed in
commit e6978e4bf181fb3b5f8cb6f71b4fe30fbf1b655c. The issue was that
perf code that can run in pretty much any context called access_ok().

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

#1637705 — Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

On Mon, May 08, 2017 at 04:06:35PM +0200, Jann Horn wrote:

> I think Kees might be talking about
> https://bugs.chromium.org/p/project-zero/issues/detail?id=822, fixed in
> commit e6978e4bf181fb3b5f8cb6f71b4fe30fbf1b655c. The issue was that
> perf code that can run in pretty much any context called access_ok().

And that commit has *NOT* solved the problem.  perf_callchain_user()
can be called synchronously, without passing through that code.
Tracepoint shite...

That set_fs() should be done in get_perf_callchain(), just around the call of
perf_callchain_user().  Along with pagefault_disable(), actually.

BTW, that's a nice example demonstrating why doing that on the kernel
boundary is wrong.  Wider (in theory) area being "protected" => easier
to miss the ways not crossing its border.

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

Page 4 of 5 — ← Prev page 1 2 3 [4] 5  Next page →

Back to top | Article view | linux.kernel

csiph-web