Re: [PATCH] fbdev: colormap fixes

On 7/28/05, Geert Uytterhoeven [off-list ref] wrote:

On Wed, 27 Jul 2005, Linux Kernel Mailing List wrote:

There are a couple of ways to fix this. 

1) Add a check to limit use of the sysfs attributes to 256 entries. If
you want more you have to use /dev/fb0 and the ioctl. More is an
uncommon case.
2) Switch this to a binary parameter. Now you have to use tools like
hexdump instead of cat to work with the data. It was nice to be able
to use cat to see the current map.

Does anyone have preferences for which way to fix it?

Thanks for catching the problems. I'm posting these patches to fbdev
for review first so it is best to catch bugs there.

quoted

tree 17014af0ea8b19dae7848736d324499715b7a1a3
parent 3ca34fcbfbf8a7cbe99d54ae81c4e28fdc6f4ac6
author Jon Smirl [off-list ref] Thu, 28 Jul 2005 01:46:05 -0700
committer Linus Torvalds [off-list ref] Thu, 28 Jul 2005 06:26:18 -0700

[PATCH] fbdev: colormap fixes

Color maps have up to 256 entries.  4096/256 allows for 16 characters per

             ^^^^^^^^^^^^^^^^^^^^^^
Most (all?) current drivers have this limitation. But there exists hardware
with more entries.

quoted

line.  The format for a cmap entry is "%02x%c%4x%4x%4x\n" %02x entry %c
transp %4x red %4x blue %4x green

You can read the color_map with cat fb0/color_map.

Signed-off-by: Jon Smirl <redacted>
Signed-off-by: Andrew Morton <redacted>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

 drivers/video/fbsysfs.c |   82 ++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 72 insertions(+), 10 deletions(-)

diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
--- a/drivers/video/fbsysfs.c
+++ b/drivers/video/fbsysfs.c

@@ -242,10 +242,68 @@ static ssize_t show_virtual(struct class
                      fb_info->var.yres_virtual);
 }

-static ssize_t store_cmap(struct class_device *class_device, const char * buf,
+/* Format for cmap is "%02x%c%4x%4x%4x\n" */
+/* %02x entry %c transp %4x red %4x blue %4x green \n */
+/* 255 rows at 16 chars equals 4096 */

      ^^^
256?

quoted

+/* PAGE_SIZE can be 4096 or larger */
+static ssize_t store_cmap(struct class_device *class_device, const char *buf,
                        size_t count)
 {
-//   struct fb_info *fb_info = (struct fb_info *)class_get_devdata(class_device);
+     struct fb_info *fb_info = (struct fb_info *)class_get_devdata(class_device);
+     int rc, i, start, length, transp = 0;
+
+     if ((count > 4096) || ((count % 16) != 0) || (PAGE_SIZE < 4096))
+             return -EINVAL;
+
+     if (!fb_info->fbops->fb_setcolreg && !fb_info->fbops->fb_setcmap)
+             return -EINVAL;
+
+     sscanf(buf, "%02x", &start);
+     length = count / 16;
+
+     for (i = 0; i < length; i++)
+             if (buf[i * 16 + 2] != ' ')
+                     transp = 1;
+
+     /* If we can batch, do it */
+     if (fb_info->fbops->fb_setcmap && length > 1) {
+             struct fb_cmap umap;
+
+             memset(&umap, 0, sizeof(umap));
+             if ((rc = fb_alloc_cmap(&umap, length, transp)))
+                     return rc;
+
+             umap.start = start;
+             for (i = 0; i < length; i++) {
+                     sscanf(&buf[i * 16 +  3], "%4hx", &umap.red[i]);
+                     sscanf(&buf[i * 16 +  7], "%4hx", &umap.blue[i]);
+                     sscanf(&buf[i * 16 + 11], "%4hx", &umap.green[i]);
+                     if (transp)
+                             umap.transp[i] = (buf[i * 16 +  2] != ' ');
+             }
+             rc = fb_info->fbops->fb_setcmap(&umap, fb_info);
+             fb_copy_cmap(&umap, &fb_info->cmap);
+             fb_dealloc_cmap(&umap);
+
+             return rc;
+     }
+     for (i = 0; i < length; i++) {
+             u16 red, blue, green, tsp;
+
+             sscanf(&buf[i * 16 +  3], "%4hx", &red);
+             sscanf(&buf[i * 16 +  7], "%4hx", &blue);
+             sscanf(&buf[i * 16 + 11], "%4hx", &green);
+             tsp = (buf[i * 16 +  2] != ' ');
+             if ((rc = fb_info->fbops->fb_setcolreg(start++,
+                                   red, green, blue, tsp, fb_info)))
+                     return rc;
+
+             fb_info->cmap.red[i] = red;
+             fb_info->cmap.blue[i] = blue;
+             fb_info->cmap.green[i] = green;
+             if (transp)
+                     fb_info->cmap.transp[i] = tsp;
+     }
      return 0;
 }

@@ -253,20 +311,24 @@ static ssize_t show_cmap(struct class_de
 {
      struct fb_info *fb_info =
              (struct fb_info *)class_get_devdata(class_device);
-     unsigned int offset = 0, i;
+     unsigned int i;

      if (!fb_info->cmap.red || !fb_info->cmap.blue ||
-         !fb_info->cmap.green || !fb_info->cmap.transp)
+        !fb_info->cmap.green)
+             return -EINVAL;
+
+     if (PAGE_SIZE < 4096)
              return -EINVAL;

+     /* don't mess with the format, the buffer is PAGE_SIZE */
+     /* 255 entries at 16 chars per line equals 4096 = PAGE_SIZE */

           ^^^
256?

quoted

      for (i = 0; i < fb_info->cmap.len; i++) {
-             offset += snprintf(buf, PAGE_SIZE - offset,
-                                "%d,%d,%d,%d,%d\n", i + fb_info->cmap.start,
-                                fb_info->cmap.red[i], fb_info->cmap.blue[i],
-                                fb_info->cmap.green[i],
-                                fb_info->cmap.transp[i]);
+             sprintf(&buf[ i * 16], "%02x%c%4x%4x%4x\n", i + fb_info->cmap.start,

Woops, nice buffer overflow if fb_info->cmap.len > 256...

quoted

+                     ((fb_info->cmap.transp && fb_info->cmap.transp[i]) ? '*' : ' '),
+                     fb_info->cmap.red[i], fb_info->cmap.blue[i],
+                     fb_info->cmap.green[i]);
      }
-     return offset;
+     return 4096;

               ^^^^

What if fb_info->cmap.len is smaller than 256?

quoted

 }

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

-- 
Jon Smirl
jonsmirl@gmail.com


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf