fbcmap: non working tests on unsigned cmap->start

PATCH Roel Kluin 4 participants 8 messages Apr 21, 2009 – Apr 27, 2009 Framebuffer layer

lore

Share & tools

lore

Roel Kluin

author

2009-04-21 14:26 UTC #0 lore raw

vi include/linux/fb.h +478

Note that struct fb_cmap_user cmap->start in fb_set_user_cmap() is unsigned. Should there maybe be a test:

if (cmap->start > MAX || ...)

(and what should MAX be then?)

Otherwise you may want to apply the cleanup patch below

Roel ------------------------------>8-------------8<--------------------------------- Remove redundant tests on unsigned

Signed-off-by Roel Kluin <[email protected]>

---

drivers/video/fbcmap.c +1 -2

index f53b9f1..958bf4e 100644
@@ -266,8 +266,7 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
 		rc = -ENODEV;
 		goto out;
 	}
-	if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
-				!info->fbops->fb_setcmap)) {
+	if (!info->fbops->fb_setcolreg && !info->fbops->fb_setcmap) {
 		rc = -EINVAL;
 		goto out1;
 	}

Andrew Morton

2009-04-23 20:41 UTC #1 lore raw

Quoted 22 lines · On Tue, 21 Apr 2009 16:26:33 +0200 Roel Kluin <[email protected]> wrote:

vi include/linux/fb.h +478
Note that struct fb_cmap_user cmap->start in fb_set_user_cmap() is unsigned. Should there maybe be a test:
if (cmap->start > MAX || ...)
(and what should MAX be then?)
Otherwise you may want to apply the cleanup patch below
Roel ------------------------------>8-------------8<--------------------------------- Remove redundant tests on unsigned
Signed-off-by Roel Kluin <[email protected]>
---
drivers/video/fbcmap.c +0 -0
index f53b9f1..958bf4e 100644

drivers/video/fbcmap.c int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info) · line 272

Quoted 9 lines

drivers/video/fbcmap.c +1 -2

@@ -266,8 +266,7 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
 		rc = -ENODEV;
 		goto out;
 	}
-	if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
-				!info->fbops->fb_setcmap)) {
+	if (!info->fbops->fb_setcolreg && !info->fbops->fb_setcmap) {
 		rc = -EINVAL;
 		goto out1;
 	}

argh.

- Perhaps userspace can kill the kernel by sending a "negative" `start'. Removing the test will make it even less likely that we'll fix this bug.

- If this bug doesn't exist, and there _is_ userspace which is legitimately sending in "negative" values of `start' (unlikely?) then we will break userspace if we fix this comparison.

IOW, I don't have enough knowledge about this code to be able to know what to do.

Ville Syrjälä

2009-04-23 21:47 UTC #2 lore raw

Quoted 37 lines · Andrew Morton

Quoted 22 lines · On Tue, 21 Apr 2009 16:26:33 +0200 Roel Kluin <[email protected]> wrote:
vi include/linux/fb.h +478
Note that struct fb_cmap_user cmap->start in fb_set_user_cmap() is unsigned. Should there maybe be a test:
if (cmap->start > MAX || ...)
(and what should MAX be then?)
Otherwise you may want to apply the cleanup patch below
Roel ------------------------------>8-------------8<--------------------------------- Remove redundant tests on unsigned
Signed-off-by Roel Kluin <[email protected]>
---
drivers/video/fbcmap.c +0 -0
index f53b9f1..958bf4e 100644
drivers/video/fbcmap.c int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info) · line 272
Quoted 9 lines
drivers/video/fbcmap.c +1 -2
@@ -266,8 +266,7 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
 		rc = -ENODEV;
 		goto out;
 	}
