Re: [stable] [PATCH] devmem: check vmalloc address on kmemread/write

[KAMEZAWA Hiroyuki]

On Thu, 4 Feb 2010 10:42:02 +0800
Wu Fengguang <fengguang.wu@xxxxxxxxx> wrote:

> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> 
> commit 325fda71d0badc1073dc59f12a948f24ff05796a upstream.
> 
> Otherwise vmalloc_to_page() will BUG().
> 
> This also makes the kmem read/write implementation aligned with mem(4):
> "References to nonexistent locations cause errors to be returned." Here
> we return -ENXIO (inspired by Hugh) if no bytes have been transfered
> to/from user space, otherwise return partial read/write results.
> 

Wu-san, I have additonal fix to this patch. Now, *ppos update is unstable..
Could you make merged one ?
Maybe this one makes the all behavior clearer.

==
This is a more fix for devmem-check-vmalloc-address-on-kmem-read-write.patch
Now, the condition for updating *ppos is not good. (it's updated even if EFAULT
occurs..). This fixes that.


Reported-by: "Juha Leppanen" <juha_motorsportcom@xxxxxxxxxx>
CC: Wu Fengguang <fengguang.wu@xxxxxxxxx>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
---
 drivers/char/mem.c |   34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

Index: mmotm-2.6.33-Feb01/drivers/char/mem.c
===================================================================
--- mmotm-2.6.33-Feb01.orig/drivers/char/mem.c
+++ mmotm-2.6.33-Feb01/drivers/char/mem.c
@@ -460,14 +460,18 @@ static ssize_t read_kmem(struct file *fi
 		}
 		free_page((unsigned long)kbuf);
 	}
+	/* EFAULT is always critical */
+	if (err == -EFAULT)
+		return err;
+	if (err == -ENXIO && !read)
+		return -ENXIO;
 	*ppos = p;
-	return read ? read : err;
+	return read;
 }
 
 
 static inline ssize_t
-do_write_kmem(unsigned long p, const char __user *buf,
-	      size_t count, loff_t *ppos)
+do_write_kmem(unsigned long p, const char __user *buf, size_t count)
 {
 	ssize_t written, sz;
 	unsigned long copied;
@@ -510,7 +514,6 @@ do_write_kmem(unsigned long p, const cha
 		written += sz;
 	}
 
-	*ppos += written;
 	return written;
 }
 
@@ -521,6 +524,7 @@ do_write_kmem(unsigned long p, const cha
 static ssize_t write_kmem(struct file * file, const char __user * buf, 
 			  size_t count, loff_t *ppos)
 {
+	/* Kernel virtual memory never exceeds unsigned long */
 	unsigned long p = *ppos;
 	ssize_t wrote = 0;
 	ssize_t virtr = 0;
@@ -530,7 +534,7 @@ static ssize_t write_kmem(struct file * 
 	if (p < (unsigned long) high_memory) {
 		unsigned long to_write = min_t(unsigned long, count,
 					       (unsigned long)high_memory - p);
-		wrote = do_write_kmem(p, buf, to_write, ppos);
+		wrote = do_write_kmem(p, buf, to_write);
 		if (wrote != to_write)
 			return wrote;
 		p += wrote;
@@ -540,8 +544,13 @@ static ssize_t write_kmem(struct file * 
 
 	if (count > 0) {
 		kbuf = (char *)__get_free_page(GFP_KERNEL);
-		if (!kbuf)
-			return wrote ? wrote : -ENOMEM;
+		if (!kbuf) {
+			if (wrote) { /* update ppos and return copied bytes */
+				*ppos = p;
+				return wrote;
+			} else
+				return -ENOMEM;
+		}
 		while (count > 0) {
 			unsigned long sz = size_inside_page(p, count);
 			unsigned long n;
@@ -563,9 +572,16 @@ static ssize_t write_kmem(struct file * 
 		}
 		free_page((unsigned long)kbuf);
 	}
-
+	/* EFAULT is always critical. */
+	if (err == -EFAULT)
+		return err;
+	if (err == -ENXIO) {
+		/* We reached the end of vmalloc area..check real bug or not*/
+		if (!(virtr + wrote)) /* nothing written */
+			return -ENXIO;
+	}
 	*ppos = p;
-	return virtr + wrote ? : err;
+	return virtr + wrote;
 }
 #endif
 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/