[PATCH] vt: Reject zero-sized screen buffer size.

[PATCH] vt: Reject zero-sized screen buffer size.

[Tetsuo Handa]

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 being passed
to ioctl(FBIOPUT_VSCREENINFO) request on /dev/fb0 , for gotoxy(vc, 0, 0)
 from reset_terminal() from vc_init() from vc_allocate() 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 and/or 0 row makes sense, and
I think that we can reject such bogus arguments in fb_set_var() from
ioctl(FBIOPUT_VSCREENINFO). Regardless, I think that it is safer to also
check ZERO_SIZE_PTR when allocating vc->vc_screenbuf from vc_allocate()
 from con_install() from tty_init_dev() from tty_open().

[1] https://syzkaller.appspot.com/bug?extid=017265e8553724e514e8

Reported-by: syzbot <syzbot+017265e8553724e514e8@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 drivers/tty/vt/vt.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 48a8199f7845..8497e9206607 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -1126,7 +1126,7 @@ int vc_allocate(unsigned int currcons)	/* return 0 on success */
 		con_set_default_unimap(vc);
 
 	vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_KERNEL);
-	if (!vc->vc_screenbuf)
+	if (ZERO_OR_NULL_PTR(vc->vc_screenbuf))
 		goto err_free;
 
 	/* If no drivers have overridden us and the user didn't pass a
@@ -1212,7 +1212,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 +3393,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_screenbuf_size is sane here, for this is __init code. */
 		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);
-- 
2.18.4

fbconsole needs more parameter validations.

[Tetsuo Handa]

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.

Re: [PATCH] vt: Reject zero-sized screen buffer size.

[Greg Kroah-Hartman]

On Fri, Jul 10, 2020 at 02:53:29PM +0900, Tetsuo Handa wrote:
> 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 being passed
> to ioctl(FBIOPUT_VSCREENINFO) request on /dev/fb0 , for gotoxy(vc, 0, 0)
>  from reset_terminal() from vc_init() from vc_allocate() 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 and/or 0 row makes sense, and
> I think that we can reject such bogus arguments in fb_set_var() from
> ioctl(FBIOPUT_VSCREENINFO). Regardless, I think that it is safer to also
> check ZERO_SIZE_PTR when allocating vc->vc_screenbuf from vc_allocate()
>  from con_install() from tty_init_dev() from tty_open().
> 
> [1] https://syzkaller.appspot.com/bug?extid=017265e8553724e514e8
> 
> Reported-by: syzbot <syzbot+017265e8553724e514e8@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  drivers/tty/vt/vt.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 48a8199f7845..8497e9206607 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -1126,7 +1126,7 @@ int vc_allocate(unsigned int currcons)	/* return 0 on success */
>  		con_set_default_unimap(vc);
>  
>  	vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_KERNEL);
> -	if (!vc->vc_screenbuf)
> +	if (ZERO_OR_NULL_PTR(vc->vc_screenbuf))

No, let's check this before we do kzalloc() please, that's just an odd
way of doing an allocation we shouldn't have had to do.

>  		goto err_free;
>  
>  	/* If no drivers have overridden us and the user didn't pass a
> @@ -1212,7 +1212,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 +3393,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_screenbuf_size is sane here, for this is __init code. */

Shouldn't we also check this here, or before we get here, too?

Just checking the values and rejecting that as a valid screen size
should be sufficient.

thanks,

greg k-h

Re: fbconsole needs more parameter validations.

[Greg Kroah-Hartman]

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);
>  		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.


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

Re: [PATCH] vt: Reject zero-sized screen buffer size.

[Tetsuo Handa]

