linux: CVE-2014-0069: cifs: incorrect handling of bogus user pointers during uncached writes

Debian Bug report logs - #741958
linux: CVE-2014-0069: cifs: incorrect handling of bogus user pointers during uncached writes

Toggle useless messages

Message #5 received at submit@bugs.debian.org (full text, mbox, reply):

From: Raphael Geissert <geissert@debian.org>

To: Debian Bug Tracking System <submit@bugs.debian.org>

Subject: linux: CVE-2014-0069: cifs: incorrect handling of bogus user pointers during uncached writes

Date: Mon, 17 Mar 2014 17:30:26 +0100

[Message part 1 (text/plain, inline)]

Source: linux
Version: 3.2.51-1
Tags: patch security
X-debbugs-cc: jmm@debian.org

Hi,

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

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

[CVE-2014-0069.patch (text/x-patch, attachment)]

Message #10 received at 741958@bugs.debian.org (full text, mbox, reply):

From: Ben Hutchings <ben@decadent.org.uk>

To: Raphael Geissert <geissert@debian.org>, 741958@bugs.debian.org

Subject: Re: Bug#741958: linux: CVE-2014-0069: cifs: incorrect handling of bogus user pointers during uncached writes

Date: Mon, 07 Apr 2014 02:33:50 +0100

[Message part 1 (text/plain, inline)]

On Mon, 2014-03-17 at 17:30 +0100, Raphael Geissert wrote:
> Source: linux
> Version: 3.2.51-1
> Tags: patch security
> X-debbugs-cc: jmm@debian.org
> 
> Hi,
> 
> Attached patch is what I believe would be the correct backport for 3.2
> of the specific fix for CVE-2014-0069, which is
> 5d81de8e8667da7135d3a32a964087c0faf5483f.

Sorry, I forgot about this and ended up doing my own backport
(attached).  Comparing my diff with yours:

[...]
> 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;
>                 }

All the same so far.

