[PATCH] vfs: rw_copy_check_uvector()

* [PATCH] vfs: rw_copy_check_uvector() - free iov on error
@ 2014-04-15 14:57 Miklos Szeredi
  2014-04-16 18:04 ` Dave Jones
  0 siblings, 1 reply; 9+ messages in thread
From: Miklos Szeredi @ 2014-04-15 14:57 UTC (permalink / raw)
  To: Al Viro; +Cc: linux-kernel, linux-fsdevel

From: Miklos Szeredi <mszeredi@suse.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@suse.cz>
Cc: stable@vger.kernel.org
---
 fs/compat.c     |   19 +++++++++++++------
 fs/read_write.c |   13 ++++++++++---
 2 files changed, 23 insertions(+), 9 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,14 +592,14 @@ 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;
@@ -613,7 +612,15 @@ ssize_t compat_rw_copy_check_uvector(int
 	ret = tot_len;
 
 out:
+	*ret_pointer = iov;
 	return ret;
+
+out_free:
+	if (iov != fast_pointer) {
+		kfree(iov);
+		iov = NULL;
+	}
+	goto out;
 }
 
 static inline long

^ permalink raw reply	[flat|nested] 9+ messages in thread