-	if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
-				!info->fbops->fb_setcmap)) {
+	if (!info->fbops->fb_setcolreg && !info->fbops->fb_setcmap) {
 		rc = -EINVAL;
 		goto out1;
 	}
argh.

Quoted 3 lines

- Perhaps userspace can kill the kernel by sending a "negative" `start'. Removing the test will make it even less likely that we'll fix this bug.

Shouldn't happen. 'start' is used as the starting index for the hardware palette, 'start+len-1' is the last index. All drivers should already check the passed values since the maximum index depends on the display mode. And I suppose the worst thing that could happen if the driver fails to check the values would be incorrect colors.

Quoted 3 lines

- If this bug doesn't exist, and there _is_ userspace which is legitimately sending in "negative" values of `start' (unlikely?) then we will break userspace if we fix this comparison.

The comparison will always be false since 'start' is unsigned so how would anything break?

Signature · 3 lines

Ville Syrjälä
[email protected]
http://www.sci.fi/~syrjala/

Andrew Morton

2009-04-23 21:56 UTC #3 lore raw

Quoted 42 lines · On Fri, 24 Apr 2009 00:47:38 +0300 Ville Syrj__l__ <[email protected]> wrote:

Quoted 37 lines · Andrew Morton
Quoted 22 lines · On Tue, 21 Apr 2009 16:26:33 +0200 Roel Kluin <[email protected]> wrote:
vi include/linux/fb.h +478
Note that struct fb_cmap_user cmap->start in fb_set_user_cmap() is unsigned. Should there maybe be a test:
if (cmap->start > MAX || ...)
(and what should MAX be then?)
Otherwise you may want to apply the cleanup patch below
Roel ------------------------------>8-------------8<--------------------------------- Remove redundant tests on unsigned
Signed-off-by Roel Kluin <[email protected]>
---
drivers/video/fbcmap.c +0 -0
index f53b9f1..958bf4e 100644
drivers/video/fbcmap.c int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info) · line 272
Quoted 9 lines
drivers/video/fbcmap.c +1 -2
@@ -266,8 +266,7 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
 		rc = -ENODEV;
 		goto out;
 	}
-	if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
-				!info->fbops->fb_setcmap)) {
+	if (!info->fbops->fb_setcolreg && !info->fbops->fb_setcmap) {
 		rc = -EINVAL;
 		goto out1;
 	}
argh.
Quoted 3 lines
- Perhaps userspace can kill the kernel by sending a "negative" `start'. Removing the test will make it even less likely that we'll fix this bug.

Quoted 5 lines

Shouldn't happen. 'start' is used as the starting index for the hardware palette, 'start+len-1' is the last index. All drivers should already check the passed values since the maximum index depends on the display mode. And I suppose the worst thing that could happen if the driver fails to check the values would be incorrect colors.

OK.

I wonder if all drivers _do_ check. It probably doesn't matter much, as it's a privileged operation (I assume?)

Quoted 6 lines

Quoted 3 lines
- If this bug doesn't exist, and there _is_ userspace which is legitimately sending in "negative" values of `start' (unlikely?) then we will break userspace if we fix this comparison.
The comparison will always be false since 'start' is unsigned so how would anything break?

By "fix this comparison" I meant converting it to

	if ((signed)cmap->start < 0)

Roel Kluin

author

2009-04-26 12:20 UTC #4 lore raw

In reply to Ville Syrjälä

cmap->start is unsigned, A negative start could result in incorrect colors. `start' is used as the starting index for the hardware palette, 'start+len-1' is the last index.

Signed-off-by Roel Kluin <[email protected]>

---

Quoted 17 lines

Quoted 9 lines
Quoted 9 lines
vi include/linux/fb.h +478
Note that struct fb_cmap_user cmap->start in fb_set_user_cmap() is unsigned. Should there maybe be a test:
if (cmap->start > MAX || ...)
(and what should MAX be then?)
Quoted 5 lines · Andrew Morton
argh.
- Perhaps userspace can kill the kernel by sending a "negative" `start'. Removing the test will make it even less likely that we'll fix this bug.

Quoted 5 lines

Shouldn't happen. 'start' is used as the starting index for the hardware palette, 'start+len-1' is the last index. All drivers should already check the passed values since the maximum index depends on the display mode. And I suppose the worst thing that could happen if the driver fails to check the values would be incorrect colors.

Thanks for your explanation, I think this should fix it?

drivers/video/fbcmap.c +1 -1

index f53b9f1..ea62d87 100644
@@ -266,7 +266,7 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
 		rc = -ENODEV;
 		goto out;
 	}