> +               /*
> +                * 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--) ;
> +

I simplified this to:

		npages = i + 1;

but the comment and that simplification are only correct in the error
case so I should use min().

>                 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]);

If cur_len == 0 then also i == 0 and npages == 1, so this loop is
equivalent to:

			kunmap(pages[0]);

which is what I used (avoiding the need to reorder code).

> +                       total_written = -EFAULT;

Good spot, I left this as rc = -EFAULT which will have no effect.
However, to match the upstream version, this assignment should be
dependent on !total_written (i.e. the failure happened before anything
was written).

I'll send another mail with my revised version.

Ben.

> +                       break;
> +               }
> +
>                 do {
>                         if (open_file->invalidHandle) {
>                                 rc = cifs_reopen_file(open_file, false);

-- 
Ben Hutchings
Sturgeon's Law: Ninety percent of everything is crap.

[signature.asc (application/pgp-signature, inline)]

Message #15 received at 741958@bugs.debian.org (full text, mbox, reply):

From: Ben Hutchings <ben@decadent.org.uk>

To: linux-kernel@vger.kernel.org

Cc: stable@vger.kernel.org, akpm@linux-foundation.org, Jeff Layton <jlayton@redhat.com>, Steve French <smfrench@gmail.com>, Al Viro <viro@zeniv.linux.org.uk>, Pavel Shilovsky <piastry@etersoft.ru>, Raphael Geissert <geissert@debian.org>, 741958@bugs.debian.org

Subject: Re: [PATCH 3.2 17/18] cifs: ensure that uncached writes handle unmapped areas correctly

Date: Mon, 07 Apr 2014 02:41:21 +0100

[Message part 1 (text/plain, inline)]

On Mon, 2014-04-07 at 00:35 +0100, Ben Hutchings wrote:
> 3.2.57-rc1 review patch.  If anyone has any objections, please let me know.
[...]

Raphael Geissert previously prepared a backport of this for Debian,
which I overlooked while trying to handle it for 3.2-stable.  When
comparing the two, I found bugs in mine, so here's my v2.

Ben.

---
From: Jeff Layton <jlayton@redhat.com>
Date: Fri, 14 Feb 2014 07:20:35 -0500
Subject: cifs: ensure that uncached writes handle unmapped areas correctly

commit 5d81de8e8667da7135d3a32a964087c0faf5483f upstream.

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]

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>
[bwh: Backported to 3.2:
 - Adjust context
 - s/nr_pages/npages/
 - s/wdata->pages/pages/
 - In case of an error with no data copied, we must kunmap() page 0,
   but in neither case should we free anything else]
 Thanks to Raphael Geissert for an independent backport that showed some
 bugs in my first version.]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 fs/cifs/file.c | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -2107,7 +2107,7 @@ cifs_iovec_write(struct file *file, cons
 {
 	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, cons
 	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;
 		}
 
 		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,
+		 * free anything we allocated and bail out.
+		 */
+		if (!cur_len) {
+			kunmap(pages[0]);
+			if (!total_written)
+				total_written = -EFAULT;
+			break;
+		}
+
+		/*
+		 * i + 1 now represents the number of pages we actually used in
+		 * the copy phase above.
+		 */
+		npages = min(npages, i + 1);
+
 		do {
 			if (open_file->invalidHandle) {
 				rc = cifs_reopen_file(open_file, false);


-- 
Ben Hutchings
Sturgeon's Law: Ninety percent of everything is crap.

[signature.asc (application/pgp-signature, inline)]

Message #20 received at 741958@bugs.debian.org (full text, mbox, reply):

From: Raphael Geissert <geissert@debian.org>

To: Ben Hutchings <ben@decadent.org.uk>

Cc: linux-kernel@vger.kernel.org, stable@vger.kernel.org, akpm@linux-foundation.org, Jeff Layton <jlayton@redhat.com>, Steve French <smfrench@gmail.com>, Al Viro <viro@zeniv.linux.org.uk>, Pavel Shilovsky <piastry@etersoft.ru>, 741958@bugs.debian.org

Subject: Re: [PATCH 3.2 17/18] cifs: ensure that uncached writes handle unmapped areas correctly

Date: Mon, 7 Apr 2014 15:45:51 +0200

Hi,

On 7 April 2014 03:41, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Mon, 2014-04-07 at 00:35 +0100, Ben Hutchings wrote:
>> 3.2.57-rc1 review patch.  If anyone has any objections, please let me know.
> [...]
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
[...]
> +               /*
> +                * i + 1 now represents the number of pages we actually used in
> +                * the copy phase above.
> +                */
> +               npages = min(npages, i + 1);

I'm having trouble understanding why min() is needed here. It
shouldn't harm either, but I find it confusing.

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

Message #25 received at 741958@bugs.debian.org (full text, mbox, reply):

From: Ben Hutchings <ben@decadent.org.uk>

To: Raphael Geissert <geissert@debian.org>

Cc: linux-kernel@vger.kernel.org, stable@vger.kernel.org, akpm@linux-foundation.org, Jeff Layton <jlayton@redhat.com>, Steve French <smfrench@gmail.com>, Al Viro <viro@zeniv.linux.org.uk>, Pavel Shilovsky <piastry@etersoft.ru>, 741958@bugs.debian.org

Subject: Re: [PATCH 3.2 17/18] cifs: ensure that uncached writes handle unmapped areas correctly

Date: Mon, 07 Apr 2014 20:14:16 +0100

[Message part 1 (text/plain, inline)]

On Mon, 2014-04-07 at 15:45 +0200, Raphael Geissert wrote:
> Hi,
> 
> On 7 April 2014 03:41, Ben Hutchings <ben@decadent.org.uk> wrote:
> > On Mon, 2014-04-07 at 00:35 +0100, Ben Hutchings wrote:
> >> 3.2.57-rc1 review patch.  If anyone has any objections, please let me know.
> > [...]
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> [...]
> > +               /*
> > +                * i + 1 now represents the number of pages we actually used in
> > +                * the copy phase above.
> > +                */
> > +               npages = min(npages, i + 1);
> 
> I'm having trouble understanding why min() is needed here. It
> shouldn't harm either, but I find it confusing.

Because in the case where no error occurred, the comment is not true:
i == npages and npages have been used.  The loop used upstream works,
but it seemed a bit silly to use a loop here.

Ben.

-- 
Ben Hutchings
Sturgeon's Law: Ninety percent of everything is crap.

[signature.asc (application/pgp-signature, inline)]

Message #30 received at 741958-done@bugs.debian.org (full text, mbox, reply):

From: Raphael Geissert <geissert@debian.org>

To: 741958-done@bugs.debian.org

Subject: CVE-2014-0069: closing as already fixed

Date: Thu, 12 Jun 2014 16:27:59 +0200

Source: linux
Source-Version: 3.2.57-1

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

Send a report that this bug log contains spam.