kfifo: sanitize *_user error handling

Right now for kfifo_*_user it's not easily possible to distingush between
a user copy failing and the FIFO not containing enough data.  The problem
is that both conditions are multiplexed into the same return code.

Avoid this by moving the "copy length" into a separate output parameter
and only return 0/-EFAULT in the main return value.

I didn't fully adapt the weird "record" variants, those seem
to be unused anyways and were rather messy (should they be just removed?)

I would appreciate some double checking if I did all the conversions
correctly.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
Cc: Stefani Seibold <stefani@seibold.net>
Cc: Roland Dreier <rdreier@cisco.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Andy Walls <awalls@radix.net>
Cc: Vikram Dhillon <dhillonv10@gmail.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
index 6fb495ea956aa6f11377cdc1277e1aee857fe353..86ad50a900c8a6de172325c0721d7d3225618874 100644 (file)
--- a/include/linux/kfifo.h
+++ b/include/linux/kfifo.h
@@ -235,11 +235,11 @@ static inline __must_check unsigned int kfifo_out_locked(struct kfifo *fifo,
 
 extern void kfifo_skip(struct kfifo *fifo, unsigned int len);
 
-extern __must_check unsigned int kfifo_from_user(struct kfifo *fifo,
-       const void __user *from, unsigned int n);
+extern __must_check int kfifo_from_user(struct kfifo *fifo,
+       const void __user *from, unsigned int n, unsigned *lenout);
 
-extern __must_check unsigned int kfifo_to_user(struct kfifo *fifo,
-       void __user *to, unsigned int n);
+extern __must_check int kfifo_to_user(struct kfifo *fifo,
+       void __user *to, unsigned int n, unsigned *lenout);
 
 /*
  * __kfifo_add_out internal helper function for updating the out offset

diff --git a/kernel/kfifo.c b/kernel/kfifo.c
index ab615e6950525c839ac84e1616945b3e6b946835..b50bb622e8b0892e89c518bd2b548a1bbcc6de75 100644 (file)
--- a/kernel/kfifo.c
+++ b/kernel/kfifo.c
@@ -159,8 +159,9 @@ static inline void __kfifo_out_data(struct kfifo *fifo,
        memcpy(to + l, fifo->buffer, len - l);
 }
 
-static inline unsigned int __kfifo_from_user_data(struct kfifo *fifo,
-        const void __user *from, unsigned int len, unsigned int off)
+static inline int __kfifo_from_user_data(struct kfifo *fifo,
+        const void __user *from, unsigned int len, unsigned int off,
+        unsigned *lenout)
 {
        unsigned int l;
        int ret;
@@ -177,16 +178,20 @@ static inline unsigned int __kfifo_from_user_data(struct kfifo *fifo,
        /* first put the data starting from fifo->in to buffer end */
        l = min(len, fifo->size - off);
        ret = copy_from_user(fifo->buffer + off, from, l);
-
-       if (unlikely(ret))
-               return ret + len - l;
+       if (unlikely(ret)) {
+               *lenout = ret;
+               return -EFAULT;
+       }
+       *lenout = l;
 
        /* then put the rest (if any) at the beginning of the buffer */
-       return copy_from_user(fifo->buffer, from + l, len - l);
+       ret = copy_from_user(fifo->buffer, from + l, len - l);
+       *lenout += ret ? ret : len - l;
+       return ret ? -EFAULT : 0;
 }
 