On 2020/07/10 19:55, Greg Kroah-Hartman wrote:
>> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
>> index 48a8199f7845..8497e9206607 100644
>> --- a/drivers/tty/vt/vt.c
>> +++ b/drivers/tty/vt/vt.c
>> @@ -1126,7 +1126,7 @@ int vc_allocate(unsigned int currcons)	/* return 0 on success */
>>  		con_set_default_unimap(vc);
>>  
>>  	vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_KERNEL);
>> -	if (!vc->vc_screenbuf)
>> +	if (ZERO_OR_NULL_PTR(vc->vc_screenbuf))
> 
> No, let's check this before we do kzalloc() please, that's just an odd
> way of doing an allocation we shouldn't have had to do.

OK. I can change to

+	if (vc->vc_screenbuf_size > KMALLOC_MAX_SIZE || !vc->vc_screenbuf_size)
+		goto err_free;
 	vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_KERNEL);
 	if (!vc->vc_screenbuf)
 		goto err_free;

like vc_do_resize() does. But I'm currently waiting for syzbot to test this patch, for
I don't have an environment for reproducing this problem.

> 
>>  		goto err_free;
>>  
>>  	/* If no drivers have overridden us and the user didn't pass a
>> @@ -1212,7 +1212,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 +3393,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_screenbuf_size is sane here, for this is __init code. */
> 
> Shouldn't we also check this here, or before we get here, too?

This is an __init function. Can we somehow pass column=0 or row=0 ?

> 
> Just checking the values and rejecting that as a valid screen size
> should be sufficient.

Hmm, where are we checking that column * row does not exceed UINT_MAX, given that
"struct vc_data"->vc_{cols,rows,screenbuf_size} are "unsigned int" and we do

  vc->vc_size_row = vc->vc_cols << 1;
  vc->vc_screenbuf_size = vc->vc_rows * vc->vc_size_row;

in visual_init() ? Don't we need to reject earlier?

Re: [PATCH] vt: Reject zero-sized screen buffer size.

[Greg Kroah-Hartman]

On Fri, Jul 10, 2020 at 08:31:42PM +0900, Tetsuo Handa wrote:
> On 2020/07/10 19:55, Greg Kroah-Hartman wrote:
> >> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> >> index 48a8199f7845..8497e9206607 100644
> >> --- a/drivers/tty/vt/vt.c
> >> +++ b/drivers/tty/vt/vt.c
> >> @@ -1126,7 +1126,7 @@ int vc_allocate(unsigned int currcons)	/* return 0 on success */
> >>  		con_set_default_unimap(vc);
> >>  
> >>  	vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_KERNEL);
> >> -	if (!vc->vc_screenbuf)
> >> +	if (ZERO_OR_NULL_PTR(vc->vc_screenbuf))
> > 
> > No, let's check this before we do kzalloc() please, that's just an odd
> > way of doing an allocation we shouldn't have had to do.
> 
> OK. I can change to
> 
> +	if (vc->vc_screenbuf_size > KMALLOC_MAX_SIZE || !vc->vc_screenbuf_size)
> +		goto err_free;
>  	vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_KERNEL);
>  	if (!vc->vc_screenbuf)
>  		goto err_free;
> 
> like vc_do_resize() does. But I'm currently waiting for syzbot to test this patch, for
> I don't have an environment for reproducing this problem.

That looks much more sane, thanks.


> 
> > 
> >>  		goto err_free;
> >>  
> >>  	/* If no drivers have overridden us and the user didn't pass a
> >> @@ -1212,7 +1212,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 +3393,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_screenbuf_size is sane here, for this is __init code. */
> > 
> > Shouldn't we also check this here, or before we get here, too?
> 
> This is an __init function. Can we somehow pass column=0 or row=0 ?

You could, it's much less likely, but why not catch this if you can?

> > Just checking the values and rejecting that as a valid screen size
> > should be sufficient.
> 
> Hmm, where are we checking that column * row does not exceed UINT_MAX, given that
> "struct vc_data"->vc_{cols,rows,screenbuf_size} are "unsigned int" and we do
> 
>   vc->vc_size_row = vc->vc_cols << 1;
>   vc->vc_screenbuf_size = vc->vc_rows * vc->vc_size_row;
> 
> in visual_init() ? Don't we need to reject earlier?

