Bug 1062578 - CVE-2014-0069 kernel: cifs: uncached writes don't handle bad user addresses correctly [fedora-rawhide]
Summary: CVE-2014-0069 kernel: cifs: uncached writes don't handle bad user addresses c...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: kernel
Version: rawhide
Hardware: All
OS: Linux
urgent
urgent
Target Milestone: ---
Assignee: Jeff Layton
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1062584 1062585 1062590 CVE-2014-0069
TreeView+ depends on / blocked
 
Reported: 2014-02-07 11:19 UTC by Jeff Layton
Modified: 2014-06-18 07:43 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Release Note
Doc Text:
Clone Of:
: 1062584 1062588 1062590 (view as bug list)
Environment:
Last Closed: 2014-02-14 19:12:27 UTC
Type: Bug


Attachments (Terms of Use)
patch -- cifs: ensure that uncached writes handle unmapped areas correctly (3.28 KB, patch)
2014-02-07 11:29 UTC, Jeff Layton
no flags Details | Diff
patch -- cifs: sanity check length of data to send before sending (1.93 KB, patch)
2014-02-12 16:32 UTC, Jeff Layton
no flags Details | Diff

Description Jeff Layton 2014-02-07 11:19:44 UTC
Al Viro discovered the following problem in cifs.ko while overhauling some of the VFS layer write code:

>         Take a look at cifs_iovec_writev().  We have decided that
> the next chunk to send should cover nr_pages worth of data, allocated
> the pages and
>                 save_len = cur_len;
>                 for (i = 0; i < nr_pages; i++) {
>                         copied = min_t(const size_t, cur_len, PAGE_SIZE);
>                         copied = iov_iter_copy_from_user(wdata->pages[i], &it,
>                                                          0, copied);
>                         cur_len -= copied;
>                         iov_iter_advance(&it, copied);
>                 }
>                 cur_len = save_len - cur_len;
> tried to copy from iovec into said pages.  What happens if iovec spans
> an munmapped area?  It'll copy as much as it can and from that point on
> iov_iter_copy_from_user() will be returning 0.  And iov_iter_advance(&it, 0)
> will, of course, do nothing.
>
> So we'll end up with cur_len well less than nr_pages * PAGE_SIZE.  Then we
> start to populate a structure that will be passed to (async) write request:
>                 wdata->sync_mode = WB_SYNC_ALL;
>                 wdata->nr_pages = nr_pages;
>                 wdata->offset = (__u64)offset;
>                 wdata->cfile = cifsFileInfo_get(open_file);
>                 wdata->pid = pid;
>                 wdata->bytes = cur_len;
>                 wdata->pagesz = PAGE_SIZE;
>                 wdata->tailsz = cur_len - ((nr_pages - 1) * PAGE_SIZE);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>                 rc = cifs_uncached_retry_writev(wdata);
>
> and we have a negative number in wdata->tailsz.  I don't have a setup
> ready for testing, so this is strictly from RTFS, but AFAICS we'll send
> nr_pages-1 full pages out (some - with valid data, some - with whatever
> page_alloc() has left there) and then hit the last one.  We kmap it and
> set an iovec with base at the address we'd mapped it on and length
> a bit under 4Gb.  Then we call kernel_sendmsg() on that turd.  Note that
> verify_iovec() isn't called - iovec is already kernel-side anyway and
> kernel_sendmsg() assumes it to be valid.
>
> Again, I hadn't tested it, but it looks like a user-triggerable oops at
> the very least.  All that is needed is writable file on cifs volume
> mounted with cache=strict...  

He's quite correct (though it's even easier to reproduce if you mount with cache=none). I have a reproducer that can trivially crash the kernel when run as an unprivileged user and a patch that fixes the problem. I'll attach those in a bit.

Since fixing this is holding up some important work in other areas, I'm going to propose that we disclose this in one week (February 14th).

Comment 2 Jeff Layton 2014-02-07 11:29:23 UTC
Created attachment 860442 [details]
patch -- cifs: ensure that uncached writes handle unmapped areas correctly

This patch fixes the bug. It should apply fairly cleanly to most recent kernels (including RHEL6/7).

Comment 3 Jeff Layton 2014-02-12 16:32:46 UTC
Created attachment 862435 [details]
patch -- cifs: sanity check length of data to send before sending

Second patch to help prevent issues like this in the future.

If we get a bad send request from the upper layers, have the transport layer throw a warning and an error:

When testing with the reproducer, I get this with this patch applied and not the other one:

[ 2253.669549] WARNING: CPU: 1 PID: 8911 at fs/cifs/transport.c:312 smb_send_rqst+0x22f/0x290 [cifs]()
[ 2253.670968] Send length mismatch(send_length=4294971460 smb_buf_length=4160)

...plus the expected WARN stack trace. I think we'll want to submit both patches for inclusion.

Comment 5 Jeff Layton 2014-02-14 12:28:44 UTC
Patches have now been sent upstream:

    http://article.gmane.org/gmane.linux.kernel.cifs/9401
    http://article.gmane.org/gmane.linux.kernel.cifs/9402

...they should make their way into mainline soon, and the first one should go to stable soon afterward.

Comment 6 Josh Boyer 2014-02-14 17:45:19 UTC
Fixed in git.

Comment 7 Josh Boyer 2014-02-14 19:12:27 UTC
Building.  In the 3.14-rc2-git4 snapshot kernel.  Should hit rawhide tomorrow.


Note You need to log in before you can comment on or make changes to this bug.