-static inline unsigned int __kfifo_to_user_data(struct kfifo *fifo,
-               void __user *to, unsigned int len, unsigned int off)
+static inline int __kfifo_to_user_data(struct kfifo *fifo,
+               void __user *to, unsigned int len, unsigned int off, unsigned *lenout)
 {
        unsigned int l;
        int ret;
@@ -203,12 +208,21 @@ static inline unsigned int __kfifo_to_user_data(struct kfifo *fifo,
        /* first get the data from fifo->out until the end of the buffer */
        l = min(len, fifo->size - off);
        ret = copy_to_user(to, fifo->buffer + off, l);
-
-       if (unlikely(ret))
-               return ret + len - l;
+       *lenout = l;
+       if (unlikely(ret)) {
+               *lenout -= ret;
+               return -EFAULT;
+       }
 
        /* then get the rest (if any) from the beginning of the buffer */
-       return copy_to_user(to + l, fifo->buffer, len - l);
+       len -= l;
+       ret = copy_to_user(to + l, fifo->buffer, len);
+       if (unlikely(ret)) {
+               *lenout += len - ret;
+               return -EFAULT;
+       }
+       *lenout += len;
+       return 0;
 }
 
 unsigned int __kfifo_in_n(struct kfifo *fifo,
@@ -299,10 +313,13 @@ EXPORT_SYMBOL(__kfifo_out_generic);
 unsigned int __kfifo_from_user_n(struct kfifo *fifo,
        const void __user *from, unsigned int len, unsigned int recsize)
 {
+       unsigned total;
+
        if (kfifo_avail(fifo) < len + recsize)
                return len + 1;
 
-       return __kfifo_from_user_data(fifo, from, len, recsize);
+       __kfifo_from_user_data(fifo, from, len, recsize, &total);
+       return total;
 }
 EXPORT_SYMBOL(__kfifo_from_user_n);
 
@@ -313,18 +330,21 @@ EXPORT_SYMBOL(__kfifo_from_user_n);
  * @len: the length of the data to be added.
  *
  * This function copies at most @len bytes from the @from into the
- * FIFO depending and returns the number of copied bytes.
+ * FIFO depending and returns -EFAULT/0.
  *
  * Note that with only one concurrent reader and one concurrent
  * writer, you don't need extra locking to use these functions.
  */
-unsigned int kfifo_from_user(struct kfifo *fifo,
-       const void __user *from, unsigned int len)
+int kfifo_from_user(struct kfifo *fifo,
+        const void __user *from, unsigned int len, unsigned *total)
 {
+       int ret;
        len = min(kfifo_avail(fifo), len);
-       len -= __kfifo_from_user_data(fifo, from, len, 0);
+       ret = __kfifo_from_user_data(fifo, from, len, 0, total);
+       if (ret)
+               return ret;
        __kfifo_add_in(fifo, len);
-       return len;
+       return 0;
 }
 EXPORT_SYMBOL(kfifo_from_user);
 
@@ -339,17 +359,17 @@ unsigned int __kfifo_to_user_n(struct kfifo *fifo,
        void __user *to, unsigned int len, unsigned int reclen,
        unsigned int recsize)
 {
-       unsigned int ret;
+       unsigned int ret, total;
 
        if (kfifo_len(fifo) < reclen + recsize)
                return len;
 
-       ret = __kfifo_to_user_data(fifo, to, reclen, recsize);
+       ret = __kfifo_to_user_data(fifo, to, reclen, recsize, &total);
 
        if (likely(ret == 0))
                __kfifo_add_out(fifo, reclen + recsize);
 
-       return ret;
+       return total;
 }
 EXPORT_SYMBOL(__kfifo_to_user_n);
 
@@ -358,20 +378,22 @@ EXPORT_SYMBOL(__kfifo_to_user_n);
  * @fifo: the fifo to be used.
  * @to: where the data must be copied.
  * @len: the size of the destination buffer.
+ @ @lenout: pointer to output variable with copied data
  *
  * This function copies at most @len bytes from the FIFO into the
- * @to buffer and returns the number of copied bytes.
+ * @to buffer and 0 or -EFAULT.
  *
  * Note that with only one concurrent reader and one concurrent
  * writer, you don't need extra locking to use these functions.
  */
-unsigned int kfifo_to_user(struct kfifo *fifo,
-       void __user *to, unsigned int len)
+int kfifo_to_user(struct kfifo *fifo,
+       void __user *to, unsigned int len, unsigned *lenout)
 {
+       int ret;
        len = min(kfifo_len(fifo), len);
-       len -= __kfifo_to_user_data(fifo, to, len, 0);
-       __kfifo_add_out(fifo, len);
-       return len;
+       ret = __kfifo_to_user_data(fifo, to, len, 0, lenout);
+       __kfifo_add_out(fifo, *lenout);
+       return ret;
 }
 EXPORT_SYMBOL(kfifo_to_user);