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>

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>
Andi Kleen · Linus Torvalds
1 parent 8ecc295153
Showing 2 changed files with 53 additions and 31 deletions Side-by-side Diff
include/linux/kfifo.h
kernel/kfifo.c
@@ -235,11 +235,11 @@
  
 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
@@ -159,8 +159,9 @@
 	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 @@
 	/* 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)) {
+		*lenout = ret;
+		return -EFAULT;
+	}
+	*lenout = l;
  
-	if (unlikely(ret))
-		return ret + len - 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 @@
 	/* 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);
+	*lenout = l;
+	if (unlikely(ret)) {
+		*lenout -= ret;
+		return -EFAULT;
+	}
  
-	if (unlikely(ret))
-		return ret + len - l;
-
 	/* 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 @@
 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 @@
  * @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 @@
 	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 @@
  * @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);
...	...	@@ -235,11 +235,11 @@
235	235
236	236	extern void kfifo_skip(struct kfifo *fifo, unsigned int len);
237	237
238		-extern __must_check unsigned int kfifo_from_user(struct kfifo *fifo,
239		- const void __user *from, unsigned int n);
	238	+extern __must_check int kfifo_from_user(struct kfifo *fifo,
	239	+ const void __user from, unsigned int n, unsigned lenout);
240	240
241		-extern __must_check unsigned int kfifo_to_user(struct kfifo *fifo,
242		- void __user *to, unsigned int n);
	241	+extern __must_check int kfifo_to_user(struct kfifo *fifo,
	242	+ void __user to, unsigned int n, unsigned lenout);
243	243
244	244	/*
245	245	* __kfifo_add_out internal helper function for updating the out offset
...	...	@@ -159,8 +159,9 @@
159	159	memcpy(to + l, fifo->buffer, len - l);
160	160	}
161	161
162		-static inline unsigned int __kfifo_from_user_data(struct kfifo *fifo,
163		- const void __user *from, unsigned int len, unsigned int off)
	162	+static inline int __kfifo_from_user_data(struct kfifo *fifo,
	163	+ const void __user *from, unsigned int len, unsigned int off,
	164	+ unsigned *lenout)
164	165	{
165	166	unsigned int l;
166	167	int ret;
167	168
168	169
169	170
...	...	@@ -177,16 +178,20 @@
177	178	/* first put the data starting from fifo->in to buffer end */
178	179	l = min(len, fifo->size - off);
179	180	ret = copy_from_user(fifo->buffer + off, from, l);
	181	+ if (unlikely(ret)) {
	182	+ *lenout = ret;
	183	+ return -EFAULT;
	184	+ }
	185	+ *lenout = l;
180	186
181		- if (unlikely(ret))
182		- return ret + len - l;
183		-
184	187	/* then put the rest (if any) at the beginning of the buffer */
185		- return copy_from_user(fifo->buffer, from + l, len - l);
	188	+ ret = copy_from_user(fifo->buffer, from + l, len - l);
	189	+ *lenout += ret ? ret : len - l;
	190	+ return ret ? -EFAULT : 0;
186	191	}
187	192
188		-static inline unsigned int __kfifo_to_user_data(struct kfifo *fifo,
189		- void __user *to, unsigned int len, unsigned int off)
	193	+static inline int __kfifo_to_user_data(struct kfifo *fifo,
	194	+ void __user to, unsigned int len, unsigned int off, unsigned lenout)
