Commit 5d81de8e8667da7135d3a32a964087c0faf5483f

Authored by Jeff Layton
Committed by Steve French
1 parent 42eacf9e57

cifs: ensure that uncached writes handle unmapped areas correctly

It's possible for userland to pass down an iovec via writev() that has a
bogus user pointer in it. If that happens and we're doing an uncached
write, then we can end up getting less bytes than we expect from the
call to iov_iter_copy_from_user. This is CVE-2014-0069

cifs_iovec_write isn't set up to handle that situation however. It'll
blindly keep chugging through the page array and not filling those pages
with anything useful. Worse yet, we'll later end up with a negative
number in wdata->tailsz, which will confuse the sending routines and
cause an oops at the very least.

Fix this by having the copy phase of cifs_iovec_write stop copying data
in this situation and send the last write as a short one. At the same
time, we want to avoid sending a zero-length write to the server, so
break out of the loop and set rc to -EFAULT if that happens. This also
allows us to handle the case where no address in the iovec is valid.

[Note: Marking this for stable on v3.4+ kernels, but kernels as old as
       v2.6.38 may have a similar problem and may need similar fix]

Cc: <stable@vger.kernel.org> # v3.4+
Reviewed-by: Pavel Shilovsky <piastry@etersoft.ru>
Reported-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
Signed-off-by: Steve French <smfrench@gmail.com>

Showing 1 changed file with 34 additions and 3 deletions Side-by-side Diff

... ... @@ -2389,7 +2389,7 @@
2389 2389 unsigned long nr_segs, loff_t *poffset)
2390 2390 {
2391 2391 unsigned long nr_pages, i;
2392   - size_t copied, len, cur_len;
  2392 + size_t bytes, copied, len, cur_len;
2393 2393 ssize_t total_written = 0;
2394 2394 loff_t offset;
2395 2395 struct iov_iter it;
2396 2396  
2397 2397  
2398 2398  
... ... @@ -2444,13 +2444,44 @@
2444 2444  
2445 2445 save_len = cur_len;
2446 2446 for (i = 0; i < nr_pages; i++) {
2447   - copied = min_t(const size_t, cur_len, PAGE_SIZE);
  2447 + bytes = min_t(const size_t, cur_len, PAGE_SIZE);
2448 2448 copied = iov_iter_copy_from_user(wdata->pages[i], &it,
2449   - 0, copied);
  2449 + 0, bytes);
2450 2450 cur_len -= copied;
2451 2451 iov_iter_advance(&it, copied);
  2452 + /*
  2453 + * If we didn't copy as much as we expected, then that
  2454 + * may mean we trod into an unmapped area. Stop copying
  2455 + * at that point. On the next pass through the big
  2456 + * loop, we'll likely end up getting a zero-length
  2457 + * write and bailing out of it.
  2458 + */
  2459 + if (copied < bytes)
  2460 + break;
2452 2461 }
2453 2462 cur_len = save_len - cur_len;
  2463 +
  2464 + /*
  2465 + * If we have no data to send, then that probably means that
  2466 + * the copy above failed altogether. That's most likely because
  2467 + * the address in the iovec was bogus. Set the rc to -EFAULT,
  2468 + * free anything we allocated and bail out.
  2469 + */
  2470 + if (!cur_len) {
  2471 + for (i = 0; i < nr_pages; i++)
  2472 + put_page(wdata->pages[i]);
  2473 + kfree(wdata);
  2474 + rc = -EFAULT;
  2475 + break;
  2476 + }
  2477 +
  2478 + /*
  2479 + * i + 1 now represents the number of pages we actually used in
  2480 + * the copy phase above. Bring nr_pages down to that, and free
  2481 + * any pages that we didn't use.
  2482 + */
  2483 + for ( ; nr_pages > i + 1; nr_pages--)
  2484 + put_page(wdata->pages[nr_pages - 1]);
2454 2485  
2455 2486 wdata->sync_mode = WB_SYNC_ALL;
2456 2487 wdata->nr_pages = nr_pages;