linux-kernel - Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 6 Feb 2017 09:37:12 +0100
From:   Miklos Szeredi <miklos@...redi.hu>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Linux NFS list <linux-nfs@...r.kernel.org>,
        ceph-devel@...r.kernel.org, lustre-devel@...ts.lustre.org,
        v9fs-developer@...ts.sourceforge.net,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Jan Kara <jack@...e.cz>,
        Chris Wilson <chris@...is-wilson.co.uk>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Jeff Layton <jlayton@...hat.com>
Subject: Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to
 allocate more pages per call

On Sun, Feb 5, 2017 at 11:04 PM, Al Viro <viro@...iv.linux.org.uk> wrote:
> On Sun, Feb 05, 2017 at 10:19:20PM +0100, Miklos Szeredi wrote:
>
>> Then we can't break out of that deadlock: we wait until
>> fuse_dev_do_write() is done until calling request_end() which
>> ultimately results in unlocking page.  But fuse_dev_do_write() won't
>> complete until the page is unlocked.
>
> Wait a sec.  What happens if
>
> process A: fuse_lookup()
>                 struct fuse_entry_out outarg on stack
>                 ...
>                 fuse_request_send() with req->out.args[0].value = &outarg
>                         sleep in request_wait_answer() on req->waitq
> server: read the request, write reply
>         fuse_dev_do_write()
>                 copy_out_args()
>                         fuse_copy_args()
>                                 fuse_copy_one()
>                                         FR_LOCKED is guaranteed to be set
>                                         fuse_copy_do()
> process C on another CPU: umount -f
>         fuse_conn_abort()
>                 end_requests()
>                         request_end()
>                                 set FR_FINISHED
>                                 wake A up (via req->waitq)
> process A: regain CPU
>         bugger off from request_wait_answer(), through __fuse_request_send(),
>         fuse_request_send(), fuse_simple_request(), fuse_lookup_name(),
>         fuse_lookup() and out of fuse_lookup().
>
> In the meanwhile, server in fuse_copy_do() does memcpy() to what used to
> be outarg, corrupting the stack of process A.
>
> Sure, you need to hit a fairly narrow window, especially if you are to
> cause damage in A, but AFAICS it's not impossible.  Consider e.g. the
> situation when you lose CPU on preempt on the way to memcpy(); in that
> case server might come back when A has incremented its stack footprint
> again.  Or A might end up taking a hardware interrupt and handling it
> on the normal kernel stack, etc.
>
> Looks like *any* scenario where fuse_conn_abort() manages to run during
> that memcpy() has potential for that kind of trouble; any SMP box appears
> to be vulnerable, along with preempt UP...
>
> Am I missing something that prevents that kind of problem?

Yes: if FR_LOCKED is set, then we leave the request alone in
fuse_abort_conn().  Then, when the copy is finished and
request_unlock() is called, we return -ENOENT to fuse_dev_do_write(),
which in turn calls request_end() to wake up the original caller,
which gets -ECONNABORTED.

So basically FR_LOCKED is protecting the copy, which is guaranteed to
be atomic due to the get_user_pages magic that faults in all pages
beforehand.

Thanks,
Miklos