[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#741958: marked as done (linux: CVE-2014-0069: cifs: incorrect handling of bogus user pointers during uncached writes)

Your message dated Thu, 12 Jun 2014 16:27:59 +0200
with message-id <CAA7hUgGwUKNm_nLD=O5JNisbYUZXekHykThZ3v-1iVrPnDH-4A@mail.gmail.com>
and subject line CVE-2014-0069: closing as already fixed
has caused the Debian Bug report #741958,
regarding linux: CVE-2014-0069: cifs: incorrect handling of bogus user pointers during uncached writes
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
Bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact owner@bugs.debian.org

741958: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=741958
Debian Bug Tracking System
Contact owner@bugs.debian.org with problems
--- Begin Message ---
Source: linux
Version: 3.2.51-1
Tags: patch security
X-debbugs-cc: jmm@debian.org


Attached patch is what I believe would be the correct backport for 3.2
of the specific fix for CVE-2014-0069, which is

Raphael Geissert - Debian Developer
www.debian.org - get.debian.net
From: Jeff Layton <jlayton@redhat.com>
Date: Fri, 14 Feb 2014 07:20:35 -0500
Subject: [PATCH 2/3] cifs: ensure that uncached writes handle unmapped areas

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>

[geissert: backport to 3.2.51]

 fs/cifs/file.c | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index c55808e..fd07d24 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -2107,7 +2107,7 @@ cifs_iovec_write(struct file *file, const struct iovec *iov,
 	unsigned int written;
 	unsigned long num_pages, npages, i;
-	size_t copied, len, cur_len;
+	size_t bytes, copied, len, cur_len;
 	ssize_t total_written = 0;
 	struct kvec *to_send;
 	struct page **pages;
@@ -2165,17 +2165,45 @@ cifs_iovec_write(struct file *file, const struct iovec *iov,
 	do {
 		size_t save_len = cur_len;
 		for (i = 0; i < npages; i++) {
-			copied = min_t(const size_t, cur_len, PAGE_CACHE_SIZE);
+			bytes = min_t(const size_t, cur_len, PAGE_CACHE_SIZE);
 			copied = iov_iter_copy_from_user(pages[i], &it, 0,
-							 copied);
+							 bytes);
 			cur_len -= copied;
 			iov_iter_advance(&it, copied);
 			to_send[i+1].iov_base = kmap(pages[i]);
 			to_send[i+1].iov_len = copied;
+			/*
+			 * If we didn't copy as much as we expected, then that
+			 * may mean we trod into an unmapped area. Stop copying
+			 * at that point. On the next pass through the big
+			 * loop, we'll likely end up getting a zero-length
+			 * write and bailing out of it.
+			 */
+			if (copied < bytes)
+				break;
+		/*
+		 * i + 1 now represents the number of pages we actually used in
+		 * the copy phase above. Bring npages down to that.
+		 */
+		for ( ; npages > i + 1; npages--) ;
 		cur_len = save_len - cur_len;
+		/*
+		 * If we have no data to send, then that probably means that
+		 * the copy above failed altogether. That's most likely because
+		 * the address in the iovec was bogus. Set the rc to -EFAULT,
+		 * and bail out.
+		 */
+		if (!cur_len) {
+			for (i = 0; i < npages; i++)
+				kunmap(pages[i]);
+			total_written = -EFAULT;
+			break;
+		}
 		do {
 			if (open_file->invalidHandle) {
 				rc = cifs_reopen_file(open_file, false);

--- End Message ---
--- Begin Message ---
Source: linux
Source-Version: 3.2.57-1

Raphael Geissert - Debian Developer
www.debian.org - get.debian.net

--- End Message ---

Reply to: