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@xxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html