Probably, it's some twisty code :(

[PATCH v2] vt: Reject zero-sized screen buffer size.

[Tetsuo Handa]

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 being passed
to ioctl(FBIOPUT_VSCREENINFO) request on /dev/fb0 , for gotoxy(vc, 0, 0)
 from reset_terminal() from vc_init() from vc_allocate() 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 and/or 0 row makes sense, and
I think that we can reject such bogus arguments in fb_set_var() from
ioctl(FBIOPUT_VSCREENINFO). Regardless, I think that it is safer to also
check vc->vc_screenbuf_size when allocating vc->vc_screenbuf from
vc_allocate() from con_install() from tty_init_dev() from tty_open().

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().

But since vc_do_resize() requires cols <= 32767 and rows <= 32767,
applying 1 <= cols <= 32767 and 1 <= rows <= 32767 requirements to
vc_allocate() will be practically fine. (cols != 0 && rows != 0 is
implicitly checked via screenbuf_size != 0.)

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-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);
-- 
2.18.4

Re: fbconsole needs more parameter validations.

[Tetsuo Handa]

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, &region);
 	}
 
@@ -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, &region);
 	}
 }
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]
----------

Re: fbconsole needs more parameter validations.

[Tetsuo Handa]

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, &region);
 	}
 
-	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, &region);
 	}
 
-	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, &region);
 	}
 
-	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, &region);
 	}
 
-	if (bh) {
+	if ((int) bh > 0) {
 		region.dy = info->var.yoffset;
 		region.dx = info->var.xoffset;
                 region.height  = bh;

[PATCH v3] vt: Reject zero-sized screen buffer size.

[Tetsuo Handa]

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);
-- 
2.18.4

[PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.

[Tetsuo Handa]

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=91ecc3bf32ab1a551c33a39dab7fc0c8cd7b7e16

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, &region);
 	}
 
-	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, &region);
 	}
 
-	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, &region);
 	}
 
-	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, &region);
 	}
 
-	if (bh) {
+	if ((int) bh > 0) {
 		region.dy = info->var.yoffset;
 		region.dx = info->var.xoffset;
                 region.height  = bh;
-- 
2.18.4

Re: [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.

[Bartlomiej Zolnierkiewicz]

[ 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=91ecc3bf32ab1a551c33a39dab7fc0c8cd7b7e16
> 
> 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, &region);
>  	}
>  
> -	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, &region);
>  	}
>  
> -	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, &region);
>  	}
>  
> -	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, &region);
>  	}
>  
> -	if (bh) {
> +	if ((int) bh > 0) {
>  		region.dy = info->var.yoffset;
>  		region.dx = info->var.xoffset;
>                  region.height  = bh;
> 

Re: [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.

[Tetsuo Handa]

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]

Re: [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.

[Tetsuo Handa]

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 ?)

Re: [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.

[Tetsuo Handa]

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.

[PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.

[Tetsuo Handa]

syzbot is reporting general protection fault in bitfill_aligned() [1]
caused by integer underflow in bit_clear_margins(). The cause of this
problem is when and how do_vc_resize() updates vc->vc_{cols,rows}.

If vc_do_resize() fails (e.g. kzalloc() fails) when var.xres or var.yres
is going to shrink, vc->vc_{cols,rows} will not be updated. This allows
bit_clear_margins() to see info->var.xres < (vc->vc_cols * cw) or
info->var.yres < (vc->vc_rows * ch). Unexpectedly large rw or bh will
try to overrun the __iomem region and causes general protection fault.

Also, vc_resize(vc, 0, 0) does not set vc->vc_{cols,rows} = 0 due to

  new_cols = (cols ? cols : vc->vc_cols);
  new_rows = (lines ? lines : vc->vc_rows);

exception. Since cols and lines are calculated as

  cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
  rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
  cols /= vc->vc_font.width;
  rows /= vc->vc_font.height;
  vc_resize(vc, cols, rows);

in fbcon_modechanged(), var.xres < vc->vc_font.width makes cols = 0
and var.yres < vc->vc_font.height makes rows = 0. This means that

  const int fd = open("/dev/fb0", O_ACCMODE);
  struct fb_var_screeninfo var = { };
  ioctl(fd, FBIOGET_VSCREENINFO, &var);
  var.xres = var.yres = 1;
  ioctl(fd, FBIOPUT_VSCREENINFO, &var);

easily reproduces integer underflow bug explained above.

Of course, callers of vc_resize() are not handling vc_do_resize() failure
is bad. But we can't avoid vc_resize(vc, 0, 0) which returns 0. Therefore,
as a band-aid workaround, this patch checks integer underflow in
"struct fbcon_ops"->clear_margins call, assuming that
vc->vc_cols * vc->vc_font.width and vc->vc_rows * vc->vc_font.heigh do not
cause integer overflow.

[1] https://syzkaller.appspot.com/bug?id=a565882df74fa76f10d3a6fec4be31098dbb37c6

Reported-and-tested-by: syzbot <syzbot+e5fd3e65515b48c02a30@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 drivers/video/fbdev/core/bitblit.c   | 4 ++--
 drivers/video/fbdev/core/fbcon_ccw.c | 4 ++--
 drivers/video/fbdev/core/fbcon_cw.c  | 4 ++--
 drivers/video/fbdev/core/fbcon_ud.c  | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
index ca935c09a261..35ebeeccde4d 100644
--- a/drivers/video/fbdev/core/bitblit.c
+++ b/drivers/video/fbdev/core/bitblit.c
@@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
 	region.color = color;
 	region.rop = ROP_COPY;
 
-	if (rw && !bottom_only) {
+	if ((int) rw > 0 && !bottom_only) {
 		region.dx = info->var.xoffset + rs;
 		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, &region);
 	}
 
-	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, &region);
 	}
 
-	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, &region);
 	}
 
