[PATCH] readv/writev bounds checking fixes

- writev currently returns -EFAULT if _any_ of the segments has an
invalid address.  We should only return -EFAULT if the first segment
has a bad address.

If some of the first segments have valid addresses we need to write
them and return a partial result.

- The current code only checks if the sum-of-lengths is negative.  If
individual segments have a negative length but the result is positive
we miss that.

So rework the code to detect this, and to be immune to odd wrapping
situations.

As a bonus, we save one pass across the iovec.

- ditto for readv.

The check for "does any segment have a negative length" has already
been performed in do_readv_writev(), but it's basically free here, and
we need to do it for generic_file_read/write anyway.

This all means that the iov_length() function is unsafe because of
wrap/overflow isues.  It should only be used after the
generic_file_read/write or do_readv_writev() checking has been
performed.  Its callers have been reviewed and they are OK.

The code now passes LTP testing and has been QA'd by Janet's team.

diff --git a/include/linux/uio.h b/include/linux/uio.h
index ec098c8e67931389b8bc994384d95bb0658499dd..85b2f0ec9d3f0ba65fbed81865a2832f1bedcd56 100644 (file)
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -35,7 +35,11 @@ struct iovec
 #endif
 
 /*
- * Total number of bytes covered by an iovec
+ * Total number of bytes covered by an iovec.
+ *
+ * NOTE that it is not safe to use this function until all the iovec's
+ * segment lengths have been validated.  Because the individual lengths can
+ * overflow a size_t when added together.
  */
 static inline size_t iov_length(const struct iovec *iov, unsigned long nr_segs)
 {

diff --git a/mm/filemap.c b/mm/filemap.c
index ea37b24135aa6435258cb7c6e28a8acfdecbb653..4cfb5939082ec0a5b7937a5c507c3a65ecb53274 100644 (file)
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1134,10 +1134,26 @@ __generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
        struct file *filp = iocb->ki_filp;
        ssize_t retval;
        unsigned long seg;
-       size_t count = iov_length(iov, nr_segs);
+       size_t count;
 
-       if ((ssize_t) count < 0)
-               return -EINVAL;
+       count = 0;
+       for (seg = 0; seg < nr_segs; seg++) {
+               const struct iovec *iv = &iov[seg];
+
+               /*
+                * If any segment has a negative length, or the cumulative
+                * length ever wraps negative then return -EINVAL.
+                */
+               count += iv->iov_len;
+               if (unlikely((ssize_t)(count|iv->iov_len) < 0))
+                       return -EINVAL;
+               if (access_ok(VERIFY_WRITE, iv->iov_base, iv->iov_len))
+                       continue;
+               if (seg == 0)
+                       return -EFAULT;
+               nr_segs = seg;
+               break;
+       }
 
        /* coalesce the iovecs and go direct-to-BIO for O_DIRECT */
        if (filp->f_flags & O_DIRECT) {
@@ -1166,11 +1182,6 @@ __generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
                goto out;
        }
 
-       for (seg = 0; seg < nr_segs; seg++) {
-               if (!access_ok(VERIFY_WRITE,iov[seg].iov_base,iov[seg].iov_len))
-                       return -EFAULT;
-       }
-
        retval = 0;
        if (count) {
                for (seg = 0; seg < nr_segs; seg++) {
@@ -2032,8 +2043,8 @@ generic_file_write_nolock(struct file *file, const struct iovec *iov,
 {
        struct address_space * mapping = file->f_dentry->d_inode->i_mapping;
        struct address_space_operations *a_ops = mapping->a_ops;
-       const size_t ocount = iov_length(iov, nr_segs);
-       size_t count =  ocount;
+       size_t ocount;          /* original count */
+       size_t count;           /* after file limit checks */
        struct inode    *inode = mapping->host;
        unsigned long   limit = current->rlim[RLIMIT_FSIZE].rlim_cur;
        long            status = 0;
@@ -2050,13 +2061,25 @@ generic_file_write_nolock(struct file *file, const struct iovec *iov,
        unsigned long   seg;
        char            *buf;
 
-       if (unlikely((ssize_t)count < 0))
-               return -EINVAL;
-
+       ocount = 0;
        for (seg = 0; seg < nr_segs; seg++) {
-               if (!access_ok(VERIFY_READ,iov[seg].iov_base,iov[seg].iov_len))
+               const struct iovec *iv = &iov[seg];
+
+               /*
+                * If any segment has a negative length, or the cumulative
+                * length ever wraps negative then return -EINVAL.
+                */
+               ocount += iv->iov_len;
+               if (unlikely((ssize_t)(ocount|iv->iov_len) < 0))
+                       return -EINVAL;
+               if (access_ok(VERIFY_READ, iv->iov_base, iv->iov_len))
+                       continue;
+               if (seg == 0)
                        return -EFAULT;
+               nr_segs = seg;
+               break;
        }
+       count = ocount;
 
        pos = *ppos;
        if (unlikely(pos < 0))