-	if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
+	if (cmap->start >= cmap->len || (!info->fbops->fb_setcolreg &&
 				!info->fbops->fb_setcmap)) {
 		rc = -EINVAL;
 		goto out1;

Ville Syrjälä

2009-04-26 13:15 UTC #5 lore raw

Quoted 36 lines · Roel Kluin

cmap->start is unsigned, A negative start could result in incorrect colors. `start' is used as the starting index for the hardware palette, 'start+len-1' is the last index.
Signed-off-by Roel Kluin <[email protected]>
---
Quoted 17 lines
Quoted 9 lines
Quoted 9 lines
vi include/linux/fb.h +478
Note that struct fb_cmap_user cmap->start in fb_set_user_cmap() is unsigned. Should there maybe be a test:
if (cmap->start > MAX || ...)
(and what should MAX be then?)
Quoted 5 lines · Andrew Morton
argh.
- Perhaps userspace can kill the kernel by sending a "negative" `start'. Removing the test will make it even less likely that we'll fix this bug.
Quoted 5 lines
Shouldn't happen. 'start' is used as the starting index for the hardware palette, 'start+len-1' is the last index. All drivers should already check the passed values since the maximum index depends on the display mode. And I suppose the worst thing that could happen if the driver fails to check the values would be incorrect colors.
Thanks for your explanation, I think this should fix it?
drivers/video/fbcmap.c +0 -0
index f53b9f1..ea62d87 100644

drivers/video/fbcmap.c int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info) · line 272

Quoted 8 lines

drivers/video/fbcmap.c +1 -1

@@ -266,7 +266,7 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
 		rc = -ENODEV;
 		goto out;
 	}