-	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, &region);
 	}
 
-	if (bh) {
+	if ((int) bh > 0) {
 		region.dy = info->var.yoffset;
 		region.dx = info->var.xoffset;
                 region.height  = bh;
-- 
2.18.4

Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.

[Dan Carpenter]

On Wed, Jul 15, 2020 at 10:51:02AM +0900, Tetsuo Handa wrote:
> syzbot is reporting general protection fault in bitfill_aligned() [1]
> caused by integer underflow in bit_clear_margins(). The cause of this
> problem is when and how do_vc_resize() updates vc->vc_{cols,rows}.
> 
> If vc_do_resize() fails (e.g. kzalloc() fails) when var.xres or var.yres
> is going to shrink, vc->vc_{cols,rows} will not be updated. This allows
> bit_clear_margins() to see info->var.xres < (vc->vc_cols * cw) or
> info->var.yres < (vc->vc_rows * ch). Unexpectedly large rw or bh will
> try to overrun the __iomem region and causes general protection fault.
> 
> Also, vc_resize(vc, 0, 0) does not set vc->vc_{cols,rows} = 0 due to
> 
>   new_cols = (cols ? cols : vc->vc_cols);
>   new_rows = (lines ? lines : vc->vc_rows);
> 
> exception. Since cols and lines are calculated as
> 
>   cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
>   rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
>   cols /= vc->vc_font.width;
>   rows /= vc->vc_font.height;
>   vc_resize(vc, cols, rows);
> 
> in fbcon_modechanged(), var.xres < vc->vc_font.width makes cols = 0
> and var.yres < vc->vc_font.height makes rows = 0. This means that
> 
>   const int fd = open("/dev/fb0", O_ACCMODE);
>   struct fb_var_screeninfo var = { };
>   ioctl(fd, FBIOGET_VSCREENINFO, &var);
>   var.xres = var.yres = 1;
>   ioctl(fd, FBIOPUT_VSCREENINFO, &var);
> 
> easily reproduces integer underflow bug explained above.
> 
> Of course, callers of vc_resize() are not handling vc_do_resize() failure
> is bad. But we can't avoid vc_resize(vc, 0, 0) which returns 0. Therefore,
> as a band-aid workaround, this patch checks integer underflow in
> "struct fbcon_ops"->clear_margins call, assuming that
> vc->vc_cols * vc->vc_font.width and vc->vc_rows * vc->vc_font.heigh do not
> cause integer overflow.
> 
> [1] https://syzkaller.appspot.com/bug?id=a565882df74fa76f10d3a6fec4be31098dbb37c6
> 
> Reported-and-tested-by: syzbot <syzbot+e5fd3e65515b48c02a30@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  drivers/video/fbdev/core/bitblit.c   | 4 ++--
>  drivers/video/fbdev/core/fbcon_ccw.c | 4 ++--
>  drivers/video/fbdev/core/fbcon_cw.c  | 4 ++--
>  drivers/video/fbdev/core/fbcon_ud.c  | 4 ++--
>  4 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
> index ca935c09a261..35ebeeccde4d 100644
> --- a/drivers/video/fbdev/core/bitblit.c
> +++ b/drivers/video/fbdev/core/bitblit.c
> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
>  	region.color = color;
>  	region.rop = ROP_COPY;
>  
> -	if (rw && !bottom_only) {
> +	if ((int) rw > 0 && !bottom_only) {
>  		region.dx = info->var.xoffset + rs;
                            ^^^^^^^^^^^^^^^^^^^^^^

If you choose a very high positive "rw" then this addition can overflow.
info->var.xoffset comes from the user and I don't think it's checked...

regards,
dan carpenter

Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.

[Tetsuo Handa]

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.

Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.

[Tetsuo Handa]

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
----------

Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.

[Dan Carpenter]

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);
 

Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.

[Tetsuo Handa]

On 2020/07/16 0:12, Dan Carpenter wrote:
> I've complained about integer overflows in fbdev for a long time...
> 
> What I'd like to see is something like the following maybe.  I don't
> know how to get the vc_data in fbmem.c so it doesn't include your checks
> for negative.

Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
we want basic checks. That's a task for fbdev people who should be familiar with
necessary constraints.

Anyway, my two patches are small and low cost; can we apply these patches regardless
of basic checks?

Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.

[Daniel Vetter]

On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
> On 2020/07/16 0:12, Dan Carpenter wrote:
> > I've complained about integer overflows in fbdev for a long time...
> > 
> > What I'd like to see is something like the following maybe.  I don't
> > know how to get the vc_data in fbmem.c so it doesn't include your checks
> > for negative.
> 
> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
> we want basic checks. That's a task for fbdev people who should be familiar with
> necessary constraints.

I think the worldwide supply of people who understand fbdev and willing to
work on it is roughly 0. So if someone wants to fix this mess properly
(which likely means adding tons of over/underflow checks at entry points,
since you're never going to catch the driver bugs, there's too many and
not enough people who care) they need to fix this themselves.

Just to avoid confusion here.

> Anyway, my two patches are small and low cost; can we apply these patches regardless
> of basic checks?

Which two patches where?

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.

[Tetsuo Handa]

On 2020/07/16 19:00, Daniel Vetter wrote:
> On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
>> On 2020/07/16 0:12, Dan Carpenter wrote:
>>> I've complained about integer overflows in fbdev for a long time...
>>>
>>> What I'd like to see is something like the following maybe.  I don't
>>> know how to get the vc_data in fbmem.c so it doesn't include your checks
>>> for negative.
>>
>> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
>> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
>> we want basic checks. That's a task for fbdev people who should be familiar with
>> necessary constraints.
> 
> I think the worldwide supply of people who understand fbdev and willing to
> work on it is roughly 0. So if someone wants to fix this mess properly
> (which likely means adding tons of over/underflow checks at entry points,
> since you're never going to catch the driver bugs, there's too many and
> not enough people who care) they need to fix this themselves.

But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks()
(which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for
"32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only
rows and cols < 32768 ?

> 
> Just to avoid confusion here.
> 
>> Anyway, my two patches are small and low cost; can we apply these patches regardless
>> of basic checks?
> 
> Which two patches where?

[PATCH v3] vt: Reject zero-sized screen buffer size.
 from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp

[PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
 from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp

Re: [PATCH v2] vt: Reject zero-sized screen buffer size.

[Brooke Basile]

On 7/10/20 10:34 AM, Tetsuo Handa wrote:
> 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 being passed
> to ioctl(FBIOPUT_VSCREENINFO) request on /dev/fb0 , for gotoxy(vc, 0, 0)
>   from reset_terminal() from vc_init() from vc_allocate() 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 and/or 0 row makes sense, and
> I think that we can reject such bogus arguments in fb_set_var() from
> ioctl(FBIOPUT_VSCREENINFO). Regardless, I think that it is safer to also
> check vc->vc_screenbuf_size when allocating vc->vc_screenbuf from
> vc_allocate() from con_install() from tty_init_dev() from tty_open().
> 
> 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().
> 
> But since vc_do_resize() requires cols <= 32767 and rows <= 32767,
> applying 1 <= cols <= 32767 and 1 <= rows <= 32767 requirements to
> vc_allocate() will be practically fine. (cols != 0 && rows != 0 is
> implicitly checked via screenbuf_size != 0.)
> 
> 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-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);
> 

Hi,

Looks like this patch also fixes this bug reported by syzbot:
	https://syzkaller.appspot.com/bug?id=dc5c6b1ae4952a5d72d0e82de0eeeb9e5f767efc

There's a lot of other bugs that were reported by syzbot that also touch 
this code, so I just wanted to give a heads up in case you weren't 
already aware of them.  It seems like this patch could be a fix for all 
of them.

Here are the links to those other bugs:
	https://syzkaller.appspot.com/bug?id=3e2ad4922b18026c1579f50900747401842acdff
	https://syzkaller.appspot.com/bug?id=7329638ab83b70fc8fab07e14c4b2fcdc73af21d
	https://syzkaller.appspot.com/bug?id=01703eb07363bd1f9757bc4a54994455fc9db9dc
	https://syzkaller.appspot.com/bug?id=7a04be77a06aae337077e00f0ecdb2239dfc2fc3
	https://syzkaller.appspot.com/bug?id=ff1543b5ade351b9d6c4ef51c805d89422a8271d

Best,
Brooke Basile

Re: [PATCH v2] vt: Reject zero-sized screen buffer size.

[Tetsuo Handa]

On 2020/07/21 0:40, Brooke Basile wrote:
> Looks like this patch also fixes this bug reported by syzbot:
>     https://syzkaller.appspot.com/bug?id=dc5c6b1ae4952a5d72d0e82de0eeeb9e5f767efc
> 
> There's a lot of other bugs that were reported by syzbot that also touch this code, so I just wanted to give a heads up in case you weren't already aware of them.  It seems like this patch could be a fix for all of them.
> 
> Here are the links to those other bugs:
>     https://syzkaller.appspot.com/bug?id=3e2ad4922b18026c1579f50900747401842acdff
>     https://syzkaller.appspot.com/bug?id=7329638ab83b70fc8fab07e14c4b2fcdc73af21d
>     https://syzkaller.appspot.com/bug?id=01703eb07363bd1f9757bc4a54994455fc9db9dc
>     https://syzkaller.appspot.com/bug?id=7a04be77a06aae337077e00f0ecdb2239dfc2fc3
>     https://syzkaller.appspot.com/bug?id=ff1543b5ade351b9d6c4ef51c805d89422a8271d
> 

Indeed they all access around UINT_MAX address. Marked as dup. Thank you.

Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.

[Greg Kroah-Hartman]

On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
> On 2020/07/16 19:00, Daniel Vetter wrote:
> > On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
> >> On 2020/07/16 0:12, Dan Carpenter wrote:
> >>> I've complained about integer overflows in fbdev for a long time...
> >>>
> >>> What I'd like to see is something like the following maybe.  I don't
> >>> know how to get the vc_data in fbmem.c so it doesn't include your checks
> >>> for negative.
> >>
> >> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
> >> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
> >> we want basic checks. That's a task for fbdev people who should be familiar with
> >> necessary constraints.
> > 
> > I think the worldwide supply of people who understand fbdev and willing to
> > work on it is roughly 0. So if someone wants to fix this mess properly
> > (which likely means adding tons of over/underflow checks at entry points,
> > since you're never going to catch the driver bugs, there's too many and
> > not enough people who care) they need to fix this themselves.
> 
> But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks()
> (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for
> "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only
> rows and cols < 32768 ?
> 
> > 
> > Just to avoid confusion here.
> > 
> >> Anyway, my two patches are small and low cost; can we apply these patches regardless
> >> of basic checks?
> > 
> > Which two patches where?
> 
> [PATCH v3] vt: Reject zero-sized screen buffer size.
>  from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp

This is now in my tree.

> [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
>  from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp

That should be taken by the fbdev maintainer, but I can take it too if
people want.

thanks,

greg k-h

Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.

[Daniel Vetter]

On Tue, Jul 21, 2020 at 6:08 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
> > On 2020/07/16 19:00, Daniel Vetter wrote:
> > > On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
> > >> On 2020/07/16 0:12, Dan Carpenter wrote:
> > >>> I've complained about integer overflows in fbdev for a long time...
> > >>>
> > >>> What I'd like to see is something like the following maybe.  I don't
> > >>> know how to get the vc_data in fbmem.c so it doesn't include your checks
> > >>> for negative.
> > >>
> > >> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
> > >> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
> > >> we want basic checks. That's a task for fbdev people who should be familiar with
> > >> necessary constraints.
> > >
> > > I think the worldwide supply of people who understand fbdev and willing to
> > > work on it is roughly 0. So if someone wants to fix this mess properly
> > > (which likely means adding tons of over/underflow checks at entry points,
> > > since you're never going to catch the driver bugs, there's too many and
> > > not enough people who care) they need to fix this themselves.
> >
> > But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks()
> > (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for
> > "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only
> > rows and cols < 32768 ?
> >
> > >
> > > Just to avoid confusion here.
> > >
> > >> Anyway, my two patches are small and low cost; can we apply these patches regardless
> > >> of basic checks?
> > >
> > > Which two patches where?
> >
> > [PATCH v3] vt: Reject zero-sized screen buffer size.
> >  from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp
>
> This is now in my tree.
>
> > [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
> >  from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp
>
> That should be taken by the fbdev maintainer, but I can take it too if
> people want.

Just missed this weeks pull request train and feeling like not worth
making this an exception (it's been broken forever after all), so
maybe best if you just add this to vt.

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Also this avoids the impression I know what's going on in fbdev code,
maybe with sufficient abandon from my side someone will pop up who
cares an fixes the bazillion of syzkaller issues we seem to have
around console/vt and everything related.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.

[Greg Kroah-Hartman]

On Wed, Jul 22, 2020 at 10:07:06AM +0200, Daniel Vetter wrote:
> On Tue, Jul 21, 2020 at 6:08 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
> > > On 2020/07/16 19:00, Daniel Vetter wrote:
> > > > On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
> > > >> On 2020/07/16 0:12, Dan Carpenter wrote:
> > > >>> I've complained about integer overflows in fbdev for a long time...
> > > >>>
> > > >>> What I'd like to see is something like the following maybe.  I don't
> > > >>> know how to get the vc_data in fbmem.c so it doesn't include your checks
> > > >>> for negative.
> > > >>
> > > >> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
> > > >> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
> > > >> we want basic checks. That's a task for fbdev people who should be familiar with
> > > >> necessary constraints.
> > > >
> > > > I think the worldwide supply of people who understand fbdev and willing to
> > > > work on it is roughly 0. So if someone wants to fix this mess properly
> > > > (which likely means adding tons of over/underflow checks at entry points,
> > > > since you're never going to catch the driver bugs, there's too many and
> > > > not enough people who care) they need to fix this themselves.
> > >
> > > But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks()
> > > (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for
> > > "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only
> > > rows and cols < 32768 ?
> > >
> > > >
> > > > Just to avoid confusion here.
> > > >
> > > >> Anyway, my two patches are small and low cost; can we apply these patches regardless
> > > >> of basic checks?
> > > >
> > > > Which two patches where?
> > >
> > > [PATCH v3] vt: Reject zero-sized screen buffer size.
> > >  from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp
> >
> > This is now in my tree.
> >
> > > [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
> > >  from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp
> >
> > That should be taken by the fbdev maintainer, but I can take it too if
> > people want.
> 
> Just missed this weeks pull request train and feeling like not worth
> making this an exception (it's been broken forever after all), so
> maybe best if you just add this to vt.
> 
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Also this avoids the impression I know what's going on in fbdev code,
> maybe with sufficient abandon from my side someone will pop up who
> cares an fixes the bazillion of syzkaller issues we seem to have
> around console/vt and everything related.

Great, will go queue it up now, thanks!

greg k-h

Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.

[Bartlomiej Zolnierkiewicz]

On 7/23/20 4:21 PM, Greg Kroah-Hartman wrote:
> On Wed, Jul 22, 2020 at 10:07:06AM +0200, Daniel Vetter wrote:
>> On Tue, Jul 21, 2020 at 6:08 PM Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>>>
>>> On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
>>>> On 2020/07/16 19:00, Daniel Vetter wrote:
>>>>> On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
>>>>>> On 2020/07/16 0:12, Dan Carpenter wrote:
>>>>>>> I've complained about integer overflows in fbdev for a long time...
>>>>>>>
>>>>>>> What I'd like to see is something like the following maybe.  I don't
>>>>>>> know how to get the vc_data in fbmem.c so it doesn't include your checks
>>>>>>> for negative.
>>>>>>
>>>>>> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
>>>>>> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
>>>>>> we want basic checks. That's a task for fbdev people who should be familiar with
>>>>>> necessary constraints.
>>>>>
>>>>> I think the worldwide supply of people who understand fbdev and willing to
>>>>> work on it is roughly 0. So if someone wants to fix this mess properly
>>>>> (which likely means adding tons of over/underflow checks at entry points,
>>>>> since you're never going to catch the driver bugs, there's too many and
>>>>> not enough people who care) they need to fix this themselves.
>>>>
>>>> But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks()
>>>> (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for
>>>> "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only
>>>> rows and cols < 32768 ?
>>>>
>>>>>
>>>>> Just to avoid confusion here.
>>>>>
>>>>>> Anyway, my two patches are small and low cost; can we apply these patches regardless
>>>>>> of basic checks?
>>>>>
>>>>> Which two patches where?
>>>>
>>>> [PATCH v3] vt: Reject zero-sized screen buffer size.
>>>>  from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp
>>>
>>> This is now in my tree.
>>>
>>>> [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
>>>>  from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp
>>>
>>> That should be taken by the fbdev maintainer, but I can take it too if
>>> people want.
>>
>> Just missed this weeks pull request train and feeling like not worth
>> making this an exception (it's been broken forever after all), so
>> maybe best if you just add this to vt.
>>
>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> Also this avoids the impression I know what's going on in fbdev code,
>> maybe with sufficient abandon from my side someone will pop up who
>> cares an fixes the bazillion of syzkaller issues we seem to have
>> around console/vt and everything related.
> 
> Great, will go queue it up now, thanks!
Fine with me, thanks!

PS I'll later queue the patch from George to drm-misc-next (after
reading both fbdev patches in detail it seems that both are needed).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH v3] vt: Reject zero-sized screen buffer size.

[Kees Cook]

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;


-- 
Kees Cook