From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <stable-owner@vger.kernel.org>
Received: from zeniv.linux.org.uk ([195.92.253.2]:33302 "EHLO
        ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S933696AbcLMRFo (ORCPT
        <rfc822;stable@vger.kernel.org>); Tue, 13 Dec 2016 12:05:44 -0500
Date: Tue, 13 Dec 2016 17:05:39 +0000
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Jiri Slaby <jslaby@suse.cz>, Greg KH <greg@kroah.com>,
        stable <stable@vger.kernel.org>
Subject: Re: [patch NOT added to 3.12 stable tree] Don't feed anything but
 regular iovec's to blk_rq_map_user_iov
Message-ID: <20161213170539.GN1555@ZenIV.linux.org.uk>
References: <20161212150727.16486-1-jslaby@suse.cz>
 <20161212224937.GA32167@kroah.com>
 <7645f60e-6b45-828d-7964-033c9b4296df@suse.cz>
 <20161213152428.GA14683@kroah.com>
 <2924abbf-ceb2-cc73-1675-be8a07051c00@suse.cz>
 <CA+55aFxrWdGaYMCmPr712S2kbiWUDO9ED1EcTn1B+nhnSnY9TQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CA+55aFxrWdGaYMCmPr712S2kbiWUDO9ED1EcTn1B+nhnSnY9TQ@mail.gmail.com>
Sender: stable-owner@vger.kernel.org
List-ID: <stable.vger.kernel.org>

On Tue, Dec 13, 2016 at 08:46:56AM -0800, Linus Torvalds wrote:
> On Tue, Dec 13, 2016 at 7:32 AM, Jiri Slaby <jslaby@suse.cz> wrote:
> >
> > Nope, blk_rq_map_user_iov above does not even have "const struct
> > iov_iter *iter" as a parameter yet. Instead, "struct sg_iovec *iov" is
> > there.
> >
> > So maybe, the patch is not even needed. But whoever can tell me for
> > sure, please do so.
> 
> The issue is that we don't wand to get there through splice() creating
> an iov that has kernel addresses in it, but I don't think 3.12 can do
> that anyway - it would need an explicit splice function in the file
> operations.

Not really - for 3.12 NULL ->splice_write() means going for
default_file_splice_write(), which will call write_pipe_buf() on the kmapped
pipe buffers, which will call __kernel_write(), which will do
set_fs(get_ds()); and call ->write().

And sg_write() called under set_fs(KERNEL_DS) is vulnerable there as well -
if nothing else, sg_write() -> sg_new_write():
694)        if (__copy_from_user(hp, buf, SZ_SG_IO_HDR)) {
...
722)        if (!access_ok(VERIFY_READ, hp->cmdp, hp->cmd_len)) {
723)                sg_remove_request(sfp, srp);
724)                return -EFAULT; /* protects following copy_from_user()s + get_user()s */
725)        }
726)        if (__copy_from_user(cmnd, hp->cmdp, hp->cmd_len)) {
already gives you read from arbitrary kernel address, and when you get
to sg_start_req() you have
701)                iov = memdup_user(hp->dxferp, size);
702)                if (IS_ERR(iov))
703)                        return PTR_ERR(iov);
704) 
705)                len = iov_length(iov, iov_count);
706)                if (hp->dxfer_len < len) {
707)                        iov_count = iov_shorten(iov, iov_count, hp->dxfer_len);
708)                        len = hp->dxfer_len;
709)                }
710) 
711)                res = blk_rq_map_user_iov(q, rq, md, (struct sg_iovec *)iov,
712)                                          iov_count,
713)                                          len, GFP_ATOMIC);
and there's your kernel-backed iovec passed to blk_rq_map_user_iov().  All
access_ok() will pass, of course, and if you have misaligned iovec array
element you'll force it into __bio_copy_vec(), with copy_{to,from}_user()
on user-supplied kernel address under set_fs(KERNEL_DS).  IOW, reads
and writes at arbitrary kernel address...

> So I think 3.12 is fine without this. Al?

I really doubt it - there might be something subtle I'd missed, but AFAICS
it is vulnerable to the scenario above.