Re: [PATCH] fbdev: colormap fixes

[Geert Uytterhoeven]

On Wed, 27 Jul 2005, Linux Kernel Mailing List wrote:
> tree 17014af0ea8b19dae7848736d324499715b7a1a3
> parent 3ca34fcbfbf8a7cbe99d54ae81c4e28fdc6f4ac6
> author Jon Smirl <jonsmirl@xxxxxxxxx> Thu, 28 Jul 2005 01:46:05 -0700
> committer Linus Torvalds <torvalds@xxxxxxxxxxx> 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.

> 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 <jonsmirl@xxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxx>
> Signed-off-by: Linus Torvalds <torvalds@xxxxxxxx>
> 
>  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?

> +/* 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?

>  	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...

> +			((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?

>  }

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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
-
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/