Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1632979 > unrolled thread
| Started by | Thomas Garnier <thgarnie@google.com> |
|---|---|
| First post | 2017-04-28 17:40 +0200 |
| Last post | 2017-05-09 08:40 +0200 |
| Articles | 20 on this page of 47 — 13 participants |
Back to article view | Back to linux.kernel
[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 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: [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 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 2 of 3 — ← Prev page 1 [2] 3 Next page →
| From | Andy Lutomirski <luto@kernel.org> |
|---|---|
| Date | 2017-05-10 04:10 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFpeG-1Rz-5@gated-at.bofh.it> |
| In reply to | #1638451 |
On Tue, May 9, 2017 at 4:31 PM, Kees Cook <keescook@chromium.org> wrote: > On Tue, May 9, 2017 at 3:52 PM, Andy Lutomirski <luto@amacapital.net> wrote: >> On Tue, May 9, 2017 at 9:50 AM, Kees Cook <keescook@chromium.org> wrote: >>> On Tue, May 9, 2017 at 9:03 AM, 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 >>> >>> Ooooh yes! Nice work. >>> >>> I love this: >>> http://git.infradead.org/users/hch/vfs.git/commitdiff/51e83f50f824ca23f5584c172138e6b7c2ff786d >>> but I wonder what it'll cause out-of-tree code to do. I mean, I'd >>> rather nothing out-of-tree be calling these, but I'd hate 3rd party >>> hacks even more. >>> >>> http://git.infradead.org/users/hch/vfs.git/commitdiff/018e0e9030777121fe87e89d43066691e7366587 >>> This accidentally(?) removes the kernel-doc comments. >>> >>> http://git.infradead.org/users/hch/vfs.git/commitdiff/78b62c730254fc39fa888cdbdca08fde6e09a798 >>> Could this be made defensive? (Return 0 if ret wraps, for example?) I >>> see what the comment says, but not everyone will read that. :( >>> >>> http://git.infradead.org/users/hch/vfs.git/commitdiff/a106276ca0294be054bc89ce97219933fe543df1 >>> Perhaps unconditionally set USER_DS on exit instead of retaining >>> whatever was there? >> >> I don't like silent fixups. If we want to do this, we should BUG or >> at least WARN, not just change the addr limit. But I'm also not >> convinced it's indicative of an actual bug here. > > Nothing should enter that function with KERNEL_DS set, right? > > BUG_ON(get_fs() != USER_DS); > set_fs(KERNEL_DS); > ... > set_fs(USER_DS); > It's not immediately obvious to me that this shouldn't happen. Why not do it the way Christoph did and then, once the rest of set_fs() is tamed, consider this change (or trying to mass-convert file_operations implementations to get rid of it entirely)? --Andy
[toc] | [prev] | [next] | [standalone]
| From | Christoph Hellwig <hch@infradead.org> |
|---|---|
| Date | 2017-05-10 09:20 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFu4G-69Y-9@gated-at.bofh.it> |
| In reply to | #1638451 |
On Tue, May 09, 2017 at 04:31:00PM -0700, Kees Cook wrote: > > I don't like silent fixups. If we want to do this, we should BUG or > > at least WARN, not just change the addr limit. But I'm also not > > convinced it's indicative of an actual bug here. > > Nothing should enter that function with KERNEL_DS set, right? It might very well do. Various drivers or the networking code mess with the address limits for fairly broad sections of code.
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@alien8.de> |
|---|---|
| Date | 2017-05-11 13:30 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFUs9-5GE-9@gated-at.bofh.it> |
| In reply to | #1638451 |
On Tue, May 09, 2017 at 04:31:00PM -0700, Kees Cook wrote:
> > I don't like silent fixups. If we want to do this, we should BUG or
> > at least WARN, not just change the addr limit. But I'm also not
> > convinced it's indicative of an actual bug here.
>
> Nothing should enter that function with KERNEL_DS set, right?
>
> BUG_ON(get_fs() != USER_DS);
We're feeling triggerhappy, aren't we? A nice juicy WARN-splat along
with a fixup looks much better than killing the box, to me.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
[toc] | [prev] | [next] | [standalone]
| From | Christoph Hellwig <hch@infradead.org> |
|---|---|
| Date | 2017-05-10 08:50 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFtBD-5Jp-3@gated-at.bofh.it> |
| In reply to | #1638283 |
On Tue, May 09, 2017 at 09:50:32AM -0700, Kees Cook wrote: > http://git.infradead.org/users/hch/vfs.git/commitdiff/018e0e9030777121fe87e89d43066691e7366587 > This accidentally(?) removes the kernel-doc comments. That was sort of intentional, the kerneldoc boilerplate adds very little value for method instances. > http://git.infradead.org/users/hch/vfs.git/commitdiff/78b62c730254fc39fa888cdbdca08fde6e09a798 > Could this be made defensive? (Return 0 if ret wraps, for example?) I > see what the comment says, but not everyone will read that. :( It's a straight copy of iov_length, and I'd rather keep it that way.
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2017-05-10 04:20 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFpom-21R-5@gated-at.bofh.it> |
| In reply to | #1638255 |
On Tue, May 09, 2017 at 09:03:22AM -0700, Christoph Hellwig 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. Oh? > 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 And just what happens to driver that has no ->read_iter()? Unless I'm seriously misreading that, NAK with extreme prejudice.
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2017-05-10 04:50 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFpRo-2n4-7@gated-at.bofh.it> |
| In reply to | #1638496 |
On Wed, May 10, 2017 at 03:11:18AM +0100, Al Viro wrote: > On Tue, May 09, 2017 at 09:03:22AM -0700, Christoph Hellwig 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. > > Oh? > > > 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 > > And just what happens to driver that has no ->read_iter()? Unless I'm > seriously misreading that, NAK with extreme prejudice. 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. That stuff is used in too many situations; we can't guarantee that all of them will be for files that have those. 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: arch/s390/hypfs/inode.c:437: .read_iter = hypfs_read_iter, drivers/char/mem.c:798: .read_iter = read_iter_null, drivers/char/mem.c:813: .read_iter = read_iter_zero, drivers/char/mem.c:824: .read_iter = read_iter_zero, drivers/char/raw.c:286: .read_iter = blkdev_read_iter, drivers/net/tap.c:1134: .read_iter = tap_read_iter, drivers/net/tun.c:2423: .read_iter = tun_chr_read_iter, drivers/usb/gadget/function/f_fs.c:1255: .read_iter = ffs_epfile_read_iter, drivers/usb/gadget/legacy/inode.c:703: .read_iter = ep_read_iter, drivers/vhost/net.c:1252: .read_iter = vhost_net_chr_read_iter, fs/9p/vfs_file.c:641: .read_iter = generic_file_read_iter, fs/9p/vfs_file.c:652: .read_iter = generic_file_read_iter, fs/9p/vfs_file.c:664: .read_iter = v9fs_file_read_iter, fs/9p/vfs_file.c:675: .read_iter = v9fs_file_read_iter, fs/9p/vfs_file.c:687: .read_iter = v9fs_mmap_file_read_iter, fs/9p/vfs_file.c:698: .read_iter = v9fs_mmap_file_read_iter, fs/fuse/cuse.c:180: .read_iter = cuse_read_iter, fs/fuse/file.c:3015: .read_iter = fuse_direct_read_iter, fs/hugetlbfs/inode.c:980: .read_iter = hugetlbfs_read_iter, fs/ncpfs/file.c:248: .read_iter = ncp_file_read_iter, fs/orangefs/file.c:742: .read_iter = orangefs_file_read_iter, fs/pipe.c:1011: .read_iter = pipe_read, sound/core/pcm_native.c:3696: .read_iter = snd_pcm_readv, is the full list and I'm fairly certain that most of them will work with generic_file_splice_read() just fine. drivers/char definitely will, so will ncpfs/orangefs/hugetlbfs/most of 9p ones (two of the latter might need some care in p9_client_read(), but that should be doable easily enough). pipe is irrelevant (->splice_read() won't be called for those). fuse ones should be doable, but that might take a bit more infrastructure work in lib/iov_iter.c. vhost, gadgetfs, tun/tap - no idea at the moment.
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2017-05-10 05:20 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFqkp-2YZ-1@gated-at.bofh.it> |
| In reply to | #1638515 |
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. Egads... OK, I *have* misread what you are doing there. Your vfs_iter_read() works for files sans ->read_iter(). For strange values of "works" - you hardwire "it's either iovec or kvec iterator" into its calling conventions, which is a trouble waiting to happen. What's the point? What's wrong with having kernel_read()/kernel_readv()/etc.? You still have set_fs() in there; doing that one level up in call chain would be just fine... IDGI. Broken commit: "net: don't play with address limits in kernel_recvmsg". It would be OK if it was only about data. Unfortunately, that's not true in one case: svc_udp_recvfrom() wants ->msg_control. Another delicate place: you can't assume that write() always advances file position by its (positive) return value. btrfs stuff is sensitive to that. ashmem probably _is_ OK with demanding ->read_iter(), but I'm not sure about blind asma->file->f_pos += ret. That's begging for races. Actually, scratch that - it *is* racy.
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2017-05-10 05:30 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFqu5-34C-11@gated-at.bofh.it> |
| In reply to | #1638521 |
On Wed, May 10, 2017 at 04:12:54AM +0100, Al Viro wrote: > Broken commit: "net: don't play with address limits in kernel_recvmsg". > It would be OK if it was only about data. Unfortunately, that's not > true in one case: svc_udp_recvfrom() wants ->msg_control. > > Another delicate place: you can't assume that write() always advances > file position by its (positive) return value. btrfs stuff is sensitive > to that. > > ashmem probably _is_ OK with demanding ->read_iter(), but I'm not sure > about blind asma->file->f_pos += ret. That's begging for races. Actually, > scratch that - it *is* racy. kvec_length(): please, don't. I would rather have the last remaining iov_length() gone... What do you need it for, anyway? You have only two users and both have the count passed to them (as *count and *cnt resp.)
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2017-05-10 05:50 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFqNr-3jp-5@gated-at.bofh.it> |
| In reply to | #1638526 |
On Wed, May 10, 2017 at 04:21:37AM +0100, Al Viro wrote: > On Wed, May 10, 2017 at 04:12:54AM +0100, Al Viro wrote: > > > Broken commit: "net: don't play with address limits in kernel_recvmsg". > > It would be OK if it was only about data. Unfortunately, that's not > > true in one case: svc_udp_recvfrom() wants ->msg_control. > > > > Another delicate place: you can't assume that write() always advances > > file position by its (positive) return value. btrfs stuff is sensitive > > to that. > > > > ashmem probably _is_ OK with demanding ->read_iter(), but I'm not sure > > about blind asma->file->f_pos += ret. That's begging for races. Actually, > > scratch that - it *is* racy. > > kvec_length(): please, don't. I would rather have the last remaining > iov_length() gone... What do you need it for, anyway? You have only > two users and both have the count passed to them (as *count and *cnt resp.) fcntl stuff: I've decided not to put something similar into work.compat since I couldn't decide what to do with compat stuff - word-by-word copy from userland converting to struct flock + conversion to posix_lock + actual work + conversion to flock + word-by-word copy to userland... Smells like we might be better off with compat_flock_to_posix_lock() et.al. I'm still not sure; played a bit one way and another and dediced to drop it for now. Hell knows...
[toc] | [prev] | [next] | [standalone]
| From | Christoph Hellwig <hch@infradead.org> |
|---|---|
| Date | 2017-05-10 09:00 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFtLj-5MF-1@gated-at.bofh.it> |
| In reply to | #1638541 |
On Wed, May 10, 2017 at 04:39:12AM +0100, Al Viro wrote: > fcntl stuff: I've decided not to put something similar into work.compat > since I couldn't decide what to do with compat stuff - word-by-word copy > from userland converting to struct flock + conversion to posix_lock + > actual work + conversion to flock + word-by-word copy to userland... Smells > like we might be better off with compat_flock_to_posix_lock() et.al. > I'm still not sure; played a bit one way and another and dediced to drop > it for now. Hell knows... My version already is an improvement in lines of code alone. Between that and stopping to mess with the address limit I think it's a clear winner. But it's pretty independent of the rest, and I'll just run it through Jeff and Bruce and ask them what they think.
[toc] | [prev] | [next] | [standalone]
| From | Christoph Hellwig <hch@infradead.org> |
|---|---|
| Date | 2017-05-10 09:00 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFtLk-5MF-25@gated-at.bofh.it> |
| In reply to | #1638521 |
On Wed, May 10, 2017 at 04:12:54AM +0100, Al Viro wrote: > What's the point? What's wrong with having kernel_read()/kernel_readv()/etc.? > You still have set_fs() in there; doing that one level up in call chain would > be just fine... IDGI. The problem is that they modify the address limit, which the whole subthread here wants to get rid of. > Broken commit: "net: don't play with address limits in kernel_recvmsg". > It would be OK if it was only about data. Unfortunately, that's not > true in one case: svc_udp_recvfrom() wants ->msg_control. Dropped, but we'll need to fix that eventually. > Another delicate place: you can't assume that write() always advances > file position by its (positive) return value. btrfs stuff is sensitive > to that. If we don't want to assume that we need to pass pointer to pos to kernel_read/write. Which might be a good idea in general. > ashmem probably _is_ OK with demanding ->read_iter(), but I'm not sure > about blind asma->file->f_pos += ret. That's begging for races. Actually, > scratch that - it *is* racy. I think the proper fix is to not even bother to maintain f_pos of the backing file, as we don't ever use it - all reads from it pass in an explicit position anyway.
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2017-05-10 09:30 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFuem-6dd-15@gated-at.bofh.it> |
| In reply to | #1638596 |
On Tue, May 09, 2017 at 11:53:01PM -0700, Christoph Hellwig wrote: > On Wed, May 10, 2017 at 04:12:54AM +0100, Al Viro wrote: > > What's the point? What's wrong with having kernel_read()/kernel_readv()/etc.? > > You still have set_fs() in there; doing that one level up in call chain would > > be just fine... IDGI. > > The problem is that they modify the address limit, which the whole > subthread here wants to get rid of. And you *still* do the same. Christoph, this is ridiculous - the worst part of the area is not a couple of functions in fs/read_write.c, it's a fucking lot of ->read() and ->write() instances in shitty driver code, pardon the redundance. And _that_ is still done under set_fs(KERNEL_DS). Claiming that set_fs() done one function deeper in callchain (both in fs/read_write.c) is somehow better because it reduces the amount of code under that thing... Get real, please - helpers that encapsulate those set_fs() pairs (a-la kernel_read(), etc.) absolutely make sense and converting their open-coded instances to calls of those helpers is clearly a good thing. However, we are not * getting rid of low-quality code run under KERNEL_DS * gettind rid of set_fs() itself * getting a generic kernel_read() variant that would really take an iov_iter. That's what I'm objecting to. Centralized kernel_readv() et.al. - sure, and fs/read_write.c is the right place for those. No arguments here. Conversion to those - absolutely; drivers have no fucking business touching set_fs() at all. But your primitives are trouble waiting to happen. Let them take kvec arrays. And let them, in case when there's no ->read_iter()/->write_iter(), do set_fs(). Statically, without this if (iter->type & ITER_KVEC) ... stuff. > > Another delicate place: you can't assume that write() always advances > > file position by its (positive) return value. btrfs stuff is sensitive > > to that. > > If we don't want to assume that we need to pass pointer to pos to > kernel_read/write. Which might be a good idea in general. Yes. > > ashmem probably _is_ OK with demanding ->read_iter(), but I'm not sure > > about blind asma->file->f_pos += ret. That's begging for races. Actually, > > scratch that - it *is* racy. > > I think the proper fix is to not even bother to maintain f_pos of the > backing file, as we don't ever use it - all reads from it pass in > an explicit position anyway. vfs_llseek() used by ashmem_llseek()...
[toc] | [prev] | [next] | [standalone]
| From | Christoph Hellwig <hch@infradead.org> |
|---|---|
| Date | 2017-05-10 09:40 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFuo3-6ge-23@gated-at.bofh.it> |
| In reply to | #1638609 |
On Wed, May 10, 2017 at 08:27:47AM +0100, Al Viro wrote: > And you *still* do the same. Christoph, this is ridiculous - the worst > part of the area is not a couple of functions in fs/read_write.c, it's > a fucking lot of ->read() and ->write() instances in shitty driver code, > pardon the redundance. And _that_ is still done under set_fs(KERNEL_DS). For now yes. But it provides a sane API on top, and then we can start moving the drivers that matter to the iter variants and drop support for the rest soon. Most in-kernel I/O is done to files, and the rest to a very limited set of devices. (not accounting for sockets through their own APIs, thats another story) > That's what I'm objecting to. Centralized kernel_readv() et.al. - sure, > and fs/read_write.c is the right place for those. No arguments here. > Conversion to those - absolutely; drivers have no fucking business touching > set_fs() at all. But your primitives are trouble waiting to happen. > Let them take kvec arrays. And let them, in case when there's no > ->read_iter()/->write_iter(), do set_fs(). Statically, without this > if (iter->type & ITER_KVEC) ... stuff. Oh weĺl. I can do that first, but I think eventually we'll have to get back to what I've done now.
[toc] | [prev] | [next] | [standalone]
| From | Christoph Hellwig <hch@infradead.org> |
|---|---|
| Date | 2017-05-10 09:00 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFtLk-5MF-21@gated-at.bofh.it> |
| In reply to | #1638515 |
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]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2017-05-10 09:30 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFuen-6dd-25@gated-at.bofh.it> |
| In reply to | #1638255 |
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]
| From | Christoph Hellwig <hch@infradead.org> |
|---|---|
| Date | 2017-05-10 09:40 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFuo2-6ge-15@gated-at.bofh.it> |
| In reply to | #1638610 |
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]
| From | Brian Gerst <brgerst@gmail.com> |
|---|---|
| Date | 2017-05-09 18:10 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFfS1-41K-7@gated-at.bofh.it> |
| In reply to | #1638110 |
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]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2017-05-10 09:40 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFuo2-6ge-17@gated-at.bofh.it> |
| In reply to | #1638110 |
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]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2017-05-10 10:10 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFuR4-6F7-29@gated-at.bofh.it> |
| In reply to | #1638616 |
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]
| From | Christoph Hellwig <hch@infradead.org> |
|---|---|
| Date | 2017-05-10 10:20 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFv0K-6Kh-15@gated-at.bofh.it> |
| In reply to | #1638642 |
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]
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
Back to top | Article view | linux.kernel
csiph-web