-	if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
+	if (cmap->start >= cmap->len || (!info->fbops->fb_setcolreg &&
 				!info->fbops->fb_setcmap)) {
 		rc = -EINVAL;
 		goto out1;

That's not correct. There's nothing wrong with having 'start' >= 'len'.

You would rather need something like
'if (start+len > 1 << max(red.len, green.len, blue.len, transp.len))' and a check to make sure that start+len doesn't overflow.

Oh and I guess it should also check that the visual is pseudocolor or directcolor.

Signature · 10 lines

Ville Syrjälä
[email protected]
http://www.sci.fi/~syrjala/

------------------------------------------------------------------------------
Crystal Reports &#45; New Free Runtime and 30 Day Trial
Check out the new simplified licensign option that enables unlimited
royalty&#45;free distribution of the report engine for externally facing 
server and web deployment.
http://p.sf.net/sfu/businessobjects

Roel Kluin

author

2009-04-27 11:58 UTC #6 lore raw

Quoted 9 lines · Ville Syrjälä

Quoted 8 lines · Roel Kluin
cmap->start is unsigned,
Quoted 6 lines · Andrew Morton
Quoted 5 lines
argh.
- Perhaps userspace can kill the kernel by sending a "negative" `start'. Removing the test will make it even less likely that we'll fix this bug.

Quoted 8 lines

Quoted 5 lines
Quoted 5 lines
Shouldn't happen. 'start' is used as the starting index for the hardware palette, 'start+len-1' is the last index. All drivers should already check the passed values since the maximum index depends on the display mode. And I suppose the worst thing that could happen if the driver fails to check the values would be incorrect colors.
You would rather need something like
'if (start+len > 1 << max(red.len, green.len, blue.len, transp.len))'

what do you mean with red.len'? is that info->var.red.length'?

Quoted 4 lines

and a check to make sure that start+len doesn't overflow.
Oh and I guess it should also check that the visual is pseudocolor or directcolor.

I am fairly new so please review carefully.

Not yet signed off-by: Roel Kluin <[email protected]>
---

drivers/video/fbcmap.c +17 -2

index f53b9f1..b34e74e 100644
@@ -249,6 +249,7 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
 {
 	int rc, size = cmap->len * sizeof(u16);
 	struct fb_cmap umap;
+	__u32 rgba_max = 0;
 
 	memset(&umap, 0, sizeof(struct fb_cmap));
 	rc = fb_alloc_cmap(&umap, cmap->len, cmap->transp != NULL);

@@ -261,13 +262,27 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
 		rc = -EFAULT;
 		goto out;
 	}
+
+	if (cmap->start + cmap->len < cmap->start) {
+		rc = -EINVAL;
+		goto out;
+	}
+
 	umap.start = cmap->start;
 	if (!lock_fb_info(info)) {
 		rc = -ENODEV;
 		goto out;
 	}
-	if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
-				!info->fbops->fb_setcmap)) {
+
+	rgba_max = max(info->var.red.length, info->var.green.length);
+	rgba_max = max(rgba_max, info->var.blue.length);
+	rgba_max = max(rgba_max, info->var.transp.length);
+
+	if (cmap->start + cmap->len > 1 << rgba_max ||
+			!(info->fbops->fb_setcolreg ||
+				info->fbops->fb_setcmap) ||
+			!(info->fix.visual == FB_VISUAL_PSEUDOCOLOR ||
+				info->fix.visual == FB_VISUAL_TRUECOLOR)) {
 		rc = -EINVAL;
 		goto out1;
 	}

------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial Check out the new simplified licensign option that enables unlimited royalty-free distribution of the report engine for externally facing server and web deployment.
http://p.sf.net/sfu/businessobjects

Geert Uytterhoeven

2009-04-27 12:21 UTC #7 lore raw

Quoted 16 lines · Roel Kluin

Quoted 14 lines · Ville Syrjälä
Quoted 13 lines · Roel Kluin
cmap->start is unsigned,
Quoted 11 lines · Andrew Morton
Quoted 5 lines
argh.
- Perhaps userspace can kill the kernel by sending a "negative" `start'. Removing the test will make it even less likely that we'll fix this bug.
Shouldn't happen. 'start' is used as the starting index for the hardware palette, 'start+len-1' is the last index. All drivers should already check the passed values since the maximum index depends on the display mode. And I suppose the worst thing that could happen if the driver fails to check the values would be incorrect colors.

Quoted 4 lines

Quoted 2 lines
You would rather need something like
'if (start+len > 1 << max(red.len, green.len, blue.len, transp.len))'
what do you mean with red.len'? is that info->var.red.length'?

That's correct.

Quoted 8 lines

Quoted 4 lines
and a check to make sure that start+len doesn't overflow.
Oh and I guess it should also check that the visual is pseudocolor or directcolor.
I am fairly new so please review carefully.
Not yet signed off-by: Roel Kluin <[email protected]>

Looks OK, thx!

Quoted 55 lines

---

drivers/video/fbcmap.c +17 -3

index f53b9f1..b34e74e 100644
@@ -249,6 +249,7 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
  {
        int rc, size = cmap->len * sizeof(u16);
        struct fb_cmap umap;
+       __u32 rgba_max = 0;
 
        memset(&umap, 0, sizeof(struct fb_cmap));
        rc = fb_alloc_cmap(&umap, cmap->len, cmap->transp != NULL);

@@ -261,13 +262,27 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
                rc = -EFAULT;
                goto out;
        }
+
+       if (cmap->start + cmap->len < cmap->start) {
+               rc = -EINVAL;
+               goto out;
+       }
+
        umap.start = cmap->start;
        if (!lock_fb_info(info)) {
                rc = -ENODEV;
                goto out;
        }
-       if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
-                               !info->fbops->fb_setcmap)) {
+
+       rgba_max = max(info->var.red.length, info->var.green.length);
+       rgba_max = max(rgba_max, info->var.blue.length);
+       rgba_max = max(rgba_max, info->var.transp.length);
+
+       if (cmap->start + cmap->len > 1 << rgba_max ||
+                       !(info->fbops->fb_setcolreg ||
+                               info->fbops->fb_setcmap) ||
+                       !(info->fix.visual == FB_VISUAL_PSEUDOCOLOR ||
+                               info->fix.visual == FB_VISUAL_TRUECOLOR)) {
                rc = -EINVAL;
                goto out1;
        }
 
------------------------------------------------------------------------------

Crystal Reports - New Free Runtime and 30 Day Trial Check out the new simplified licensign option that enables unlimited royalty-free distribution of the report engine for externally facing server and web deployment.
http://p.sf.net/sfu/businessobjects
_______________________________________________
Linux-fbdev-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Signature · 10 lines

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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