[PATCH][FBDEV] Fix copy_to/from_user in fbmem.c:fb_read/write

Hi,

This patch fixes a prblem reported by David S. Miller [off-list ref]

"I just noticed that fb_{read,write}() uses copy_*_user() with
the kernel buffer being the frame buffer.  It needs to use
the proper device address accessor functions."

Tony

Signed-off-by: Antonino Daplas <redacted>
---

diff -uprN linux-2.6.9-rc1-mm1-orig/drivers/video/fbmem.c linux-2.6.9-rc1-mm1/drivers/video/fbmem.c

--- linux-2.6.9-rc1-mm1-orig/drivers/video/fbmem.c	2004-08-27 05:24:32.000000000 +0800
+++ linux-2.6.9-rc1-mm1/drivers/video/fbmem.c	2004-08-27 05:25:02.200341392 +0800

@@ -878,6 +878,8 @@ fb_read(struct file *file, char __user *
 	struct inode *inode = file->f_dentry->d_inode;
 	int fbidx = iminor(inode);
 	struct fb_info *info = registered_fb[fbidx];
+	u32 *buffer, *dst, *src;
+	int c, i, cnt = 0, err = 0;
 
 	if (!info || ! info->screen_base)
 		return -ENODEV;

@@ -894,18 +896,40 @@ fb_read(struct file *file, char __user *
 	    count = info->fix.smem_len;
 	if (count + p > info->fix.smem_len)
 		count = info->fix.smem_len - p;
+
+	cnt = 0;
+	buffer = kmalloc((count > 64 * 1024) ? 64 * 1024 : count,
+			 GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	src = (u32 *) (info->screen_base + p);
+
 	if (info->fbops->fb_sync)
 		info->fbops->fb_sync(info);
-	if (count) {
-	    char *base_addr;
-
-	    base_addr = info->screen_base;
-	    count -= copy_to_user(buf, base_addr+p, count);
-	    if (!count)
-		return -EFAULT;
-	    *ppos += count;
+	
+	while (count) {
+		c  = (count > 64 * 1024) ? 64 * 1024 : count;
+		dst = buffer;
+		for (i = c >> 2; i--; ) 
+			*dst++ = fb_readl(src++);
+		if (c & 3) {
+			for (i = c & 3; i--;) 
+				*((u8 *)dst++) = fb_readb((u8 *) src++);
+		}
+		
+		if (copy_to_user(buf, buffer, c)) {
+			err = -EFAULT;
+			break;
+		}
+		*ppos += c;
+		buf += c;
+		cnt += c;
+		count -= c;
 	}
-	return count;
+
+	kfree(buffer);
+	return (err) ? err : cnt;
 }
 
 static ssize_t

@@ -915,7 +939,8 @@ fb_write(struct file *file, const char _
 	struct inode *inode = file->f_dentry->d_inode;
 	int fbidx = iminor(inode);
 	struct fb_info *info = registered_fb[fbidx];
-	int err;
+	u32 *buffer, *dst, *src;
+	int c, i, cnt = 0, err;
 
 	if (!info || !info->screen_base)
 		return -ENODEV;

@@ -935,19 +960,38 @@ fb_write(struct file *file, const char _
 	    count = info->fix.smem_len - p;
 	    err = -ENOSPC;
 	}
+	cnt = 0;
+	buffer = kmalloc((count > 64 * 1024) ? 64 * 1024 : count,
+			 GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+	
+	dst = (u32 *) (info->screen_base + p);
+
 	if (info->fbops->fb_sync)
 		info->fbops->fb_sync(info);
-	if (count) {
-	    char *base_addr;
 
-	    base_addr = info->screen_base;
-	    count -= copy_from_user(base_addr+p, buf, count);
-	    *ppos += count;
-	    err = -EFAULT;
+	while (count) {
+		c = (count > 64 * 1024) ? 64 * 1024 : count;
+		src = buffer;
+		if (copy_from_user(src, buf, c)) {
+			err = -EFAULT;
+			break;
+		}
+		for (i = c >> 2; i--; )
+			fb_writel(*src++, dst++);
+		if (c & 3) {
+			for (i = c & 3; i--; )
+				fb_writeb(*(u8 *)src++, (u8 *)dst++);
+		}
+		*ppos += c;
+		buf += c;
+		cnt += c;
+		count -= c;
 	}
-	if (count)
-		return count;
-	return err;
+	kfree(buffer);
+
+	return (err) ? err : cnt;
 }
 
 #ifdef CONFIG_KMOD






-------------------------------------------------------

This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=5047&alloc_id=10808&op=click

On Mon, 30 Aug 2004 10:47:28 +0800
"Antonino A. Daplas" [off-list ref] wrote:

This patch fixes a prblem reported by David S. Miller [off-list ref]

"I just noticed that fb_{read,write}() uses copy_*_user() with
the kernel buffer being the frame buffer.  It needs to use
the proper device address accessor functions."

Looks great Tony, thanks for fixing this.


-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=5047&alloc_id=10808&op=click

On Mon, 30 Aug 2004, Antonino A. Daplas wrote:

                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

+			 GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;

Woops, what's the probability of a 64 kiB kmalloc() to fail?

    [...]

+	while (count) {
+		c  = (count > 64 * 1024) ? 64 * 1024 : count;
+		dst = buffer;
+		for (i = c >> 2; i--; )
+			*dst++ = fb_readl(src++);
+		if (c & 3) {
+			for (i = c & 3; i--;)
+				*((u8 *)dst++) = fb_readb((u8 *) src++);
+		}
+
+		if (copy_to_user(buf, buffer, c)) {
+			err = -EFAULT;
+			break;
+		}
+		*ppos += c;
+		buf += c;
+		cnt += c;
+		count -= c;
 	}

I don't expect the penalty for always using a buffer of PAGE_SIZE to be very
large. Or am I missing something?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=5047&alloc_id=10808&op=click

Geert Uytterhoeven [off-list ref] wrote:

quoted

+

 > +	cnt = 0;
 > +	buffer = kmalloc((count > 64 * 1024) ? 64 * 1024 : count,
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 > +			 GFP_KERNEL);
 > +	if (!buffer)
 > +		return -ENOMEM;

 Woops, what's the probability of a 64 kiB kmalloc() to fail?

Relatively high.

     [...]

 > +	while (count) {
 > +		c  = (count > 64 * 1024) ? 64 * 1024 : count;
 > +		dst = buffer;
 > +		for (i = c >> 2; i--; )
 > +			*dst++ = fb_readl(src++);
 > +		if (c & 3) {
 > +			for (i = c & 3; i--;)
 > +				*((u8 *)dst++) = fb_readb((u8 *) src++);
 > +		}
 > +
 > +		if (copy_to_user(buf, buffer, c)) {
 > +			err = -EFAULT;
 > +			break;
 > +		}
 > +		*ppos += c;
 > +		buf += c;
 > +		cnt += c;
 > +		count -= c;
 >  	}

 I don't expect the penalty for always using a buffer of PAGE_SIZE to be very
 large. Or am I missing something?

The 64k copy buffer will most likely be slower than using an 8k one.  it
depends, of course, upon the vintage of the CPU.

4k or 8k should be fine here.


-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=5047&alloc_id=10808&op=click

On Monday 30 August 2004 17:10, Geert Uytterhoeven wrote:

I don't expect the penalty for always using a buffer of PAGE_SIZE to be
very large. Or am I missing something?

You're absolutely correct, there is no significant performance penalty whether
using a small or large-sized buffer.

Will resubmit a patch.

Tony




-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=5047&alloc_id=10808&op=click