| Message ID | 20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins. | expand |
On Wed, Jul 15, 2020 at 10:51:02AM +0900, Tetsuo Handa wrote: > syzbot is reporting general protection fault in bitfill_aligned() [1] > caused by integer underflow in bit_clear_margins(). The cause of this > problem is when and how do_vc_resize() updates vc->vc_{cols,rows}. > > If vc_do_resize() fails (e.g. kzalloc() fails) when var.xres or var.yres > is going to shrink, vc->vc_{cols,rows} will not be updated. This allows > bit_clear_margins() to see info->var.xres < (vc->vc_cols * cw) or > info->var.yres < (vc->vc_rows * ch). Unexpectedly large rw or bh will > try to overrun the __iomem region and causes general protection fault. > > Also, vc_resize(vc, 0, 0) does not set vc->vc_{cols,rows} = 0 due to > > new_cols = (cols ? cols : vc->vc_cols); > new_rows = (lines ? lines : vc->vc_rows); > > exception. Since cols and lines are calculated as > > cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres); > rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres); > cols /= vc->vc_font.width; > rows /= vc->vc_font.height; > vc_resize(vc, cols, rows); > > in fbcon_modechanged(), var.xres < vc->vc_font.width makes cols = 0 > and var.yres < vc->vc_font.height makes rows = 0. This means that > > const int fd = open("/dev/fb0", O_ACCMODE); > struct fb_var_screeninfo var = { }; > ioctl(fd, FBIOGET_VSCREENINFO, &var); > var.xres = var.yres = 1; > ioctl(fd, FBIOPUT_VSCREENINFO, &var); > > easily reproduces integer underflow bug explained above. > > Of course, callers of vc_resize() are not handling vc_do_resize() failure > is bad. But we can't avoid vc_resize(vc, 0, 0) which returns 0. Therefore, > as a band-aid workaround, this patch checks integer underflow in > "struct fbcon_ops"->clear_margins call, assuming that > vc->vc_cols * vc->vc_font.width and vc->vc_rows * vc->vc_font.heigh do not > cause integer overflow. > > [1] https://syzkaller.appspot.com/bug?id=a565882df74fa76f10d3a6fec4be31098dbb37c6 > > Reported-and-tested-by: syzbot <syzbot+e5fd3e65515b48c02a30@syzkaller.appspotmail.com> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > --- > drivers/video/fbdev/core/bitblit.c | 4 ++-- > drivers/video/fbdev/core/fbcon_ccw.c | 4 ++-- > drivers/video/fbdev/core/fbcon_cw.c | 4 ++-- > drivers/video/fbdev/core/fbcon_ud.c | 4 ++-- > 4 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c > index ca935c09a261..35ebeeccde4d 100644 > --- a/drivers/video/fbdev/core/bitblit.c > +++ b/drivers/video/fbdev/core/bitblit.c > @@ -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... regards, dan carpenter
On 2020/07/15 18:48, Dan Carpenter wrote: >> @@ -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.
On 2020/07/15 20:17, Tetsuo Handa wrote: > On 2020/07/15 18:48, Dan Carpenter wrote: >>> @@ -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 rs=1024 info->var.yoffset=0 bs=800 ----------
On Wed, Jul 15, 2020 at 11:02:58PM +0900, Tetsuo Handa wrote: > On 2020/07/15 20:17, Tetsuo Handa wrote: > > On 2020/07/15 18:48, Dan Carpenter wrote: > >>> @@ -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 rs=1024 info->var.yoffset=0 bs=800 > ---------- 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);
On 2020/07/16 0:12, Dan Carpenter wrote: > 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. Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ , we want basic checks. That's a task for fbdev people who should be familiar with necessary constraints. Anyway, my two patches are small and low cost; can we apply these patches regardless of basic checks?
On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote: > On 2020/07/16 0:12, Dan Carpenter wrote: > > 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. > > Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at > https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ , > we want basic checks. That's a task for fbdev people who should be familiar with > necessary constraints. I think the worldwide supply of people who understand fbdev and willing to work on it is roughly 0. So if someone wants to fix this mess properly (which likely means adding tons of over/underflow checks at entry points, since you're never going to catch the driver bugs, there's too many and not enough people who care) they need to fix this themselves. Just to avoid confusion here. > Anyway, my two patches are small and low cost; can we apply these patches regardless > of basic checks? Which two patches where? Cheers, Daniel
On 2020/07/16 19:00, Daniel Vetter wrote: > On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote: >> On 2020/07/16 0:12, Dan Carpenter wrote: >>> 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. >> >> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at >> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ , >> we want basic checks. That's a task for fbdev people who should be familiar with >> necessary constraints. > > I think the worldwide supply of people who understand fbdev and willing to > work on it is roughly 0. So if someone wants to fix this mess properly > (which likely means adding tons of over/underflow checks at entry points, > since you're never going to catch the driver bugs, there's too many and > not enough people who care) they need to fix this themselves. But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks() (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only rows and cols < 32768 ? > > Just to avoid confusion here. > >> Anyway, my two patches are small and low cost; can we apply these patches regardless >> of basic checks? > > Which two patches where? [PATCH v3] vt: Reject zero-sized screen buffer size. from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins. from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp
On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote: > On 2020/07/16 19:00, Daniel Vetter wrote: > > On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote: > >> On 2020/07/16 0:12, Dan Carpenter wrote: > >>> 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. > >> > >> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at > >> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ , > >> we want basic checks. That's a task for fbdev people who should be familiar with > >> necessary constraints. > > > > I think the worldwide supply of people who understand fbdev and willing to > > work on it is roughly 0. So if someone wants to fix this mess properly > > (which likely means adding tons of over/underflow checks at entry points, > > since you're never going to catch the driver bugs, there's too many and > > not enough people who care) they need to fix this themselves. > > But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks() > (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for > "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only > rows and cols < 32768 ? > > > > > Just to avoid confusion here. > > > >> Anyway, my two patches are small and low cost; can we apply these patches regardless > >> of basic checks? > > > > Which two patches where? > > [PATCH v3] vt: Reject zero-sized screen buffer size. > from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp This is now in my tree. > [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins. > from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp That should be taken by the fbdev maintainer, but I can take it too if people want. thanks, greg k-h
On Tue, Jul 21, 2020 at 6:08 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote: > > On 2020/07/16 19:00, Daniel Vetter wrote: > > > On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote: > > >> On 2020/07/16 0:12, Dan Carpenter wrote: > > >>> 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. > > >> > > >> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at > > >> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ , > > >> we want basic checks. That's a task for fbdev people who should be familiar with > > >> necessary constraints. > > > > > > I think the worldwide supply of people who understand fbdev and willing to > > > work on it is roughly 0. So if someone wants to fix this mess properly > > > (which likely means adding tons of over/underflow checks at entry points, > > > since you're never going to catch the driver bugs, there's too many and > > > not enough people who care) they need to fix this themselves. > > > > But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks() > > (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for > > "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only > > rows and cols < 32768 ? > > > > > > > > Just to avoid confusion here. > > > > > >> Anyway, my two patches are small and low cost; can we apply these patches regardless > > >> of basic checks? > > > > > > Which two patches where? > > > > [PATCH v3] vt: Reject zero-sized screen buffer size. > > from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp > > This is now in my tree. > > > [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins. > > from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp > > That should be taken by the fbdev maintainer, but I can take it too if > people want. Just missed this weeks pull request train and feeling like not worth making this an exception (it's been broken forever after all), so maybe best if you just add this to vt. Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> Also this avoids the impression I know what's going on in fbdev code, maybe with sufficient abandon from my side someone will pop up who cares an fixes the bazillion of syzkaller issues we seem to have around console/vt and everything related. -Daniel
On Wed, Jul 22, 2020 at 10:07:06AM +0200, Daniel Vetter wrote: > On Tue, Jul 21, 2020 at 6:08 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote: > > > On 2020/07/16 19:00, Daniel Vetter wrote: > > > > On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote: > > > >> On 2020/07/16 0:12, Dan Carpenter wrote: > > > >>> 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. > > > >> > > > >> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at > > > >> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ , > > > >> we want basic checks. That's a task for fbdev people who should be familiar with > > > >> necessary constraints. > > > > > > > > I think the worldwide supply of people who understand fbdev and willing to > > > > work on it is roughly 0. So if someone wants to fix this mess properly > > > > (which likely means adding tons of over/underflow checks at entry points, > > > > since you're never going to catch the driver bugs, there's too many and > > > > not enough people who care) they need to fix this themselves. > > > > > > But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks() > > > (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for > > > "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only > > > rows and cols < 32768 ? > > > > > > > > > > > Just to avoid confusion here. > > > > > > > >> Anyway, my two patches are small and low cost; can we apply these patches regardless > > > >> of basic checks? > > > > > > > > Which two patches where? > > > > > > [PATCH v3] vt: Reject zero-sized screen buffer size. > > > from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp > > > > This is now in my tree. > > > > > [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins. > > > from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp > > > > That should be taken by the fbdev maintainer, but I can take it too if > > people want. > > Just missed this weeks pull request train and feeling like not worth > making this an exception (it's been broken forever after all), so > maybe best if you just add this to vt. > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Also this avoids the impression I know what's going on in fbdev code, > maybe with sufficient abandon from my side someone will pop up who > cares an fixes the bazillion of syzkaller issues we seem to have > around console/vt and everything related. Great, will go queue it up now, thanks! greg k-h
On 7/23/20 4:21 PM, Greg Kroah-Hartman wrote: > On Wed, Jul 22, 2020 at 10:07:06AM +0200, Daniel Vetter wrote: >> On Tue, Jul 21, 2020 at 6:08 PM Greg Kroah-Hartman >> <gregkh@linuxfoundation.org> wrote: >>> >>> On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote: >>>> On 2020/07/16 19:00, Daniel Vetter wrote: >>>>> On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote: >>>>>> On 2020/07/16 0:12, Dan Carpenter wrote: >>>>>>> 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. >>>>>> >>>>>> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at >>>>>> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ , >>>>>> we want basic checks. That's a task for fbdev people who should be familiar with >>>>>> necessary constraints. >>>>> >>>>> I think the worldwide supply of people who understand fbdev and willing to >>>>> work on it is roughly 0. So if someone wants to fix this mess properly >>>>> (which likely means adding tons of over/underflow checks at entry points, >>>>> since you're never going to catch the driver bugs, there's too many and >>>>> not enough people who care) they need to fix this themselves. >>>> >>>> But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks() >>>> (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for >>>> "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only >>>> rows and cols < 32768 ? >>>> >>>>> >>>>> Just to avoid confusion here. >>>>> >>>>>> Anyway, my two patches are small and low cost; can we apply these patches regardless >>>>>> of basic checks? >>>>> >>>>> Which two patches where? >>>> >>>> [PATCH v3] vt: Reject zero-sized screen buffer size. >>>> from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp >>> >>> This is now in my tree. >>> >>>> [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins. >>>> from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp >>> >>> That should be taken by the fbdev maintainer, but I can take it too if >>> people want. >> >> Just missed this weeks pull request train and feeling like not worth >> making this an exception (it's been broken forever after all), so >> maybe best if you just add this to vt. >> >> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> >> Also this avoids the impression I know what's going on in fbdev code, >> maybe with sufficient abandon from my side someone will pop up who >> cares an fixes the bazillion of syzkaller issues we seem to have >> around console/vt and everything related. > > Great, will go queue it up now, thanks! Fine with me, thanks! PS I'll later queue the patch from George to drm-misc-next (after reading both fbdev patches in detail it seems that both are needed). Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c index ca935c09a261..35ebeeccde4d 100644 --- a/drivers/video/fbdev/core/bitblit.c +++ b/drivers/video/fbdev/core/bitblit.c @@ -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; region.dy = 0; region.width = rw; @@ -224,7 +224,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info, info->fbops->fb_fillrect(info, ®ion); } - if (bh) { + if ((int) bh > 0) { region.dx = info->var.xoffset; region.dy = info->var.yoffset + bs; region.width = rs; diff --git a/drivers/video/fbdev/core/fbcon_ccw.c b/drivers/video/fbdev/core/fbcon_ccw.c index dfa9a8aa4509..78f3a5621478 100644 --- a/drivers/video/fbdev/core/fbcon_ccw.c +++ b/drivers/video/fbdev/core/fbcon_ccw.c @@ -201,7 +201,7 @@ static void ccw_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 = 0; region.dy = info->var.yoffset; region.height = rw; @@ -209,7 +209,7 @@ static void ccw_clear_margins(struct vc_data *vc, struct fb_info *info, info->fbops->fb_fillrect(info, ®ion); } - if (bh) { + if ((int) bh > 0) { region.dx = info->var.xoffset + bs; region.dy = 0; region.height = info->var.yres_virtual; diff --git a/drivers/video/fbdev/core/fbcon_cw.c b/drivers/video/fbdev/core/fbcon_cw.c index ce08251bfd38..fd098ff17574 100644 --- a/drivers/video/fbdev/core/fbcon_cw.c +++ b/drivers/video/fbdev/core/fbcon_cw.c @@ -184,7 +184,7 @@ static void cw_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 = 0; region.dy = info->var.yoffset + rs; region.height = rw; @@ -192,7 +192,7 @@ static void cw_clear_margins(struct vc_data *vc, struct fb_info *info, info->fbops->fb_fillrect(info, ®ion); } - if (bh) { + if ((int) bh > 0) { region.dx = info->var.xoffset; region.dy = info->var.yoffset; region.height = info->var.yres; diff --git a/drivers/video/fbdev/core/fbcon_ud.c b/drivers/video/fbdev/core/fbcon_ud.c index 1936afc78fec..e165a3fad29a 100644 --- a/drivers/video/fbdev/core/fbcon_ud.c +++ b/drivers/video/fbdev/core/fbcon_ud.c @@ -231,7 +231,7 @@ static void ud_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.dy = 0; region.dx = info->var.xoffset; region.width = rw; @@ -239,7 +239,7 @@ static void ud_clear_margins(struct vc_data *vc, struct fb_info *info, info->fbops->fb_fillrect(info, ®ion); } - if (bh) { + if ((int) bh > 0) { region.dy = info->var.yoffset; region.dx = info->var.xoffset; region.height = bh;
syzbot is reporting general protection fault in bitfill_aligned() [1] caused by integer underflow in bit_clear_margins(). The cause of this problem is when and how do_vc_resize() updates vc->vc_{cols,rows}. If vc_do_resize() fails (e.g. kzalloc() fails) when var.xres or var.yres is going to shrink, vc->vc_{cols,rows} will not be updated. This allows bit_clear_margins() to see info->var.xres < (vc->vc_cols * cw) or info->var.yres < (vc->vc_rows * ch). Unexpectedly large rw or bh will try to overrun the __iomem region and causes general protection fault. Also, vc_resize(vc, 0, 0) does not set vc->vc_{cols,rows} = 0 due to new_cols = (cols ? cols : vc->vc_cols); new_rows = (lines ? lines : vc->vc_rows); exception. Since cols and lines are calculated as cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres); rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres); cols /= vc->vc_font.width; rows /= vc->vc_font.height; vc_resize(vc, cols, rows); in fbcon_modechanged(), var.xres < vc->vc_font.width makes cols = 0 and var.yres < vc->vc_font.height makes rows = 0. This means that const int fd = open("/dev/fb0", O_ACCMODE); struct fb_var_screeninfo var = { }; ioctl(fd, FBIOGET_VSCREENINFO, &var); var.xres = var.yres = 1; ioctl(fd, FBIOPUT_VSCREENINFO, &var); easily reproduces integer underflow bug explained above. Of course, callers of vc_resize() are not handling vc_do_resize() failure is bad. But we can't avoid vc_resize(vc, 0, 0) which returns 0. Therefore, as a band-aid workaround, this patch checks integer underflow in "struct fbcon_ops"->clear_margins call, assuming that vc->vc_cols * vc->vc_font.width and vc->vc_rows * vc->vc_font.heigh do not cause integer overflow. [1] https://syzkaller.appspot.com/bug?id=a565882df74fa76f10d3a6fec4be31098dbb37c6 Reported-and-tested-by: syzbot <syzbot+e5fd3e65515b48c02a30@syzkaller.appspotmail.com> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- drivers/video/fbdev/core/bitblit.c | 4 ++-- drivers/video/fbdev/core/fbcon_ccw.c | 4 ++-- drivers/video/fbdev/core/fbcon_cw.c | 4 ++-- drivers/video/fbdev/core/fbcon_ud.c | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-)