linux-kernel - Re: [PATCH] vfs: rw_copy_check_uvector()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 25 Apr 2014 18:25:23 +0200
From:	Miklos Szeredi <miklos@...redi.hu>
To:	Eric Biggers <ebiggers3@...il.com>
Cc:	Dave Jones <davej@...hat.com>, Al Viro <viro@...iv.linux.org.uk>,
	Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Linux-Fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] vfs: rw_copy_check_uvector() - free iov on error

On Wed, Apr 23, 2014 at 12:06:39AM -0500, Eric Biggers wrote:
> The proposed patch doesn't work because in compat_rw_copy_check_uvector(),
> 'iov' is incremented in the loop before it is freed or returned.  This
> probably should be changed to indexing with 'seg', like in the non-compat
> version...

Do'h, I am indeed blind.

Updated patch below.

Thanks,
Miklos
----

Subject: vfs: rw_copy_check_uvector() - free iov on error
From: Miklos Szeredi <mszeredi@...e.cz>

Some callers (aio_run_iocb, vmsplice_to_user) forget to free the iov on
error.  This seems to be a recurring problem, with most callers being buggy
initially.

So instead of fixing the callers, fix the semantics: free the allocated iov
on error, so callers don't have to.

We could return either fast_pointer or NULL for the error case.  I've opted
for NULL.

Found by Coverity Scan.

Signed-off-by: Miklos Szeredi <mszeredi@...e.cz>
---
 fs/compat.c     |   24 +++++++++++++++---------
 fs/read_write.c |   13 ++++++++++---
 2 files changed, 25 insertions(+), 12 deletions(-)

--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -689,7 +689,7 @@ ssize_t rw_copy_check_uvector(int type,
 	}
 	if (copy_from_user(iov, uvector, nr_segs*sizeof(*uvector))) {
 		ret = -EFAULT;
-		goto out;
+		goto out_free;
 	}
 
 	/*
@@ -710,12 +710,12 @@ ssize_t rw_copy_check_uvector(int type,
 		 * it's about to overflow ssize_t */
 		if (len < 0) {
 			ret = -EINVAL;
-			goto out;
+			goto out_free;
 		}
 		if (type >= 0
 		    && unlikely(!access_ok(vrfy_dir(type), buf, len))) {
 			ret = -EFAULT;
-			goto out;
+			goto out_free;
 		}
 		if (len > MAX_RW_COUNT - ret) {
 			len = MAX_RW_COUNT - ret;
@@ -726,6 +726,13 @@ ssize_t rw_copy_check_uvector(int type,
 out:
 	*ret_pointer = iov;
 	return ret;
+
+out_free:
+	if (iov != fast_pointer) {
+		kfree(iov);
+		iov = NULL;
+	}
+	goto out;
 }
 
 static ssize_t do_readv_writev(int type, struct file *file,
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -549,7 +549,7 @@ ssize_t compat_rw_copy_check_uvector(int
 		struct iovec **ret_pointer)
 {
 	compat_ssize_t tot_len;
-	struct iovec *iov = *ret_pointer = fast_pointer;
+	struct iovec *iov = fast_pointer;
 	ssize_t ret = 0;
 	int seg;
 
@@ -570,11 +570,10 @@ ssize_t compat_rw_copy_check_uvector(int
 		if (iov == NULL)
 			goto out;
 	}
-	*ret_pointer = iov;
 
 	ret = -EFAULT;
 	if (!access_ok(VERIFY_READ, uvector, nr_segs*sizeof(*uvector)))
-		goto out;
+		goto out_free;
 
 	/*
 	 * Single unix specification:
@@ -593,27 +592,34 @@ ssize_t compat_rw_copy_check_uvector(int
 		if (__get_user(len, &uvector->iov_len) ||
 		   __get_user(buf, &uvector->iov_base)) {
 			ret = -EFAULT;
-			goto out;
+			goto out_free;
 		}
 		if (len < 0)	/* size_t not fitting in compat_ssize_t .. */
-			goto out;
+			goto out_free;
 		if (type >= 0 &&
 		    !access_ok(vrfy_dir(type), compat_ptr(buf), len)) {
 			ret = -EFAULT;
-			goto out;
+			goto out_free;
 		}
 		if (len > MAX_RW_COUNT - tot_len)
 			len = MAX_RW_COUNT - tot_len;
 		tot_len += len;
-		iov->iov_base = compat_ptr(buf);
-		iov->iov_len = (compat_size_t) len;
+		iov[seg].iov_base = compat_ptr(buf);
+		iov[seg].iov_len = (compat_size_t) len;
 		uvector++;
-		iov++;
 	}
 	ret = tot_len;
 
 out:
+	*ret_pointer = iov;
 	return ret;
+
+out_free:
+	if (iov != fast_pointer) {
+		kfree(iov);
+		iov = NULL;
+	}
+	goto out;
 }
 
 static inline long
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/