Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
From: Dan Carpenter <hidden>
Date: 2020-07-15 15:12:48
Also in:
dri-devel, lkml
Subsystem:
framebuffer core, framebuffer layer, the rest · Maintainers:
Simona Vetter, Helge Deller, Linus Torvalds
On Wed, Jul 15, 2020 at 11:02:58PM +0900, Tetsuo Handa wrote:
quoted hunk ↗ jump to hunk
On 2020/07/15 20:17, Tetsuo Handa wrote:quoted
On 2020/07/15 18:48, Dan Carpenter wrote:quoted
quoted
@@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info, region.color = color; region.rop = ROP_COPY; - if (rw && !bottom_only) { + if ((int) rw > 0 && !bottom_only) { region.dx = info->var.xoffset + rs;^^^^^^^^^^^^^^^^^^^^^^ If you choose a very high positive "rw" then this addition can overflow. info->var.xoffset comes from the user and I don't think it's checked...Well, I think it would be checked by "struct fb_ops"->check_var hook. For example, vmw_fb_check_var() has if ((var->xoffset + var->xres) > par->max_width || (var->yoffset + var->yres) > par->max_height) { DRM_ERROR("Requested geom can not fit in framebuffer\n"); return -EINVAL; } check. Of course, there might be integer overflow in that check... Having sanity check at caller of "struct fb_ops"->check_var might be nice.Well, while const int fd = open("/dev/fb0", O_ACCMODE); struct fb_var_screeninfo var = { }; ioctl(fd, FBIOGET_VSCREENINFO, &var); var.xres = var.yres = 4; var.xoffset = 4294967292U; ioctl(fd, FBIOPUT_VSCREENINFO, &var); bypassed (var->xoffset + var->xres) > par->max_width check in vmw_fb_check_var(), ------------- a/drivers/video/fbdev/core/bitblit.c +++ b/drivers/video/fbdev/core/bitblit.c@@ -216,6 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info, region.color = color; region.rop = ROP_COPY; + printk(KERN_INFO "%s info->var.xoffset=%u rs=%u info->var.yoffset=%u bs=%u\n", __func__, info->var.xoffset, rs, info->var.yoffset, bs); if ((int) rw > 0 && !bottom_only) { region.dx = info->var.xoffset + rs; region.dy = 0; ----------says that info->var.xoffset does not come from the user. ---------- bit_clear_margins info->var.xoffset=0 rs24 info->var.yoffset=0 bs€0 ----------
In fb_set_var() we do:
drivers/video/fbdev/core/fbmem.c
1055 ret = info->fbops->fb_check_var(var, info);
1056
1057 if (ret)
1058 return ret;
1059
1060 if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
1061 return 0;
1062
1063 if (!basic_checks(var))
1064 return -EINVAL;
1065
1066 if (info->fbops->fb_get_caps) {
1067 ret = fb_check_caps(info, var, var->activate);
1068
1069 if (ret)
1070 return ret;
1071 }
1072
1073 old_var = info->var;
1074 info->var = *var;
^^^^^^^^^^^^^^^^
This should set "info->var.offset".
1075
1076 if (info->fbops->fb_set_par) {
1077 ret = info->fbops->fb_set_par(info);
1078
1079 if (ret) {
1080 info->var = old_var;
1081 printk(KERN_WARNING "detected "
I've complained about integer overflows in fbdev for a long time...
What I'd like to see is something like the following maybe. I don't
know how to get the vc_data in fbmem.c so it doesn't include your checks
for negative.
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index caf817bcb05c..5c74181fea5d 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c@@ -934,6 +934,54 @@ fb_pan_display(struct fb_info *info, struct fb_var_screeninfo *var) } EXPORT_SYMBOL(fb_pan_display); +static bool basic_checks(struct fb_var_screeninfo *var) +{ + unsigned int v_margins, h_margins; + + /* I think var->height and var->width = UINT_MAX means something. */ + + if (var->xres > INT_MAX || + var->yres > INT_MAX || + var->xres_virtual > INT_MAX || + var->yres_virtual > INT_MAX || + var->xoffset > INT_MAX || + var->yoffset > INT_MAX || + var->left_margin > INT_MAX || + var->right_margin > INT_MAX || + var->upper_margin > INT_MAX || + var->lower_margin > INT_MAX || + var->hsync_len > INT_MAX || + var->vsync_len > INT_MAX) + return false; + + if (var->bits_per_pixel > 128) + return false; + if (var->rotate > FB_ROTATE_CCW) + return false; + + if (var->xoffset > INT_MAX - var->xres) + return false; + if (var->yoffset > INT_MAX - var->yres) + return false; + + if (var->left_margin > INT_MAX - var->right_margin || + var->upper_margin > INT_MAX - var->lower_margin) + return false; + + v_margins = var->left_margin + var->right_margin; + h_margins = var->upper_margin + var->lower_margin; + + if (var->xres > INT_MAX - var->hsync_len || + var->yres > INT_MAX - var->vsync_len) + return false; + + if (v_margins > INT_MAX - var->hsync_len - var->xres || + h_margins > INT_MAX - var->vsync_len - var->yres) + return false; + + return true; +} + static int fb_check_caps(struct fb_info *info, struct fb_var_screeninfo *var, u32 activate) {
@@ -1012,6 +1060,9 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW) return 0; + if (!basic_checks(var)) + return -EINVAL; + if (info->fbops->fb_get_caps) { ret = fb_check_caps(info, var, var->activate);