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 <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 = 1; ioctl(fd, FBIOPUT_VSCREENINFO, &var); 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.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:ffffb805012939a8 EFLAGS: 00010206 [ 20.102240] RAX: 0000000003fffe70 RBX: 00000000ffff9c20 RCX: ffffb80520982000 [ 20.102241] RDX: 0000000003fffe70 RSI: ffffb80500d7b000 RDI: 0000000000000000 [ 20.102242] RBP: ffffb805012939b8 R08: 00000000ffff9c20 R09: ffffb80500d7aff8 [ 20.102242] R10: 00000000ffffffff R11: 0000000000000000 R12: ffffffffffffffff [ 20.102243] R13: ffff976734c0c000 R14: 0000000000000000 R15: ffffb80500982c80 [ 20.102244] FS: 00007f0c9589e740(0000) GS:ffff97673aec0000(0000) knlGS:0000000000000000 [ 20.102265] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 20.102265] CR2: ffffb80500d7b000 CR3: 0000000136cdf004 CR4: 00000000001606e0 [ 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:00007ffecbdce0f8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 [ 20.103155] RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00007f0c953b8307 [ 20.103156] RDX: 00007ffecbdce100 RSI: 0000000000004601 RDI: 0000000000000003 [ 20.103156] RBP: 0000000000000000 R08: 00007f0c9568be80 R09: 0000000000000000 [ 20.103157] R10: 00007ffecbdcdb60 R11: 0000000000000246 R12: 00000000004004f2 [ 20.103158] R13: 00007ffecbdce280 R14: 0000000000000000 R15: 0000000000000000 [ 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: ffffb80500d7b000 [ 20.103196] ---[ end trace b2348f839f6524f9 ]--- [ 20.103198] RIP: 0010:bitfill_aligned+0x87/0x120 [cfbfillrect] [ 20.103200] 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.103201] RSP: 0018:ffffb805012939a8 EFLAGS: 00010206 [ 20.103202] RAX: 0000000003fffe70 RBX: 00000000ffff9c20 RCX: ffffb80520982000 [ 20.103202] RDX: 0000000003fffe70 RSI: ffffb80500d7b000 RDI: 0000000000000000 [ 20.103203] RBP: ffffb805012939b8 R08: 00000000ffff9c20 R09: ffffb80500d7aff8 [ 20.103204] R10: 00000000ffffffff R11: 0000000000000000 R12: ffffffffffffffff [ 20.103204] R13: ffff976734c0c000 R14: 0000000000000000 R15: ffffb80500982c80 [ 20.103205] FS: 00007f0c9589e740(0000) GS:ffff97673aec0000(0000) knlGS:0000000000000000 [ 20.103213] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 20.103214] CR2: ffffb80500d7b000 CR3: 0000000136cdf004 CR4: 00000000001606e0 ----------
A debug printk() patch
---------- diff --git a/drivers/video/fbdev/core/cfbfillrect.c b/drivers/video/fbdev/core/cfbfillrect.c index ba9f58b2a5e8..57e4c2d1bcc0 100644 --- a/drivers/video/fbdev/core/cfbfillrect.c +++ b/drivers/video/fbdev/core/cfbfillrect.c @@ -321,6 +321,9 @@ void cfb_fillrect(struct fb_info *p, const struct fb_fillrect *rect) fill_op32 = bitfill_aligned; break; } + if (fill_op32 == bitfill_aligned) + printk(KERN_DEBUG "height=%lu width=%lu bpp=%u bits=%u bytes=%u dst=%px dst_idx=%u p->fix.line_length=%u\n", + height, width, bpp, bits, bytes, dst, dst_idx, p->fix.line_length); while (height--) { dst += dst_idx >> (ffs(bits) - 1); dst_idx &= (bits - 1); ----------
says that width * bpp was a sane value for normal boot
[ 9.993434] height=16 width=800 bpp=32 bits=64 bytes=8 dst=ffff9864409c2000 dst_idx=21676032 p->fix.line_length=4704 [ 15.494941] height=8 width=800 bpp=32 bits=64 bytes=8 dst=ffff9864409c2000 dst_idx=22278144 p->fix.line_length=4704
but width * bpp was overflowing when the reproducer shown above was executed.
[ 28.164111] height=885 width=4294966497 bpp=32 bits=64 bytes=8 dst=ffff9864409c2000 dst_idx=25600 p->fix.line_length=4704
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.
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 <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 = 1; ioctl(fd, FBIOPUT_VSCREENINFO, &var); 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.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:ffffb805012939a8 EFLAGS: 00010206 [ 20.102240] RAX: 0000000003fffe70 RBX: 00000000ffff9c20 RCX: ffffb80520982000 [ 20.102241] RDX: 0000000003fffe70 RSI: ffffb80500d7b000 RDI: 0000000000000000 [ 20.102242] RBP: ffffb805012939b8 R08: 00000000ffff9c20 R09: ffffb80500d7aff8 [ 20.102242] R10: 00000000ffffffff R11: 0000000000000000 R12: ffffffffffffffff [ 20.102243] R13: ffff976734c0c000 R14: 0000000000000000 R15: ffffb80500982c80 [ 20.102244] FS: 00007f0c9589e740(0000) GS:ffff97673aec0000(0000) knlGS:0000000000000000 [ 20.102265] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 20.102265] CR2: ffffb80500d7b000 CR3: 0000000136cdf004 CR4: 00000000001606e0 [ 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:00007ffecbdce0f8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 [ 20.103155] RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00007f0c953b8307 [ 20.103156] RDX: 00007ffecbdce100 RSI: 0000000000004601 RDI: 0000000000000003 [ 20.103156] RBP: 0000000000000000 R08: 00007f0c9568be80 R09: 0000000000000000 [ 20.103157] R10: 00007ffecbdcdb60 R11: 0000000000000246 R12: 00000000004004f2 [ 20.103158] R13: 00007ffecbdce280 R14: 0000000000000000 R15: 0000000000000000 [ 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: ffffb80500d7b000 [ 20.103196] ---[ end trace b2348f839f6524f9 ]--- [ 20.103198] RIP: 0010:bitfill_aligned+0x87/0x120 [cfbfillrect] [ 20.103200] 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.103201] RSP: 0018:ffffb805012939a8 EFLAGS: 00010206 [ 20.103202] RAX: 0000000003fffe70 RBX: 00000000ffff9c20 RCX: ffffb80520982000 [ 20.103202] RDX: 0000000003fffe70 RSI: ffffb80500d7b000 RDI: 0000000000000000 [ 20.103203] RBP: ffffb805012939b8 R08: 00000000ffff9c20 R09: ffffb80500d7aff8 [ 20.103204] R10: 00000000ffffffff R11: 0000000000000000 R12: ffffffffffffffff [ 20.103204] R13: ffff976734c0c000 R14: 0000000000000000 R15: ffffb80500982c80 [ 20.103205] FS: 00007f0c9589e740(0000) GS:ffff97673aec0000(0000) knlGS:0000000000000000 [ 20.103213] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 20.103214] CR2: ffffb80500d7b000 CR3: 0000000136cdf004 CR4: 00000000001606e0
A debug printk() patch
diff --git a/drivers/video/fbdev/core/cfbfillrect.c b/drivers/video/fbdev/core/cfbfillrect.c index ba9f58b2a5e8..57e4c2d1bcc0 100644 --- a/drivers/video/fbdev/core/cfbfillrect.c +++ b/drivers/video/fbdev/core/cfbfillrect.c @@ -321,6 +321,9 @@ void cfb_fillrect(struct fb_info *p, const struct fb_fillrect *rect) fill_op32 = bitfill_aligned; break; }
if (fill_op32 == bitfill_aligned)printk(KERN_DEBUG "height=%lu width=%lu bpp=%u bits=%u bytes=%u dst=%px dst_idx=%u p->fix.line_length=%u\n",height, width, bpp, bits, bytes, dst, dst_idx, p->fix.line_length);
says that width * bpp was a sane value for normal boot
[ 9.993434] height=16 width=800 bpp=32 bits=64 bytes=8 dst=ffff9864409c2000 dst_idx=21676032 p->fix.line_length=4704 [ 15.494941] height=8 width=800 bpp=32 bits=64 bytes=8 dst=ffff9864409c2000 dst_idx=22278144 p->fix.line_length=4704
but width * bpp was overflowing when the reproducer shown above was executed.
[ 28.164111] height=885 width=4294966497 bpp=32 bits=64 bytes=8 dst=ffff9864409c2000 dst_idx=25600 p->fix.line_length=4704
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
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: ffffb54440d3b000 [ 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.out Not tainted 5.8.0-rc4+ #753 [ 21.855754][ T2790] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020 [ 21.855758][ T2790] RIP: 0010:bitfill_aligned+0x87/0x120 [cfbfillrect] [ 21.855759][ T2790] 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 [ 21.855760][ T2790] RSP: 0018:ffffb5444124b9a0 EFLAGS: 00010206 [ 21.855761][ T2790] RAX: 0000000003fffe70 RBX: 00000000ffff9c20 RCX: ffffb54460942000 [ 21.855762][ T2790] RDX: 0000000003fffe70 RSI: ffffb54440d3b000 RDI: 0000000000000000 [ 21.855763][ T2790] RBP: ffffb5444124b9b0 R08: 00000000ffff9c20 R09: ffffb54440d3aff8 [ 21.855763][ T2790] R10: 00000000ffffffff R11: 0000000000000000 R12: ffffffffffffffff [ 21.855764][ T2790] R13: ffffa0d534c32800 R14: 0000000000000000 R15: ffffb54440942c80 [ 21.855765][ T2790] FS: 00007f9cef82a740(0000) GS:ffffa0d53ae00000(0000) knlGS:0000000000000000 [ 21.855785][ T2790] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 21.855786][ T2790] CR2: ffffb54440d3b000 CR3: 000000013766d006 CR4: 00000000001606f0 [ 21.855797][ T2790] Call Trace: [ 21.855801][ T2790] cfb_fillrect+0x159/0x340 [cfbfillrect] [ 21.855806][ T2790] ? vprintk_func+0x5a/0x10d [ 21.855808][ T2790] ? cfb_fillrect+0x340/0x340 [cfbfillrect] [ 21.855821][ T2790] vmw_fb_fillrect+0x12/0x30 [vmwgfx] [ 21.855828][ T2790] bit_clear_margins+0xe0/0xf0 [fb] [ 21.855832][ T2790] fbcon_clear_margins+0x4c/0x50 [fb] [ 21.855835][ T2790] fbcon_switch+0x321/0x570 [fb] [ 21.855843][ T2790] redraw_screen+0xe0/0x250 [ 21.855847][ T2790] fbcon_modechanged+0x1a3/0x1f0 [fb] [ 21.855851][ T2790] fbcon_update_vcs+0x15/0x20 [fb] [ 21.855853][ T2790] fb_set_var+0x364/0x3c0 [fb] [ 21.855863][ T2790] do_fb_ioctl+0x2ff/0x3f0 [fb] [ 21.855866][ T2790] ? find_held_lock+0x35/0xa0 [ 21.855870][ T2790] ? __audit_syscall_entry+0xd8/0x120 [ 21.855888][ T2790] ? kfree+0x25a/0x2b0 [ 21.855944][ T2790] fb_ioctl+0x2e/0x40 [fb] [ 21.855947][ T2790] ksys_ioctl+0x86/0xc0 [ 21.855950][ T2790] ? do_syscall_64+0x20/0xa0 [ 21.855952][ T2790] __x64_sys_ioctl+0x15/0x20 [ 21.855954][ T2790] do_syscall_64+0x54/0xa0 [ 21.855957][ T2790] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 21.855959][ T2790] RIP: 0033:0x7f9cef344307 [ 21.855959][ T2790] Code: Bad RIP value. [ 21.855960][ T2790] RSP: 002b:00007ffddb6f2e48 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 [ 21.855962][ T2790] RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00007f9cef344307 [ 21.855962][ T2790] RDX: 00007ffddb6f2e50 RSI: 0000000000004601 RDI: 0000000000000003 [ 21.855963][ T2790] RBP: 0000000000000000 R08: 00007f9cef617e80 R09: 0000000000000000 [ 21.855964][ T2790] R10: 00007ffddb6f28a0 R11: 0000000000000246 R12: 00000000004004f2 [ 21.855964][ T2790] R13: 00007ffddb6f2fd0 R14: 0000000000000000 R15: 0000000000000000 [ 21.855969][ T2790] Modules linked in: mousedev rapl evdev input_leds led_class mac_hid psmouse pcspkr xt_tcpudp af_packet ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 ipt_REJECT nf_reject_ipv4 xt_conntrack ebtable_nat ip6table_nat ip6table_mangle ip6table_raw iptable_nat nf_nat iptable_mangle iptable_raw nf_conntrack rtc_cmos nf_defrag_ipv4 ip_set vmw_vmci nfnetlink ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter bpfilter sg ac button i2c_piix4 intel_agp 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 ahci libahci i2c_core drm_panel_orientation_quirks backlight ata_piix e1000 agpgart libata i8042 serio unix ipv6 nf_defrag_ipv6 [ 21.856040][ T2790] CR2: ffffb54440d3b000 [ 21.856042][ T2790] ---[ end trace 083bab4cc8751a86 ]--- [ 21.856044][ T2790] RIP: 0010:bitfill_aligned+0x87/0x120 [cfbfillrect] ----------
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) { 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 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?extid=017265e8553724e514e8
Reported-and-tested-by: syzbot syzbot+017265e8553724e514e8@syzkaller.appspotmail.com 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(-)
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c index 48a8199f7845..42d8c67a481f 100644 --- a/drivers/tty/vt/vt.c +++ b/drivers/tty/vt/vt.c @@ -1092,10 +1092,19 @@ static const struct tty_port_operations vc_port_ops = { .destruct = vc_port_destruct, };
+/* + * 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_MAXCOL (32767) +#define VC_MAXROW (32767) + int vc_allocate(unsigned int currcons) /* return 0 on success */ { struct vt_notifier_param param; struct vc_data *vc; + int err;
WARN_CONSOLE_UNLOCKED();
@@ -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; @@ -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; + return err; }
static inline int resize_screen(struct vc_data *vc, int width, int height, @@ -1158,14 +1172,6 @@ static inline int resize_screen(struct vc_data *vc, int width, int height, return err; }
-/* - * 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) - /** * vc_do_resize - resizing method for the tty * @tty: tty being resized @@ -1201,7 +1207,7 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc, user = vc->vc_resize_user; vc->vc_resize_user = 0;
- if (cols > VC_RESIZE_MAXCOL || lines > VC_RESIZE_MAXROW) + if (cols > VC_MAXCOL || lines > VC_MAXROW) return -EINVAL;
new_cols = (cols ? cols : vc->vc_cols); @@ -1212,7 +1218,7 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc, if (new_cols == vc->vc_cols && new_rows == vc->vc_rows) return 0;
- if (new_screen_size > KMALLOC_MAX_SIZE) + if (new_screen_size > KMALLOC_MAX_SIZE || !new_screen_size) return -EINVAL; newscreen = kzalloc(new_screen_size, GFP_USER); if (!newscreen) @@ -3393,6 +3399,7 @@ static int __init con_init(void) INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK); tty_port_init(&vc->port); visual_init(vc, currcons, 1); + /* Assuming vc->vc_{cols,rows,screenbuf_size} are sane here. */ vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT); vc_init(vc, vc->vc_rows, vc->vc_cols, currcons || !vc->vc_sw->con_save_screen);
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=91ecc3bf32ab1a551c33a39dab7fc0c8cd7b7e1...
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; 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;
[ 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=91ecc3bf32ab1a551c33a39dab7fc0c8cd7b7e1...
Signed-off-by: Tetsuo Handa penguin-kernel@I-love.SAKURA.ne.jp
How does this patch relate to:
https://marc.info/?l=linux-fbdev&m=159415024816722&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
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) { 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;
On 2020/07/14 16:22, Bartlomiej Zolnierkiewicz wrote:
How does this patch relate to:
https://marc.info/?l=linux-fbdev&m=159415024816722&w=2
?
It seems to address the same issue, I've added George and Dan to Cc:.
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
---------- --- a/drivers/tty/vt/vt.c +++ b/drivers/tty/vt/vt.c @@ -1214,6 +1214,10 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc,
if (new_screen_size > KMALLOC_MAX_SIZE) return -EINVAL; + if (!strcmp(current->comm, "a.out")) { + printk(KERN_INFO "Forcing memory allocation failure.\n"); + return -ENOMEM; + } newscreen = kzalloc(new_screen_size, GFP_USER); if (!newscreen) return -ENOMEM; ----------
. What my patch workarounds is cases when vc_do_resize() did not update vc->vc_{cols,rows} . Unless vc->vc_{cols,rows} are updated by vc_do_resize() in a way that avoids integer underflow at
unsigned int rw = info->var.xres - (vc->vc_cols*cw); unsigned int bh = info->var.yres - (vc->vc_rows*ch);
, this crash won't go away.
[ 39.995757][ T2788] Forcing memory allocation failure. [ 39.996527][ T2788] BUG: unable to handle page fault for address: ffffa9d180d7b000 [ 39.996529][ T2788] #PF: supervisor write access in kernel mode [ 39.996530][ T2788] #PF: error_code(0x0002) - not-present page [ 39.996531][ T2788] PGD 13a48c067 P4D 13a48c067 PUD 13a48d067 PMD 1324e4067 PTE 0 [ 39.996547][ T2788] Oops: 0002 [#1] SMP [ 39.996550][ T2788] CPU: 2 PID: 2788 Comm: a.out Not tainted 5.8.0-rc5+ #757 [ 39.996551][ T2788] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020 [ 39.996555][ T2788] RIP: 0010:bitfill_aligned+0x87/0x120 [cfbfillrect]
On 2020/07/14 19:27, Tetsuo Handa wrote:
On 2020/07/14 16:22, Bartlomiej Zolnierkiewicz wrote:
How does this patch relate to:
https://marc.info/?l=linux-fbdev&m=159415024816722&w=2
?
It seems to address the same issue, I've added George and Dan to Cc:.
George Kennedy's patch does not help for my case.
OK. You can add
Reported-and-tested-by: syzbot syzbot+e5fd3e65515b48c02a30@syzkaller.appspotmail.com
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=a565882df74fa76f10d3a6fec4be31098dbb37c...
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; 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;
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=a565882df74fa76f10d3a6fec4be31098dbb37c...
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.sak... , 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.sak... , 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.sak... , 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.SAKUR...
[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...
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.sak... , 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.SAKUR...
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...
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.sak... , 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.SAKUR...
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...
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.sak... , 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.SAKUR...
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...
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.sak... , 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.SAKUR...
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...
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
Hello Tetsuo,
Can you try the a.out built from the original Syzkaller modified repro C program? It walks 0-7 through xres and yres of the fb_var_screeninfo struct.
// https://syzkaller.appspot.com/bug?id=a565882df74fa76f10d3a6fec4be31098dbb37c... // autogenerated by syzkaller (https://github.com/google/syzkaller)
#define _GNU_SOURCE
#include <endian.h> #include <stdint.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #include <sys/syscall.h> #include <sys/types.h> #include <sys/fcntl.h> #include <unistd.h>
#include <errno.h>
#include <linux/fb.h>
int verbose = 0;
void dumpit(unsigned char *buf, int count, int addr) { int i, j; char bp[256];
memset(bp, 0, 256);
for (i = j = 0; i < count; i++, j++) { if (j == 16) { j = 0; printf("%s\n", bp); memset(bp, 0, 256); } if (j == 0) { sprintf(&bp[strlen(bp)], "%x: ", addr + i); } sprintf(&bp[strlen(bp)], "%02x ", buf[i]); } if (j != 0) { printf("%s\n", bp); } }
uint64_t r[1] = {0xffffffffffffffff};
int main(int argc, char **argv) { syscall(__NR_mmap, 0x20000000ul, 0x1000000ul, 3ul, 0x32ul, -1, 0); intptr_t res = 0; uint32_t activate = FB_ACTIVATE_NOW; struct fb_var_screeninfo *varp = (struct fb_var_screeninfo *)0x200001c0; struct fb_var_screeninfo *starting_varp = malloc(sizeof(struct fb_var_screeninfo *)); char *vp = (char *)varp; int i, sum, rtn, c; extern char *optarg; int limit = 0, passes = 0; unsigned int start_address = 0; unsigned int pattern = 0; int breakit = 1; while ((c = getopt (argc, argv, "a:v")) != -1) switch (c) { case 'a': activate = strtol(optarg, 0, 0); break; case 'v': verbose++; break; default: fprintf(stderr, "\nusage: %s [-a <activate code>] [-v]\n\n", argv[0]); return -1; }
int fd = open("/dev/fb0", O_RDWR); if (fd < 0) { perror("open"); return 0; } printf("fd: %d\n", fd); r[0] = fd;
rtn = syscall(__NR_ioctl, r[0], 0x4600ul, 0x200001c0ul); if (rtn < 0) { perror("ioctl"); fprintf(stderr, "rtn=%d, errno=%d\n", rtn, errno); }
if (verbose) { printf("FBIOGET_VSCREENINFO:\n"); dumpit((unsigned char *)vp, sizeof(struct fb_var_screeninfo), 0x200001c0); }
memcpy(starting_varp, varp, sizeof(struct fb_var_screeninfo));
fprintf(stderr, "activate = %d\n", activate);
varp->activate = activate;
if (verbose) { printf("Pre FBIOPUT_VSCREENINFO:\n"); dumpit((unsigned char *)vp, sizeof(struct fb_var_screeninfo), 0x200001c0);
sleep(2); }
rtn = syscall(__NR_ioctl, r[0], 0x4601ul, 0x200001c0ul); if (rtn < 0) { perror("ioctl"); fprintf(stderr, "rtn=%d, errno=%d\n", rtn, errno); } limit = 2; for (pattern = 0 ; pattern < 8 ; pattern++) { unsigned long addr = 0x200001c0; passes = 0; printf("\nWalk START addr = 0x%x, Break pattern=%x\n", addr, pattern); while (addr <= 0x2000025c) { fprintf(stderr, "======================== %d: addr=%x ========================\n", passes, addr); memcpy(varp, starting_varp, sizeof(struct fb_var_screeninfo)); *(uint32_t*)addr = pattern; varp->activate = activate; printf("Pre FBIOPUT_VSCREENINFO: pattern=%x\n", pattern); dumpit((unsigned char *)vp, sizeof(struct fb_var_screeninfo), 0x200001c0); sleep(3); rtn = syscall(__NR_ioctl, r[0], 0x4601ul, 0x200001c0ul); if (rtn < 0) { perror("ioctl"); fprintf(stderr, "rtn=%d, errno=%d\n", rtn, errno); } addr += 4; passes++; if (passes == limit) break; } } close(fd);
return 0; }
With my patch it gets output like the following:
[root@localhost ~]# ./fb_break fd: 3 activate = 0
Walk START addr = 0x200001c0, Break pattern=0 ======================== 0: addr=200001c0 ======================== Pre FBIOPUT_VSCREENINFO: pattern=0 200001c0: 00 00 00 00 00 03 00 00 00 04 00 00 00 03 00 00 200001d0: 00 00 00 00 00 00 00 00 20 00 00 00 00 00 00 00 200001e0: 10 00 00 00 08 00 00 00 00 00 00 00 08 00 00 00 200001f0: 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00 00 20000200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 20000210: 00 00 00 00 00 00 00 00 2c 01 00 00 90 01 00 00 20000220: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 20000230: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 20000240: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 20000250: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ioctl: Invalid argument rtn=-1, errno=22 ======================== 1: addr=200001c4 ======================== Pre FBIOPUT_VSCREENINFO: pattern=0 200001c0: 00 04 00 00 00 00 00 00 00 04 00 00 00 03 00 00 200001d0: 00 00 00 00 00 00 00 00 20 00 00 00 00 00 00 00 200001e0: 10 00 00 00 08 00 00 00 00 00 00 00 08 00 00 00 200001f0: 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00 00 20000200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 20000210: 00 00 00 00 00 00 00 00 2c 01 00 00 90 01 00 00 20000220: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 20000230: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 20000240: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 20000250: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ioctl: Invalid argument rtn=-1, errno=22
Walk START addr = 0x200001c0, Break pattern=1 ======================== 0: addr=200001c0 ======================== Pre FBIOPUT_VSCREENINFO: pattern=1 200001c0: 01 00 00 00 00 03 00 00 00 04 00 00 00 03 00 00 200001d0: 00 00 00 00 00 00 00 00 20 00 00 00 00 00 00 00 200001e0: 10 00 00 00 08 00 00 00 00 00 00 00 08 00 00 00 200001f0: 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00 00 20000200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 20000210: 00 00 00 00 00 00 00 00 2c 01 00 00 90 01 00 00 20000220: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 20000230: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 20000240: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 20000250: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ioctl: Invalid argument rtn=-1, errno=22
...
======================== 1: addr=200001c4 ======================== Pre FBIOPUT_VSCREENINFO: pattern=7 200001c0: 00 04 00 00 07 00 00 00 00 04 00 00 00 03 00 00 200001d0: 00 00 00 00 00 00 00 00 20 00 00 00 00 00 00 00 200001e0: 10 00 00 00 08 00 00 00 00 00 00 00 08 00 00 00 200001f0: 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00 00 20000200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 20000210: 00 00 00 00 00 00 00 00 2c 01 00 00 90 01 00 00 20000220: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 20000230: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 20000240: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 20000250: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ioctl: Invalid argument rtn=-1, errno=22 [root@localhost ~]#
Thank you, George
On 7/14/2020 6:27 AM, Tetsuo Handa wrote:
On 2020/07/14 16:22, Bartlomiej Zolnierkiewicz wrote:
How does this patch relate to:
https://marc.info/?l=linux-fbdev&m=159415024816722&w=2
?
It seems to address the same issue, I've added George and Dan to Cc:.
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
--- a/drivers/tty/vt/vt.c +++ b/drivers/tty/vt/vt.c @@ -1214,6 +1214,10 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc,
if (new_screen_size > KMALLOC_MAX_SIZE) return -EINVAL;
- if (!strcmp(current->comm, "a.out")) {
printk(KERN_INFO "Forcing memory allocation failure.\n");return -ENOMEM;- } newscreen = kzalloc(new_screen_size, GFP_USER); if (!newscreen) return -ENOMEM;
. What my patch workarounds is cases when vc_do_resize() did not update vc->vc_{cols,rows} . Unless vc->vc_{cols,rows} are updated by vc_do_resize() in a way that avoids integer underflow at
unsigned int rw = info->var.xres - (vc->vc_cols*cw); unsigned int bh = info->var.yres - (vc->vc_rows*ch);
, this crash won't go away.
[ 39.995757][ T2788] Forcing memory allocation failure. [ 39.996527][ T2788] BUG: unable to handle page fault for address: ffffa9d180d7b000 [ 39.996529][ T2788] #PF: supervisor write access in kernel mode [ 39.996530][ T2788] #PF: error_code(0x0002) - not-present page [ 39.996531][ T2788] PGD 13a48c067 P4D 13a48c067 PUD 13a48d067 PMD 1324e4067 PTE 0 [ 39.996547][ T2788] Oops: 0002 [#1] SMP [ 39.996550][ T2788] CPU: 2 PID: 2788 Comm: a.out Not tainted 5.8.0-rc5+ #757 [ 39.996551][ T2788] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020 [ 39.996555][ T2788] RIP: 0010:bitfill_aligned+0x87/0x120 [cfbfillrect]
On 2020/07/15 2:15, George Kennedy wrote:
Can you try the a.out built from the original Syzkaller modified repro C program? It walks 0-7 through xres and yres of the fb_var_screeninfo struct.
I'm not familiar with exploit code. What do you want to explain via this program?
struct fb_var_screeninfo *varp = (struct fb_var_screeninfo *)0x200001c0; struct fb_var_screeninfo *starting_varp = malloc(sizeof(struct fb_var_screeninfo *));
memcpy(starting_varp, varp, sizeof(struct fb_var_screeninfo));
memcpy(varp, starting_varp, sizeof(struct fb_var_screeninfo));
At least, I suspect there is a memory corruption bug in this program because of malloc()ing only sizeof(struct fb_var_screeninfo *) bytes.
On Sun, Jul 12, 2020 at 08:10:12PM +0900, Tetsuo Handa wrote:
[...] @@ -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;
dri-devel@lists.freedesktop.org
-
Bartlomiej Zolnierkiewicz -
Dan Carpenter -
Daniel Vetter -
George Kennedy -
Greg Kroah-Hartman -
Kees Cook -
Tetsuo Handa -
Tetsuo Handa