190	195	{
191	196	unsigned int l;
192	197	int ret;
193	198
194	199
...	...	@@ -203,12 +208,21 @@
203	208	/* first get the data from fifo->out until the end of the buffer */
204	209	l = min(len, fifo->size - off);
205	210	ret = copy_to_user(to, fifo->buffer + off, l);
	211	+ *lenout = l;
	212	+ if (unlikely(ret)) {
	213	+ *lenout -= ret;
	214	+ return -EFAULT;
	215	+ }
206	216
207		- if (unlikely(ret))
208		- return ret + len - l;
209		-
210	217	/* then get the rest (if any) from the beginning of the buffer */
211		- return copy_to_user(to + l, fifo->buffer, len - l);
	218	+ len -= l;
	219	+ ret = copy_to_user(to + l, fifo->buffer, len);
	220	+ if (unlikely(ret)) {
	221	+ *lenout += len - ret;
	222	+ return -EFAULT;
	223	+ }
	224	+ *lenout += len;
	225	+ return 0;
212	226	}
213	227
214	228	unsigned int __kfifo_in_n(struct kfifo *fifo,
215	229
...	...	@@ -299,10 +313,13 @@
299	313	unsigned int __kfifo_from_user_n(struct kfifo *fifo,
300	314	const void __user *from, unsigned int len, unsigned int recsize)
301	315	{
	316	+ unsigned total;
	317	+
302	318	if (kfifo_avail(fifo) < len + recsize)
303	319	return len + 1;
304	320
305		- return __kfifo_from_user_data(fifo, from, len, recsize);
	321	+ __kfifo_from_user_data(fifo, from, len, recsize, &total);
	322	+ return total;
306	323	}
307	324	EXPORT_SYMBOL(__kfifo_from_user_n);
308	325
309	326
310	327
311	328
312	329
...	...	@@ -313,18 +330,21 @@
313	330	* @len: the length of the data to be added.
314	331	*
315	332	* This function copies at most @len bytes from the @from into the
316		- * FIFO depending and returns the number of copied bytes.
	333	+ * FIFO depending and returns -EFAULT/0.
317	334	*
318	335	* Note that with only one concurrent reader and one concurrent
319	336	* writer, you don't need extra locking to use these functions.
320	337	*/
321		-unsigned int kfifo_from_user(struct kfifo *fifo,
322		- const void __user *from, unsigned int len)
	338	+int kfifo_from_user(struct kfifo *fifo,
	339	+ const void __user from, unsigned int len, unsigned total)
323	340	{
	341	+ int ret;
324	342	len = min(kfifo_avail(fifo), len);
325		- len -= __kfifo_from_user_data(fifo, from, len, 0);
	343	+ ret = __kfifo_from_user_data(fifo, from, len, 0, total);
	344	+ if (ret)
	345	+ return ret;
326	346	__kfifo_add_in(fifo, len);
327		- return len;
	347	+ return 0;
328	348	}
329	349	EXPORT_SYMBOL(kfifo_from_user);
330	350
331	351
332	352
...	...	@@ -339,17 +359,17 @@
339	359	void __user *to, unsigned int len, unsigned int reclen,
340	360	unsigned int recsize)
341	361	{
342		- unsigned int ret;
	362	+ unsigned int ret, total;
343	363
344	364	if (kfifo_len(fifo) < reclen + recsize)
345	365	return len;
346	366
347		- ret = __kfifo_to_user_data(fifo, to, reclen, recsize);
	367	+ ret = __kfifo_to_user_data(fifo, to, reclen, recsize, &total);
348	368
349	369	if (likely(ret == 0))
350	370	__kfifo_add_out(fifo, reclen + recsize);
351	371
352		- return ret;
	372	+ return total;
353	373	}
354	374	EXPORT_SYMBOL(__kfifo_to_user_n);
355	375
356	376
357	377
358	378
359	379
...	...	@@ -358,20 +378,22 @@
358	378	* @fifo: the fifo to be used.
359	379	* @to: where the data must be copied.
360	380	* @len: the size of the destination buffer.
	381	+ @ @lenout: pointer to output variable with copied data
361	382	*
362	383	* This function copies at most @len bytes from the FIFO into the
363		- * @to buffer and returns the number of copied bytes.
	384	+ * @to buffer and 0 or -EFAULT.
364	385	*
365	386	* Note that with only one concurrent reader and one concurrent
366	387	* writer, you don't need extra locking to use these functions.
367	388	*/
368		-unsigned int kfifo_to_user(struct kfifo *fifo,
369		- void __user *to, unsigned int len)
	389	+int kfifo_to_user(struct kfifo *fifo,
	390	+ void __user to, unsigned int len, unsigned lenout)
370	391	{
	392	+ int ret;
371	393	len = min(kfifo_len(fifo), len);
372		- len -= __kfifo_to_user_data(fifo, to, len, 0);
373		- __kfifo_add_out(fifo, len);
374		- return len;
	394	+ ret = __kfifo_to_user_data(fifo, to, len, 0, lenout);
	395	+ __kfifo_add_out(fifo, *lenout);
	396	+ return ret;
375	397	}
376	398	EXPORT_SYMBOL(kfifo_to_user);
377	399