says that width * bpp was a sane value for normal boot
[ 9.993434] height width€0 bpp2 bitsd bytes=8 dstÿff9864409c2000 dst_idx!676032 p->fix.line_lengthG04
[ 15.494941] height=8 width€0 bpp2 bitsd bytes=8 dstÿff9864409c2000 dst_idx"278144 p->fix.line_lengthG04
but width * bpp was overflowing when the reproducer shown above was executed.
[ 28.164111] heightˆ5 widthB94966497 bpp2 bitsd bytes=8 dstÿff9864409c2000 dst_idx%600 p->fix.line_lengthG04
syzbot has several bug reports which are stalling inside filling functions. I guess
that these reports are unexpectedly longer loops caused by integer overflow/underflow.
Thus, I consider that we need more sanity/constraints checks.
I don't have other devices to test. Please check your drivers.
says that width * bpp was a sane value for normal boot
[ 9.993434] height width€0 bpp2 bitsd bytes=8 dstÿff9864409c2000 dst_idx!676032 p->fix.line_lengthG04
[ 15.494941] height=8 width€0 bpp2 bitsd bytes=8 dstÿff9864409c2000 dst_idx"278144 p->fix.line_lengthG04
but width * bpp was overflowing when the reproducer shown above was executed.
[ 28.164111] heightˆ5 widthB94966497 bpp2 bitsd bytes=8 dstÿff9864409c2000 dst_idx%600 p->fix.line_lengthG04
syzbot has several bug reports which are stalling inside filling functions. I guess
that these reports are unexpectedly longer loops caused by integer overflow/underflow.
Thus, I consider that we need more sanity/constraints checks.
I don't have other devices to test. Please check your drivers.
What drivers?
Where is the over/underflow happening here when we set a size to be so
small? We should bound the size somewhere, and as you show, that's not
really working properly, right?
thanks,
greg k-h
Where is the over/underflow happening here when we set a size to be so
small? We should bound the size somewhere, and as you show, that's not
really working properly, right?
It is bit_clear_margins() where integer underflow is happening (4294966497 = 1 - 100 * 8),
but the cause of this problem seems to be fbcon_startup() or vc_do_resize().
Since fbcon_modechanged() is doing
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);
(...snipped...)
update_screen(vc);
, info->var.xres < vc->vc_font.width makes cols = 0 and info->var.yres < vc->vc_font.height
makes rows = 0. But vc_resize(vc, 0, 0) has a special meaning because vc_do_resize() is doing
new_cols = (cols ? cols : vc->vc_cols);
new_rows = (lines ? lines : vc->vc_rows);
which results in new_cols = 100 and new_rows = 37 despite var.xres = var.yres = 1,
and vc_do_resize() returns without actually resizing. Then, fbcon_modechanged() calls
fbcon_switch(vc) via vc->vc_sw->con_switch(vc) via redraw_screen(vc, 0) via update_screen(vc),
and fbcon_switch() calls bit_clear_margins() via fbcon_clear_margins(vc, 0), and integer
underflow happens due to info->var.xres=1 && vc->vc_cols0 && vc->vc_font.width=8.
And fbcon_modechanged() is too late to return -EINVAL if
info->var.xres < vc->vc_font.width || info->var.yres < vc->vc_font.height at fb_set_var().
----------
Where is the over/underflow happening here when we set a size to be so
small? We should bound the size somewhere, and as you show, that's not
really working properly, right?
It is bit_clear_margins() where integer underflow is happening (4294966497 = 1 - 100 * 8),
but the cause of this problem seems to be fbcon_startup() or vc_do_resize().
A possible cause is that vc_resize(vc, 0, 0) fails to set vc->vc_cols = vc->vc_rows = 0
because of
new_cols = (cols ? cols : vc->vc_cols);
new_rows = (lines ? lines : vc->vc_rows);
exception. I don't know how user program referenced as
/*
* Change # of rows and columns (0 means unchanged/the size of fg_console)
* [this is to be used together with some user program
* like resize that changes the hardware videomode]
*/
#define VC_RESIZE_MAXCOL (32767)
#define VC_RESIZE_MAXROW (32767)
is utilizing this exception (this code predates the git repository). But since I
don't think that a console with 0 column and/or 0 row makes sense, the real root
cause might be that fb_set_var() fails to reject too small var.xres value for
keeping cols > 0 and too small var.yres value for keeping lines > 0.
Regardless, making "struct fbcon_ops"->clear_margins no-op when integer underflow
is detected (like below diff) helps avoiding crash. Can we apply this handy
protection assuming that vc->vc_cols * vc->vc_font.width and
vc->vc_rows * vc->vc_font.heigh do not cause integer overflow?
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(-)
syzbot is reporting general protection fault in do_con_write() [1] caused
by vc->vc_screenbuf = ZERO_SIZE_PTR caused by vc->vc_screenbuf_size = 0
caused by vc->vc_cols = vc->vc_rows = vc->vc_size_row = 0 caused by
fb_set_var() from ioctl(FBIOPUT_VSCREENINFO) on /dev/fb0 , for
gotoxy(vc, 0, 0) from reset_terminal() from vc_init() from vc_allocate()
from con_install() from tty_init_dev() from tty_open() on such console
causes vc->vc_pos = 0x10000000e due to
((unsigned long) ZERO_SIZE_PTR) + -1U * 0 + (-1U << 1).
I don't think that a console with 0 column or 0 row makes sense. And it
seems that vc_do_resize() does not intend to allow resizing a console to
0 column or 0 row due to
new_cols = (cols ? cols : vc->vc_cols);
new_rows = (lines ? lines : vc->vc_rows);
exception.
Theoretically, cols and rows can be any range as long as
0 < cols * rows * 2 <= KMALLOC_MAX_SIZE is satisfied (e.g.
cols = 1048576 && rows = 2 is possible) because of
vc->vc_size_row = vc->vc_cols << 1;
vc->vc_screenbuf_size = vc->vc_rows * vc->vc_size_row;
in visual_init() and kzalloc(vc->vc_screenbuf_size) in vc_allocate().
Since we can detect cols = 0 or rows = 0 via screenbuf_size = 0 in
visual_init(), we can reject kzalloc(0). Then, vc_allocate() will return
an error, and con_write() will not be called on a console with 0 column
or 0 row.
We need to make sure that integer overflow in visual_init() won't happen.
Since vc_do_resize() restricts cols <= 32767 and rows <= 32767, applying
1 <= cols <= 32767 and 1 <= rows <= 32767 restrictions to vc_allocate()
will be practically fine.
This patch does not touch con_init(), for returning -EINVAL there
does not help when we are not returning -ENOMEM.
[1] https://syzkaller.appspot.com/bug?extid7265e8553724e514e8
Reported-and-tested-by: syzbot <redacted>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
drivers/tty/vt/vt.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
@@ -1125,6 +1134,11 @@ int vc_allocate(unsigned int currcons) /* return 0 on success */if(!*vc->vc_uni_pagedir_loc)con_set_default_unimap(vc);+err=-EINVAL;+if(vc->vc_cols>VC_MAXCOL||vc->vc_rows>VC_MAXROW||+vc->vc_screenbuf_size>KMALLOC_MAX_SIZE||!vc->vc_screenbuf_size)+gotoerr_free;+err=-ENOMEM;vc->vc_screenbuf=kzalloc(vc->vc_screenbuf_size,GFP_KERNEL);if(!vc->vc_screenbuf)gotoerr_free;
@@ -1143,7 +1157,7 @@ int vc_allocate(unsigned int currcons) /* return 0 on success */visual_deinit(vc);kfree(vc);vc_cons[currcons].d=NULL;-return-ENOMEM;+returnerr;}staticinlineintresize_screen(structvc_data*vc,intwidth,intheight,
@@ -1158,14 +1172,6 @@ static inline int resize_screen(struct vc_data *vc, int width, int height,returnerr;}-/*-*Change#ofrowsandcolumns(0meansunchanged/thesizeoffg_console)-*[thisistobeusedtogetherwithsomeuserprogram-*likeresizethatchangesthehardwarevideomode]-*/-#define VC_RESIZE_MAXCOL (32767)-#define VC_RESIZE_MAXROW (32767)-/***vc_do_resize-resizingmethodforthetty*@tty:ttybeingresized
I found 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);
causes general protection fault in bitfill_aligned(), for vc_do_resize()
updates vc->vc_{cols,rows} only when vc_do_resize() will return 0.
[ 20.102222] BUG: unable to handle page fault for address: ffffb80500d7b000
[ 20.102225] #PF: supervisor write access in kernel mode
[ 20.102226] #PF: error_code(0x0002) - not-present page
[ 20.102227] PGD 13a48c067 P4D 13a48c067 PUD 13a48d067 PMD 132525067 PTE 0
[ 20.102230] Oops: 0002 [#1] SMP
[ 20.102232] CPU: 3 PID: 2786 Comm: a.out Not tainted 5.8.0-rc4+ #749
[ 20.102233] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020
[ 20.102237] RIP: 0010:bitfill_aligned+0x87/0x120 [cfbfillrect]
[ 20.102277] Call Trace:
[ 20.102281] cfb_fillrect+0x159/0x340 [cfbfillrect]
[ 20.102747] vmw_fb_fillrect+0x12/0x30 [vmwgfx]
[ 20.102755] bit_clear_margins+0x92/0xf0 [fb]
[ 20.102760] fbcon_clear_margins+0x4c/0x50 [fb]
[ 20.102763] fbcon_switch+0x321/0x570 [fb]
[ 20.102771] redraw_screen+0xe0/0x250
[ 20.102775] fbcon_modechanged+0x164/0x1b0 [fb]
[ 20.102779] fbcon_update_vcs+0x15/0x20 [fb]
[ 20.102781] fb_set_var+0x364/0x3c0 [fb]
[ 20.102817] do_fb_ioctl+0x2ff/0x3f0 [fb]
[ 20.103139] fb_ioctl+0x2e/0x40 [fb]
[ 20.103141] ksys_ioctl+0x86/0xc0
[ 20.103146] __x64_sys_ioctl+0x15/0x20
[ 20.103148] do_syscall_64+0x54/0xa0
[ 20.103151] entry_SYSCALL_64_after_hwframe+0x44/0xa9
If vc_do_resize() fails (e.g. kzalloc() failure) when var.xres or var.yres
is going to shrink, bit_clear_margins() hits integer underflow bug due to
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.
This crash is easily reproducible by calling vc_do_resize(vc, 0, 0)
which the reproducer above will do. Since fbcon_modechanged() is doing
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);
(...snipped...)
update_screen(vc);
, var.xres < vc->vc_font.width makes cols = 0 and var.yres < vc->vc_font.height
makes rows = 0. But vc_do_resize() does not set vc->vc_cols = vc->vc_rows = 0
due to
new_cols = (cols ? cols : vc->vc_cols);
new_rows = (lines ? lines : vc->vc_rows);
exception.
Of course, the root problem is that callers of do_vc_resize() are not
handling vc_do_resize() failures, but it might not be easy to handle
them under complicated dependency. 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.
I hope that we can survive even if info->var.{xres,yres} were increased
but vc->vc_{cols,rows} were not increased due to kzalloc() failure, for
the __iomem memory for cfb_fillrect() seems to be allocated upon driver
load.
By the way, syzbot has several reports which are stalling inside filling
functions. Although reproducer for [1] is not found yet, it has tried
r0 = openat$fb0(0xffffffffffffff9c, &(0x7f0000000180)='/dev/fb0\x00', 0x0, 0x0)
ioctl$FBIOPUT_VSCREENINFO(r0, 0x4601, &(0x7f0000000000)={0x0, 0x0, 0x0, 0x500, 0x0, 0x0, 0x4})
which corresponds to
const int fd = open("/dev/fb0", O_ACCMODE);
struct fb_var_screeninfo var = { };
var.yres_virtual = 0x500;
var.bits_per_pixel = 4;
ioctl(fd, FBIOPUT_VSCREENINFO, &var);
and somehow hit unexpectedly long bit_clear_margins() loops. I don't know
why syzbot does not hit general protection fault, but it would depend on
environment because in my VMware environment ioctl(FBIOPUT_VSCREENINFO)
returns -EINVAL if var.xres = var.yres = 0.
[1] https://syzkaller.appspot.com/bug?id‘ecc3bf32ab1a551c33a39dab7fc0c8cd7b7e16
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(-)
[ Please Cc: fbdev Maintainer (happens to be me :) on fbdev patches, thanks. ]
Hi,
On 7/12/20 1:10 PM, Tetsuo Handa wrote:
I found 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);
causes general protection fault in bitfill_aligned(), for vc_do_resize()
updates vc->vc_{cols,rows} only when vc_do_resize() will return 0.
[ 20.102222] BUG: unable to handle page fault for address: ffffb80500d7b000
[ 20.102225] #PF: supervisor write access in kernel mode
[ 20.102226] #PF: error_code(0x0002) - not-present page
[ 20.102227] PGD 13a48c067 P4D 13a48c067 PUD 13a48d067 PMD 132525067 PTE 0
[ 20.102230] Oops: 0002 [#1] SMP
[ 20.102232] CPU: 3 PID: 2786 Comm: a.out Not tainted 5.8.0-rc4+ #749
[ 20.102233] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020
[ 20.102237] RIP: 0010:bitfill_aligned+0x87/0x120 [cfbfillrect]
[ 20.102277] Call Trace:
[ 20.102281] cfb_fillrect+0x159/0x340 [cfbfillrect]
[ 20.102747] vmw_fb_fillrect+0x12/0x30 [vmwgfx]
[ 20.102755] bit_clear_margins+0x92/0xf0 [fb]
[ 20.102760] fbcon_clear_margins+0x4c/0x50 [fb]
[ 20.102763] fbcon_switch+0x321/0x570 [fb]
[ 20.102771] redraw_screen+0xe0/0x250
[ 20.102775] fbcon_modechanged+0x164/0x1b0 [fb]
[ 20.102779] fbcon_update_vcs+0x15/0x20 [fb]
[ 20.102781] fb_set_var+0x364/0x3c0 [fb]
[ 20.102817] do_fb_ioctl+0x2ff/0x3f0 [fb]
[ 20.103139] fb_ioctl+0x2e/0x40 [fb]
[ 20.103141] ksys_ioctl+0x86/0xc0
[ 20.103146] __x64_sys_ioctl+0x15/0x20
[ 20.103148] do_syscall_64+0x54/0xa0
[ 20.103151] entry_SYSCALL_64_after_hwframe+0x44/0xa9
If vc_do_resize() fails (e.g. kzalloc() failure) when var.xres or var.yres
is going to shrink, bit_clear_margins() hits integer underflow bug due to
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.
This crash is easily reproducible by calling vc_do_resize(vc, 0, 0)
which the reproducer above will do. Since fbcon_modechanged() is doing
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);
(...snipped...)
update_screen(vc);
, var.xres < vc->vc_font.width makes cols = 0 and var.yres < vc->vc_font.height
makes rows = 0. But vc_do_resize() does not set vc->vc_cols = vc->vc_rows = 0
due to
new_cols = (cols ? cols : vc->vc_cols);
new_rows = (lines ? lines : vc->vc_rows);
exception.
Of course, the root problem is that callers of do_vc_resize() are not
handling vc_do_resize() failures, but it might not be easy to handle
them under complicated dependency. 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.
I hope that we can survive even if info->var.{xres,yres} were increased
but vc->vc_{cols,rows} were not increased due to kzalloc() failure, for
the __iomem memory for cfb_fillrect() seems to be allocated upon driver
load.
By the way, syzbot has several reports which are stalling inside filling
functions. Although reproducer for [1] is not found yet, it has tried
r0 = openat$fb0(0xffffffffffffff9c, &(0x7f0000000180)='/dev/fb0\x00', 0x0, 0x0)
ioctl$FBIOPUT_VSCREENINFO(r0, 0x4601, &(0x7f0000000000)={0x0, 0x0, 0x0, 0x500, 0x0, 0x0, 0x4})
which corresponds to
const int fd = open("/dev/fb0", O_ACCMODE);
struct fb_var_screeninfo var = { };
var.yres_virtual = 0x500;
var.bits_per_pixel = 4;
ioctl(fd, FBIOPUT_VSCREENINFO, &var);
and somehow hit unexpectedly long bit_clear_margins() loops. I don't know
why syzbot does not hit general protection fault, but it would depend on
environment because in my VMware environment ioctl(FBIOPUT_VSCREENINFO)
returns -EINVAL if var.xres = var.yres = 0.
[1] https://syzkaller.appspot.com/bug?id‘ecc3bf32ab1a551c33a39dab7fc0c8cd7b7e16
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
How does this patch relate to:
https://marc.info/?l=linux-fbdev&m9415024816722&w=2
?
It seems to address the same issue, I've added George and Dan to Cc:.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
George Kennedy's patch does not help for my case.
You can try a.out built from
----------
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/ioctl.h>
#include <linux/fb.h>
int main(int argc, char *argv[])
{
const int fd = open("/dev/fb0", O_ACCMODE);
struct fb_var_screeninfo var = { };
ioctl(fd, FBIOGET_VSCREENINFO, &var);
var.xres = var.yres = 16;
ioctl(fd, FBIOPUT_VSCREENINFO, &var);
return 0;
}
----------
with a fault injection patch
----------
OK. You can add
Reported-and-tested-by: syzbot <redacted>
to my patch.
By the way, if
/* bitfill_aligned() assumes that it's at least 8x8 */
is true, don't we need to also check that the rect to fill is at least
8x8 in bit_clear_margins() ? (Well, I feel did it mean multiple of 8x8 ?
Then, what is bitfill_unaligned() for ?)
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¥65882df74fa76f10d3a6fec4be31098dbb37c6
Reported-and-tested-by: syzbot <redacted>
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(-)
From: Dan Carpenter <hidden> Date: 2020-07-15 09:51:03
On Wed, Jul 15, 2020 at 10:51:02AM +0900, Tetsuo Handa wrote:
quoted hunk
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¥65882df74fa76f10d3a6fec4be31098dbb37c6
Reported-and-tested-by: syzbot <redacted>
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(-)
^^^^^^^^^^^^^^^^^^^^^^
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
^^^^^^^^^^^^^^^^^^^^^^
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.
^^^^^^^^^^^^^^^^^^^^^^
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(),
----------
^^^^^^^^^^^^^^^^^^^^^^
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(),
----------
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.
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?
From: Daniel Vetter <hidden> Date: 2020-07-16 10:00:14
On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
On 2020/07/16 0:12, Dan Carpenter wrote:
quoted
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.
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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
quoted
On 2020/07/16 0:12, Dan Carpenter wrote:
quoted
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.
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.
quoted
Anyway, my two patches are small and low cost; can we apply these patches regardless
of basic checks?
On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
On 2020/07/16 19:00, Daniel Vetter wrote:
quoted
On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
quoted
On 2020/07/16 0:12, Dan Carpenter wrote:
quoted
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.
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 ?
quoted
Just to avoid confusion here.
quoted
Anyway, my two patches are small and low cost; can we apply these patches regardless
of basic checks?
From: Daniel Vetter <hidden> Date: 2020-07-22 08:07:21
On Tue, Jul 21, 2020 at 6:08 PM Greg Kroah-Hartman
[off-list ref] wrote:
On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
quoted
On 2020/07/16 19:00, Daniel Vetter wrote:
quoted
On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
quoted
On 2020/07/16 0:12, Dan Carpenter wrote:
quoted
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.
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 ?
quoted
Just to avoid confusion here.
quoted
Anyway, my two patches are small and low cost; can we apply these patches regardless
of basic checks?
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 <redacted>
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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
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
[off-list ref] wrote:
quoted
On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
quoted
On 2020/07/16 19:00, Daniel Vetter wrote:
quoted
On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
quoted
On 2020/07/16 0:12, Dan Carpenter wrote:
quoted
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.
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 ?
quoted
Just to avoid confusion here.
quoted
Anyway, my two patches are small and low cost; can we apply these patches regardless
of basic checks?
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 <redacted>
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.
On Wed, Jul 22, 2020 at 10:07:06AM +0200, Daniel Vetter wrote:
quoted
On Tue, Jul 21, 2020 at 6:08 PM Greg Kroah-Hartman
[off-list ref] wrote:
quoted
On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
quoted
On 2020/07/16 19:00, Daniel Vetter wrote:
quoted
On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
quoted
On 2020/07/16 0:12, Dan Carpenter wrote:
quoted
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.
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 ?
quoted
Just to avoid confusion here.
quoted
Anyway, my two patches are small and low cost; can we apply these patches regardless
of basic checks?
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 <redacted>
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
On Sun, Jul 12, 2020 at 08:10:12PM +0900, Tetsuo Handa wrote:
quoted hunk
[...]
@@ -1125,6 +1134,11 @@ int vc_allocate(unsigned int currcons) /* return 0 on success */ if (!*vc->vc_uni_pagedir_loc) con_set_default_unimap(vc);+ err = -EINVAL;+ if (vc->vc_cols > VC_MAXCOL || vc->vc_rows > VC_MAXROW ||+ vc->vc_screenbuf_size > KMALLOC_MAX_SIZE || !vc->vc_screenbuf_size)+ goto err_free;+ err = -ENOMEM; vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_KERNEL); if (!vc->vc_screenbuf) goto err_free;
I realize this patch already landed, but I wanted to remind folks to
use the check_*_overflow() helpers, which can make a lot of this kind
of stuff easier to deal with.
For example, in this case, I think visual_init() could likely be changed
to return success/failure and do all the sanity checking:
if (check_shl_overflow(vc->vc_cols, 1, &vc->vc_size_row) ||
check_mul_overflow(vc->vc_rows, vc->vc_size_row, &vc->vc_screenbuf_size))
return -EINVAL;
--
Kees Cook