Re: fbconsole needs more parameter validations.
On 2020/07/11 15:16, Tetsuo Handa wrote:
> On 2020/07/10 19:56, Greg Kroah-Hartman wrote:
>> 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(-)
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) {
r
Re: fbconsole needs more parameter validations.
On 2020/07/10 19:56, Greg Kroah-Hartman wrote:
> 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_cols=100 &&
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().
--
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 42d8c67a481f..4af82cabb6c4 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -1214,6 +1214,8 @@ static int vc_do_resize(struct tty_struct *tty, struct
vc_data *vc,
new_rows = (lines ? lines : vc->vc_rows);
new_row_size = new_cols << 1;
new_screen_size = new_row_size * new_rows;
+ printk(KERN_DEBUG "%s: new_cols=%u cols=%u vc->vc_cols=%u new_rows=%u
lines=%u vc->vc_rows=%u\n",
+ __func__, new_cols, cols, vc->vc_cols, new_rows, lines,
vc->vc_rows);
if (new_cols == vc->vc_cols && new_rows == vc->vc_rows)
return 0;
diff --git a/drivers/video/fbdev/core/bitblit.c
b/drivers/video/fbdev/core/bitblit.c
index ca935c09a261..8d949679bfba 100644
--- a/drivers/video/fbdev/core/bitblit.c
+++ b/drivers/video/fbdev/core/bitblit.c
@@ -221,6 +221,8 @@ static void bit_clear_margins(struct vc_data *vc, struct
fb_info *info,
region.dy = 0;
region.width = rw;
region.height = info->var.yres_virtual;
+ printk(KERN_DEBUG "%s: rw=%u info->var.xres=%u vc->vc_cols=%u
vc->vc_font.width=%u\n",
+ __func__, rw, info->var.xres, vc->vc_cols,
vc->vc_font.width);
info->fbops->fb_fillrect(info, ®ion);
}
@@ -229,6 +231,8 @@ static void bit_clear_margins(struct vc_data *vc, struct
fb_info *info,
region.dy = info->var.yoffset + bs;
region.width = rs;
region.height = bh;
+ printk(KERN_DEBUG "%s: bh=%u info->var.yres=%u vc->vc_rows=%u
vc->vc_font.height=%u\n",
+ __func__, bh, info->var.yres, vc->vc_rows,
vc->vc_font.height);
info->fbops->fb_fillrect(info, ®ion);
}
}
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index e2a490c5ae08..f83525a58137 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2983,6 +2983,8 @@ static void fbcon_modechanged(struct fb_info *info)
if (con_is_visible(vc)) {
var_to_display(p, &info->var, info);
+ printk(KERN_DEBUG "%s: ops->rotate=%d info->var.xres=%u,
info->var.yres=%u vc->vc_font.width=%u vc->vc_font.height=%u\n",
+ __func__, ops->rotate, info->var.xres, info->var.yres,
vc->vc_font.width, vc->vc_font.height);
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;
--
--
[ 21.854895][ T2790] fbcon_modechanged: ops->rotate=0 info->var.xres=1,
info->var.yres=1 vc->vc_font.width=8 vc->vc_font.height=16
[ 21.854900][ T2790] vc_do_resize: new_cols=100 cols=0 vc->vc_cols=100
new_rows=37 lines=0 vc->vc_rows=37
[ 21.854909][ T2790] bit_clear_margins: rw=4294966497 info->var.xres=1
vc->vc_cols=100 vc->vc_font.width=8
[ 21.855743][ T2790] BUG: unable to handle page fault for address:
b54440d3b000
[ 21.855745][ T2790] #PF: supervisor write access in kernel mode
[ 21.855746][ T2790] #PF: error_code(0x0002) - not-present page
[ 21.855747][ T2790] PGD 13a48c067 P4D 13a48c067 PUD 13a48d067 PMD 13251c067
PTE 0
[ 21.855751][ T2790] Oops: 0002 [#1] SMP
[ 21.855753][ T2790] CPU: 0 PID: 2790 Comm: a.ou
Re: fbconsole needs more parameter validations.
On Fri, Jul 10, 2020 at 02:56:58PM +0900, Tetsuo Handa wrote:
> Hello.
>
> While trying to debug
> https://syzkaller.appspot.com/bug?extid=017265e8553724e514e8 ,
> I noticed that a crash can happen without opening /dev/ttyXX .
>
> For example, while a driver which syzbot is reporting accepts screen with
> var.xres = var.yres = 0 (and a crash is not visible until trying to write to
> /dev/ttyXX ), a driver for VMware environment which I'm using (dmesg says
> "fbcon:
> svgadrmfb (fb0) is primary device") rejects screen with var.xres = var.yres =
> 0.
> However, specifying var.xres = var.yres = 1 like below reproducer causes a
> crash
> in my VMware environment.
>
> --
> #include
> #include
> #include
> #include
> #include
>
> 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 = 1;
> ioctl(fd, FBIOPUT_VSCREENINFO, &var);
> return 0;
> }
> --
>
> --
> [ 20.10] BUG: unable to handle page fault for address: b80500d7b000
> [ 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.102238] Code: c3 45 85 db 0f 85 85 00 00 00 44 89 c0 31 d2 41 f7 f1 89
> c2 83 f8 07 76 41 8d 48 f8 c1 e9 03 48 83 c1 01 48 c1 e1 06 48 01 f1 <48> 89
> 3e 48 89 7e 08 48 89 7e 10 48 89 7e 18 48 89 7e 20 48 89 7e
> [ 20.102239] RSP: 0018:b805012939a8 EFLAGS: 00010206
> [ 20.102240] RAX: 03fffe70 RBX: 9c20 RCX:
> b80520982000
> [ 20.102241] RDX: 03fffe70 RSI: b80500d7b000 RDI:
>
> [ 20.102242] RBP: b805012939b8 R08: 9c20 R09:
> b80500d7aff8
> [ 20.102242] R10: R11: R12:
>
> [ 20.102243] R13: 976734c0c000 R14: R15:
> b80500982c80
> [ 20.102244] FS: 7f0c9589e740() GS:97673aec()
> knlGS:
> [ 20.102265] CS: 0010 DS: ES: CR0: 80050033
> [ 20.102265] CR2: b80500d7b000 CR3: 000136cdf004 CR4:
> 001606e0
> [ 20.102277] Call Trace:
> [ 20.102281] cfb_fillrect+0x159/0x340 [cfbfillrect]
> [ 20.102385] ? __mutex_unlock_slowpath+0x158/0x2d0
> [ 20.102493] ? cfb_fillrect+0x340/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.102894] ? find_held_lock+0x35/0xa0
> [ 20.103126] ? __audit_syscall_entry+0xd8/0x120
> [ 20.103135] ? kfree+0x25a/0x2b0
> [ 20.103139] fb_ioctl+0x2e/0x40 [fb]
> [ 20.103141] ksys_ioctl+0x86/0xc0
> [ 20.103144] ? do_syscall_64+0x20/0xa0
> [ 20.103146] __x64_sys_ioctl+0x15/0x20
> [ 20.103148] do_syscall_64+0x54/0xa0
> [ 20.103151] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 20.103152] RIP: 0033:0x7f0c953b8307
> [ 20.103153] Code: Bad RIP value.
> [ 20.103154] RSP: 002b:7ffecbdce0f8 EFLAGS: 0246 ORIG_RAX:
> 0010
> [ 20.103155] RAX: ffda RBX: 0003 RCX:
> 7f0c953b8307
> [ 20.103156] RDX: 7ffecbdce100 RSI: 4601 RDI:
> 0003
> [ 20.103156] RBP: R08: 7f0c9568be80 R09:
>
> [ 20.103157] R10: 7ffecbdcdb60 R11: 0246 R12:
> 004004f2
> [ 20.103158] R13: 7ffecbdce280 R14: R15:
>
> [ 20.103162] Modules linked in: mousedev rapl evdev input_leds led_class
> mac_hid psmouse pcspkr xt_tcpudp ip6t_rpfilter ip6t_REJECT nf_reject_ipv6
> ipt_REJECT nf_reject_ipv4 xt_conntrack sg ebtable_nat af_packet ip6table_nat
> ip6table_mangle ip6table_raw iptable_nat nf_nat iptable_mangle iptable_raw
> nf_conntrack rtc_cmos nf_defrag_ipv4 ip_set nfnetlink ebtable_filter ebtables
> ip6table_filter ip6_tables iptable_filter bpfilter i2c_piix4 vmw_vmci ac
> intel_agp button intel_gtt ip_tables x_tables ata_generic pata_acpi serio_raw
> atkbd libps2 vmwgfx drm_kms_helper cfbfillrect syscopyarea cfbimgblt
> sysfillrect sysimgblt fb_sys_fops cfbcopyarea fb fbdev ttm drm i2c_core ahci
> drm_panel_orient
fbconsole needs more parameter validations.
Hello.
While trying to debug
https://syzkaller.appspot.com/bug?extid=017265e8553724e514e8 ,
I noticed that a crash can happen without opening /dev/ttyXX .
For example, while a driver which syzbot is reporting accepts screen with
var.xres = var.yres = 0 (and a crash is not visible until trying to write to
/dev/ttyXX ), a driver for VMware environment which I'm using (dmesg says
"fbcon:
svgadrmfb (fb0) is primary device") rejects screen with var.xres = var.yres = 0.
However, specifying var.xres = var.yres = 1 like below reproducer causes a crash
in my VMware environment.
--
#include
#include
#include
#include
#include
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 = 1;
ioctl(fd, FBIOPUT_VSCREENINFO, &var);
return 0;
}
--
--
[ 20.10] BUG: unable to handle page fault for address: b80500d7b000
[ 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.102238] Code: c3 45 85 db 0f 85 85 00 00 00 44 89 c0 31 d2 41 f7 f1 89
c2 83 f8 07 76 41 8d 48 f8 c1 e9 03 48 83 c1 01 48 c1 e1 06 48 01 f1 <48> 89 3e
48 89 7e 08 48 89 7e 10 48 89 7e 18 48 89 7e 20 48 89 7e
[ 20.102239] RSP: 0018:b805012939a8 EFLAGS: 00010206
[ 20.102240] RAX: 03fffe70 RBX: 9c20 RCX: b80520982000
[ 20.102241] RDX: 03fffe70 RSI: b80500d7b000 RDI:
[ 20.102242] RBP: b805012939b8 R08: 9c20 R09: b80500d7aff8
[ 20.102242] R10: R11: R12:
[ 20.102243] R13: 976734c0c000 R14: R15: b80500982c80
[ 20.102244] FS: 7f0c9589e740() GS:97673aec()
knlGS:
[ 20.102265] CS: 0010 DS: ES: CR0: 80050033
[ 20.102265] CR2: b80500d7b000 CR3: 000136cdf004 CR4: 001606e0
[ 20.102277] Call Trace:
[ 20.102281] cfb_fillrect+0x159/0x340 [cfbfillrect]
[ 20.102385] ? __mutex_unlock_slowpath+0x158/0x2d0
[ 20.102493] ? cfb_fillrect+0x340/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.102894] ? find_held_lock+0x35/0xa0
[ 20.103126] ? __audit_syscall_entry+0xd8/0x120
[ 20.103135] ? kfree+0x25a/0x2b0
[ 20.103139] fb_ioctl+0x2e/0x40 [fb]
[ 20.103141] ksys_ioctl+0x86/0xc0
[ 20.103144] ? do_syscall_64+0x20/0xa0
[ 20.103146] __x64_sys_ioctl+0x15/0x20
[ 20.103148] do_syscall_64+0x54/0xa0
[ 20.103151] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 20.103152] RIP: 0033:0x7f0c953b8307
[ 20.103153] Code: Bad RIP value.
[ 20.103154] RSP: 002b:7ffecbdce0f8 EFLAGS: 0246 ORIG_RAX:
0010
[ 20.103155] RAX: ffda RBX: 0003 RCX: 7f0c953b8307
[ 20.103156] RDX: 7ffecbdce100 RSI: 4601 RDI: 0003
[ 20.103156] RBP: R08: 7f0c9568be80 R09:
[ 20.103157] R10: 7ffecbdcdb60 R11: 0246 R12: 004004f2
[ 20.103158] R13: 7ffecbdce280 R14: R15:
[ 20.103162] Modules linked in: mousedev rapl evdev input_leds led_class
mac_hid psmouse pcspkr xt_tcpudp ip6t_rpfilter ip6t_REJECT nf_reject_ipv6
ipt_REJECT nf_reject_ipv4 xt_conntrack sg ebtable_nat af_packet ip6table_nat
ip6table_mangle ip6table_raw iptable_nat nf_nat iptable_mangle iptable_raw
nf_conntrack rtc_cmos nf_defrag_ipv4 ip_set nfnetlink ebtable_filter ebtables
ip6table_filter ip6_tables iptable_filter bpfilter i2c_piix4 vmw_vmci ac
intel_agp button intel_gtt ip_tables x_tables ata_generic pata_acpi serio_raw
atkbd libps2 vmwgfx drm_kms_helper cfbfillrect syscopyarea cfbimgblt
sysfillrect sysimgblt fb_sys_fops cfbcopyarea fb fbdev ttm drm i2c_core ahci
drm_panel_orientation_quirks libahci backlight e1000 agpgart ata_piix libata
i8042 serio unix ipv6 nf_defrag_ipv6
[ 20.103194] CR2: b80500d7b000
[ 20.103196] ---[ end trace b2348f839f6524f9 ]---
[ 20.103198] RIP: 0010:bitfill_aligned+0x87/0x120 [cfbfillrect]
[ 20.103200] Code: c3 45 85 db 0f 85
