Re: [patch NOT added to 3.12 stable tree] Don't feed anything but regular iovec's to blk_rq_map_user_iov

[Al Viro <viro@ZenIV.linux.org.uk>]

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.