* [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect @ 2021-07-14 5:16 ` syzbot 0 siblings, 0 replies; 38+ messages in thread From: syzbot @ 2021-07-14 5:16 UTC (permalink / raw) To: akpm, b.zolnierkie, colin.king, dri-devel, linux-fbdev, linux-kernel, masahiroy, penguin-kernel, syzkaller-bugs Hello, syzbot found the following issue on: HEAD commit: 3dbdb38e Merge branch 'for-5.14' of git://git.kernel.org/p.. git tree: upstream console output: https://syzkaller.appspot.com/x/log.txt?x=1323c402300000 kernel config: https://syzkaller.appspot.com/x/.config?x=a1fcf15a09815757 dashboard link: https://syzkaller.appspot.com/bug?extid=04168c8063cfdde1db5e syz repro: https://syzkaller.appspot.com/x/repro.syz?x=11f0e772300000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1114b9b0300000 Bisection is inconclusive: the issue happens on the oldest tested release. bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=10fa45d8300000 final oops: https://syzkaller.appspot.com/x/report.txt?x=12fa45d8300000 console output: https://syzkaller.appspot.com/x/log.txt?x=14fa45d8300000 IMPORTANT: if you fix the issue, please add the following tag to the commit: Reported-by: syzbot+04168c8063cfdde1db5e@syzkaller.appspotmail.com BUG: unable to handle page fault for address: ffff888001000050 #PF: supervisor write access in kernel mode #PF: error_code(0x0003) - permissions violation PGD 10e01067 P4D 10e01067 PUD 10e02067 PMD 80000000010001e1 Oops: 0003 [#1] PREEMPT SMP KASAN CPU: 1 PID: 8433 Comm: syz-executor067 Tainted: G W 5.13.0-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 RIP: 0010:writeb arch/x86/include/asm/io.h:65 [inline] RIP: 0010:vga16fb_fillrect+0x993/0x18d0 drivers/video/fbdev/vga16fb.c:923 Code: 6c fd 48 63 44 24 10 45 31 f6 48 89 04 24 e8 44 a6 6c fd 31 ff 89 de 31 ed e8 79 ad 6c fd 85 db 4d 89 ec 74 22 e8 2d a6 6c fd <45> 88 34 24 83 c5 01 89 df 49 83 c4 01 89 ee e8 49 ae 6c fd 39 eb RSP: 0018:ffffc90000eff848 EFLAGS: 00010293 RAX: 0000000000000000 RBX: 000000000000001b RCX: 0000000000000000 RDX: ffff88802d949c40 RSI: ffffffff8408e403 RDI: 0000000000000003 RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffff8408dd8d R10: ffffffff8408e3f7 R11: 0000000000000000 R12: ffff888001000050 R13: ffff888001000050 R14: 0000000000000000 R15: 000000000ffeb7ff FS: 0000000001aa2300(0000) GS:ffff8880b9d00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: ffff888001000050 CR3: 00000000346fb000 CR4: 00000000001506e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: bit_clear_margins+0x3f6/0x4b0 drivers/video/fbdev/core/bitblit.c:224 fbcon_clear_margins+0x1f1/0x280 drivers/video/fbdev/core/fbcon.c:1315 fbcon_switch+0xa8c/0x1620 drivers/video/fbdev/core/fbcon.c:2146 redraw_screen+0x2b9/0x740 drivers/tty/vt/vt.c:1021 fbcon_modechanged+0x593/0x6d0 drivers/video/fbdev/core/fbcon.c:2651 fbcon_update_vcs+0x3a/0x50 drivers/video/fbdev/core/fbcon.c:2696 do_fb_ioctl+0x62e/0x690 drivers/video/fbdev/core/fbmem.c:1110 fb_ioctl+0xe7/0x150 drivers/video/fbdev/core/fbmem.c:1185 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:1069 [inline] __se_sys_ioctl fs/ioctl.c:1055 [inline] __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:1055 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x44/0xae RIP: 0033:0x43efd9 Code: 28 c3 e8 2a 14 00 00 66 2e 0f 1f 84 00 00 00 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48 RSP: 002b:00007ffc362df848 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 0000000000400488 RCX: 000000000043efd9 RDX: 0000000020000200 RSI: 0000000000004601 RDI: 0000000000000003 RBP: 0000000000402fc0 R08: 0000000000000000 R09: 0000000000400488 R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000403050 R13: 0000000000000000 R14: 00000000004ac018 R15: 0000000000400488 Modules linked in: CR2: ffff888001000050 ---[ end trace 39dce64bc5621bd3 ]--- RIP: 0010:writeb arch/x86/include/asm/io.h:65 [inline] RIP: 0010:vga16fb_fillrect+0x993/0x18d0 drivers/video/fbdev/vga16fb.c:923 Code: 6c fd 48 63 44 24 10 45 31 f6 48 89 04 24 e8 44 a6 6c fd 31 ff 89 de 31 ed e8 79 ad 6c fd 85 db 4d 89 ec 74 22 e8 2d a6 6c fd <45> 88 34 24 83 c5 01 89 df 49 83 c4 01 89 ee e8 49 ae 6c fd 39 eb RSP: 0018:ffffc90000eff848 EFLAGS: 00010293 RAX: 0000000000000000 RBX: 000000000000001b RCX: 0000000000000000 RDX: ffff88802d949c40 RSI: ffffffff8408e403 RDI: 0000000000000003 RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffff8408dd8d R10: ffffffff8408e3f7 R11: 0000000000000000 R12: ffff888001000050 R13: ffff888001000050 R14: 0000000000000000 R15: 000000000ffeb7ff FS: 0000000001aa2300(0000) GS:ffff8880b9d00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: ffff888001000050 CR3: 00000000346fb000 CR4: 00000000001506e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 --- This report is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at syzkaller@googlegroups.com. syzbot will keep track of this issue. See: https://goo.gl/tpsmEJ#status for how to communicate with syzbot. For information about bisection process see: https://goo.gl/tpsmEJ#bisection syzbot can test patches for this issue, for details see: https://goo.gl/tpsmEJ#testing-patches ^ permalink raw reply [flat|nested] 38+ messages in thread
* [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect @ 2021-07-14 5:16 ` syzbot 0 siblings, 0 replies; 38+ messages in thread From: syzbot @ 2021-07-14 5:16 UTC (permalink / raw) To: akpm, b.zolnierkie, colin.king, dri-devel, linux-fbdev, linux-kernel, masahiroy, penguin-kernel, syzkaller-bugs Hello, syzbot found the following issue on: HEAD commit: 3dbdb38e Merge branch 'for-5.14' of git://git.kernel.org/p.. git tree: upstream console output: https://syzkaller.appspot.com/x/log.txt?x=1323c402300000 kernel config: https://syzkaller.appspot.com/x/.config?x=a1fcf15a09815757 dashboard link: https://syzkaller.appspot.com/bug?extid=04168c8063cfdde1db5e syz repro: https://syzkaller.appspot.com/x/repro.syz?x=11f0e772300000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1114b9b0300000 Bisection is inconclusive: the issue happens on the oldest tested release. bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=10fa45d8300000 final oops: https://syzkaller.appspot.com/x/report.txt?x=12fa45d8300000 console output: https://syzkaller.appspot.com/x/log.txt?x=14fa45d8300000 IMPORTANT: if you fix the issue, please add the following tag to the commit: Reported-by: syzbot+04168c8063cfdde1db5e@syzkaller.appspotmail.com BUG: unable to handle page fault for address: ffff888001000050 #PF: supervisor write access in kernel mode #PF: error_code(0x0003) - permissions violation PGD 10e01067 P4D 10e01067 PUD 10e02067 PMD 80000000010001e1 Oops: 0003 [#1] PREEMPT SMP KASAN CPU: 1 PID: 8433 Comm: syz-executor067 Tainted: G W 5.13.0-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 RIP: 0010:writeb arch/x86/include/asm/io.h:65 [inline] RIP: 0010:vga16fb_fillrect+0x993/0x18d0 drivers/video/fbdev/vga16fb.c:923 Code: 6c fd 48 63 44 24 10 45 31 f6 48 89 04 24 e8 44 a6 6c fd 31 ff 89 de 31 ed e8 79 ad 6c fd 85 db 4d 89 ec 74 22 e8 2d a6 6c fd <45> 88 34 24 83 c5 01 89 df 49 83 c4 01 89 ee e8 49 ae 6c fd 39 eb RSP: 0018:ffffc90000eff848 EFLAGS: 00010293 RAX: 0000000000000000 RBX: 000000000000001b RCX: 0000000000000000 RDX: ffff88802d949c40 RSI: ffffffff8408e403 RDI: 0000000000000003 RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffff8408dd8d R10: ffffffff8408e3f7 R11: 0000000000000000 R12: ffff888001000050 R13: ffff888001000050 R14: 0000000000000000 R15: 000000000ffeb7ff FS: 0000000001aa2300(0000) GS:ffff8880b9d00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: ffff888001000050 CR3: 00000000346fb000 CR4: 00000000001506e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: bit_clear_margins+0x3f6/0x4b0 drivers/video/fbdev/core/bitblit.c:224 fbcon_clear_margins+0x1f1/0x280 drivers/video/fbdev/core/fbcon.c:1315 fbcon_switch+0xa8c/0x1620 drivers/video/fbdev/core/fbcon.c:2146 redraw_screen+0x2b9/0x740 drivers/tty/vt/vt.c:1021 fbcon_modechanged+0x593/0x6d0 drivers/video/fbdev/core/fbcon.c:2651 fbcon_update_vcs+0x3a/0x50 drivers/video/fbdev/core/fbcon.c:2696 do_fb_ioctl+0x62e/0x690 drivers/video/fbdev/core/fbmem.c:1110 fb_ioctl+0xe7/0x150 drivers/video/fbdev/core/fbmem.c:1185 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:1069 [inline] __se_sys_ioctl fs/ioctl.c:1055 [inline] __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:1055 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x44/0xae RIP: 0033:0x43efd9 Code: 28 c3 e8 2a 14 00 00 66 2e 0f 1f 84 00 00 00 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48 RSP: 002b:00007ffc362df848 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 0000000000400488 RCX: 000000000043efd9 RDX: 0000000020000200 RSI: 0000000000004601 RDI: 0000000000000003 RBP: 0000000000402fc0 R08: 0000000000000000 R09: 0000000000400488 R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000403050 R13: 0000000000000000 R14: 00000000004ac018 R15: 0000000000400488 Modules linked in: CR2: ffff888001000050 ---[ end trace 39dce64bc5621bd3 ]--- RIP: 0010:writeb arch/x86/include/asm/io.h:65 [inline] RIP: 0010:vga16fb_fillrect+0x993/0x18d0 drivers/video/fbdev/vga16fb.c:923 Code: 6c fd 48 63 44 24 10 45 31 f6 48 89 04 24 e8 44 a6 6c fd 31 ff 89 de 31 ed e8 79 ad 6c fd 85 db 4d 89 ec 74 22 e8 2d a6 6c fd <45> 88 34 24 83 c5 01 89 df 49 83 c4 01 89 ee e8 49 ae 6c fd 39 eb RSP: 0018:ffffc90000eff848 EFLAGS: 00010293 RAX: 0000000000000000 RBX: 000000000000001b RCX: 0000000000000000 RDX: ffff88802d949c40 RSI: ffffffff8408e403 RDI: 0000000000000003 RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffff8408dd8d R10: ffffffff8408e3f7 R11: 0000000000000000 R12: ffff888001000050 R13: ffff888001000050 R14: 0000000000000000 R15: 000000000ffeb7ff FS: 0000000001aa2300(0000) GS:ffff8880b9d00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: ffff888001000050 CR3: 00000000346fb000 CR4: 00000000001506e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 --- This report is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at syzkaller@googlegroups.com. syzbot will keep track of this issue. See: https://goo.gl/tpsmEJ#status for how to communicate with syzbot. For information about bisection process see: https://goo.gl/tpsmEJ#bisection syzbot can test patches for this issue, for details see: https://goo.gl/tpsmEJ#testing-patches ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect 2021-07-14 5:16 ` syzbot (?) @ 2021-08-30 0:24 ` Randy Dunlap 2021-08-30 2:27 ` Tetsuo Handa -1 siblings, 1 reply; 38+ messages in thread From: Randy Dunlap @ 2021-08-30 0:24 UTC (permalink / raw) To: syzbot, akpm, b.zolnierkie, colin.king, dri-devel, linux-fbdev, linux-kernel, masahiroy, penguin-kernel, syzkaller-bugs Cc: Geert Uytterhoeven On 7/13/21 10:16 PM, syzbot wrote: > Hello, > > syzbot found the following issue on: > > HEAD commit: 3dbdb38e Merge branch 'for-5.14' of git://git.kernel.org/p.. > git tree: upstream > console output: https://syzkaller.appspot.com/x/log.txt?x=1323c402300000 > kernel config: https://syzkaller.appspot.com/x/.config?x=a1fcf15a09815757 > dashboard link: https://syzkaller.appspot.com/bug?extid=04168c8063cfdde1db5e > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=11f0e772300000 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1114b9b0300000 Here are the fields that are set in the C reproducer for this ioctl: #define FBIOPUT_VSCREENINFO 0x4601 *(uint32_t*)0x20000200 = 0x356; // xres: 854 *(uint32_t*)0x20000204 = 8; // yres *(uint32_t*)0x20000208 = 0x600; // xres_virtual: 1536 *(uint32_t*)0x2000020c = 0x10000000; // yres_virtual: huge *(uint32_t*)0x20000210 = 0; // xoffset *(uint32_t*)0x20000214 = 0; // yoffset *(uint32_t*)0x20000218 = 4; // bits_per_pixel *(uint32_t*)0x2000021c = 0; // grayscale: false *(uint32_t*)0x20000220 = 0x3000000; // red bitfield *(uint32_t*)0x20000224 = 0; // green bitfield *(uint32_t*)0x20000228 = 0; // blue bitfield *(uint32_t*)0x2000022c = 0; // transp *(uint32_t*)0x20000230 = 0; // nonstd: false *(uint32_t*)0x20000234 = 0; // activate *(uint32_t*)0x20000238 = 0; // height *(uint32_t*)0x2000023c = 0; // width *(uint32_t*)0x20000240 = 0; // accel_flags *(uint32_t*)0x20000244 = 0; // pixclock *(uint32_t*)0x20000248 = 0; // left_margin *(uint32_t*)0x2000024c = 0; // right_margin *(uint32_t*)0x20000250 = 0; // upper_margin *(uint32_t*)0x20000254 = 0; // lower_margin *(uint32_t*)0x20000258 = 0; // hsync_len *(uint32_t*)0x2000025c = 0; // vsync_len *(uint32_t*)0x20000260 = 0; // sync *(uint32_t*)0x20000264 = 0; // vmode *(uint32_t*)0x20000268 = 0; // rotate *(uint32_t*)0x2000026c = 0; // colorspace *(uint32_t*)0x20000270 = 0; // rsvd0 *(uint32_t*)0x20000274 = 0; // rsvd1 *(uint32_t*)0x20000278 = 0; // rsvd2 *(uint32_t*)0x2000027c = 0; // rsvd3 *(uint32_t*)0x20000280 = 0; // notdef... *(uint32_t*)0x20000284 = 0; *(uint32_t*)0x20000288 = 0; *(uint32_t*)0x2000028c = 0; memset((void*)0x20000290, 0, 16); syscall(__NR_ioctl, r[0], 0x4601, 0x20000200ul); Note that yres_virtual is set to 0x10000000. Is there no practical limit (hence limit check) that can be used here? Also, in vga16fb_check_var(), beginning at line 404: 404 if (yres > vyres) 405 vyres = yres; 406 if (vxres * vyres > maxmem) { 407 vyres = maxmem / vxres; 408 if (vyres < yres) 409 return -ENOMEM; 410 } At line 406, the product of vxres * vyres overflows 32 bits (is 0 in this case/example), so any protection from this block is lost. But even if yres_virtual (aka vyres) is "only" 0x01000000, so no multiplication overflow occurs, the resulting value of vyres "seems" to still be too large and can cause an error [I'm not sure about this last part -- I need to use a new gcc so that KASAN will work.] > Bisection is inconclusive: the issue happens on the oldest tested release. > > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=10fa45d8300000 > final oops: https://syzkaller.appspot.com/x/report.txt?x=12fa45d8300000 > console output: https://syzkaller.appspot.com/x/log.txt?x=14fa45d8300000 > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > Reported-by: syzbot+04168c8063cfdde1db5e@syzkaller.appspotmail.com > > BUG: unable to handle page fault for address: ffff888001000050 > #PF: supervisor write access in kernel mode > #PF: error_code(0x0003) - permissions violation > PGD 10e01067 P4D 10e01067 PUD 10e02067 PMD 80000000010001e1 > Oops: 0003 [#1] PREEMPT SMP KASAN > CPU: 1 PID: 8433 Comm: syz-executor067 Tainted: G W 5.13.0-syzkaller #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 > RIP: 0010:writeb arch/x86/include/asm/io.h:65 [inline] > RIP: 0010:vga16fb_fillrect+0x993/0x18d0 drivers/video/fbdev/vga16fb.c:923 > Code: 6c fd 48 63 44 24 10 45 31 f6 48 89 04 24 e8 44 a6 6c fd 31 ff 89 de 31 ed e8 79 ad 6c fd 85 db 4d 89 ec 74 22 e8 2d a6 6c fd <45> 88 34 24 83 c5 01 89 df 49 83 c4 01 89 ee e8 49 ae 6c fd 39 eb > RSP: 0018:ffffc90000eff848 EFLAGS: 00010293 > RAX: 0000000000000000 RBX: 000000000000001b RCX: 0000000000000000 > RDX: ffff88802d949c40 RSI: ffffffff8408e403 RDI: 0000000000000003 > RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffff8408dd8d > R10: ffffffff8408e3f7 R11: 0000000000000000 R12: ffff888001000050 > R13: ffff888001000050 R14: 0000000000000000 R15: 000000000ffeb7ff > FS: 0000000001aa2300(0000) GS:ffff8880b9d00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: ffff888001000050 CR3: 00000000346fb000 CR4: 00000000001506e0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Call Trace: > bit_clear_margins+0x3f6/0x4b0 drivers/video/fbdev/core/bitblit.c:224 > fbcon_clear_margins+0x1f1/0x280 drivers/video/fbdev/core/fbcon.c:1315 > fbcon_switch+0xa8c/0x1620 drivers/video/fbdev/core/fbcon.c:2146 > redraw_screen+0x2b9/0x740 drivers/tty/vt/vt.c:1021 > fbcon_modechanged+0x593/0x6d0 drivers/video/fbdev/core/fbcon.c:2651 > fbcon_update_vcs+0x3a/0x50 drivers/video/fbdev/core/fbcon.c:2696 > do_fb_ioctl+0x62e/0x690 drivers/video/fbdev/core/fbmem.c:1110 > fb_ioctl+0xe7/0x150 drivers/video/fbdev/core/fbmem.c:1185 > vfs_ioctl fs/ioctl.c:51 [inline] > __do_sys_ioctl fs/ioctl.c:1069 [inline] > __se_sys_ioctl fs/ioctl.c:1055 [inline] > __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:1055 > do_syscall_x64 arch/x86/entry/common.c:50 [inline] > do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80 > entry_SYSCALL_64_after_hwframe+0x44/0xae > RIP: 0033:0x43efd9 > Code: 28 c3 e8 2a 14 00 00 66 2e 0f 1f 84 00 00 00 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48 > RSP: 002b:00007ffc362df848 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 > RAX: ffffffffffffffda RBX: 0000000000400488 RCX: 000000000043efd9 > RDX: 0000000020000200 RSI: 0000000000004601 RDI: 0000000000000003 > RBP: 0000000000402fc0 R08: 0000000000000000 R09: 0000000000400488 > R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000403050 > R13: 0000000000000000 R14: 00000000004ac018 R15: 0000000000400488 > Modules linked in: > CR2: ffff888001000050 > ---[ end trace 39dce64bc5621bd3 ]--- > RIP: 0010:writeb arch/x86/include/asm/io.h:65 [inline] > RIP: 0010:vga16fb_fillrect+0x993/0x18d0 drivers/video/fbdev/vga16fb.c:923 > Code: 6c fd 48 63 44 24 10 45 31 f6 48 89 04 24 e8 44 a6 6c fd 31 ff 89 de 31 ed e8 79 ad 6c fd 85 db 4d 89 ec 74 22 e8 2d a6 6c fd <45> 88 34 24 83 c5 01 89 df 49 83 c4 01 89 ee e8 49 ae 6c fd 39 eb > RSP: 0018:ffffc90000eff848 EFLAGS: 00010293 > RAX: 0000000000000000 RBX: 000000000000001b RCX: 0000000000000000 > RDX: ffff88802d949c40 RSI: ffffffff8408e403 RDI: 0000000000000003 > RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffff8408dd8d > R10: ffffffff8408e3f7 R11: 0000000000000000 R12: ffff888001000050 > R13: ffff888001000050 R14: 0000000000000000 R15: 000000000ffeb7ff > FS: 0000000001aa2300(0000) GS:ffff8880b9d00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: ffff888001000050 CR3: 00000000346fb000 CR4: 00000000001506e0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 -- ~Randy ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect 2021-08-30 0:24 ` Randy Dunlap @ 2021-08-30 2:27 ` Tetsuo Handa 2021-08-30 2:31 ` Randy Dunlap 2021-08-30 12:00 ` Geert Uytterhoeven 0 siblings, 2 replies; 38+ messages in thread From: Tetsuo Handa @ 2021-08-30 2:27 UTC (permalink / raw) To: Randy Dunlap Cc: Geert Uytterhoeven, syzbot, akpm, b.zolnierkie, colin.king, dri-devel, linux-fbdev, linux-kernel, masahiroy, syzkaller-bugs On 2021/08/30 9:24, Randy Dunlap wrote: > Note that yres_virtual is set to 0x10000000. Is there no practical limit > (hence limit check) that can be used here? > > Also, in vga16fb_check_var(), beginning at line 404: > > 404 if (yres > vyres) > 405 vyres = yres; > 406 if (vxres * vyres > maxmem) { > 407 vyres = maxmem / vxres; > 408 if (vyres < yres) > 409 return -ENOMEM; > 410 } > > At line 406, the product of vxres * vyres overflows 32 bits (is 0 in this > case/example), so any protection from this block is lost. OK. Then, we can check overflow like below. diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c index e2757ff1c23d..e483a3f5fd47 100644 --- a/drivers/video/fbdev/vga16fb.c +++ b/drivers/video/fbdev/vga16fb.c @@ -403,7 +403,7 @@ static int vga16fb_check_var(struct fb_var_screeninfo *var, if (yres > vyres) vyres = yres; - if (vxres * vyres > maxmem) { + if ((u64) vxres * vyres > (u64) maxmem) { vyres = maxmem / vxres; if (vyres < yres) return -ENOMEM; But I think we can check overflow in the common code like below. (Both patch fixed the oops.) diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 1c855145711b..8899679bbc46 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1008,6 +1008,11 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) if (var->xres < 8 || var->yres < 8) return -EINVAL; + /* Don't allow u32 * u32 to overflow. */ + if ((u64) var->xres * var->yres > (u64) UINT_MAX || + (u64) var->xres_virtual * var->yres_virtual > (u64) UINT_MAX) + return -EINVAL; + ret = info->fbops->fb_check_var(var, info); if (ret) > > But even if yres_virtual (aka vyres) is "only" 0x01000000, so no > multiplication overflow occurs, the resulting value of vyres "seems" > to still be too large and can cause an error [I'm not sure about this > last part -- I need to use a new gcc so that KASAN will work.] ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect 2021-08-30 2:27 ` Tetsuo Handa @ 2021-08-30 2:31 ` Randy Dunlap 2021-08-30 16:05 ` [PATCH] fbmem: don't allow too huge resolutions Tetsuo Handa 2021-08-30 12:00 ` Geert Uytterhoeven 1 sibling, 1 reply; 38+ messages in thread From: Randy Dunlap @ 2021-08-30 2:31 UTC (permalink / raw) To: Tetsuo Handa Cc: Geert Uytterhoeven, syzbot, akpm, b.zolnierkie, colin.king, dri-devel, linux-fbdev, linux-kernel, masahiroy, syzkaller-bugs On 8/29/21 7:27 PM, Tetsuo Handa wrote: > On 2021/08/30 9:24, Randy Dunlap wrote: >> Note that yres_virtual is set to 0x10000000. Is there no practical limit >> (hence limit check) that can be used here? >> >> Also, in vga16fb_check_var(), beginning at line 404: >> >> 404 if (yres > vyres) >> 405 vyres = yres; >> 406 if (vxres * vyres > maxmem) { >> 407 vyres = maxmem / vxres; >> 408 if (vyres < yres) >> 409 return -ENOMEM; >> 410 } >> >> At line 406, the product of vxres * vyres overflows 32 bits (is 0 in this >> case/example), so any protection from this block is lost. > > OK. Then, we can check overflow like below. > > diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c > index e2757ff1c23d..e483a3f5fd47 100644 > --- a/drivers/video/fbdev/vga16fb.c > +++ b/drivers/video/fbdev/vga16fb.c > @@ -403,7 +403,7 @@ static int vga16fb_check_var(struct fb_var_screeninfo *var, > > if (yres > vyres) > vyres = yres; > - if (vxres * vyres > maxmem) { > + if ((u64) vxres * vyres > (u64) maxmem) { > vyres = maxmem / vxres; > if (vyres < yres) > return -ENOMEM; > > But I think we can check overflow in the common code like below. (Both patch fixed the oops.) OK, great. Thanks. > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index 1c855145711b..8899679bbc46 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -1008,6 +1008,11 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) > if (var->xres < 8 || var->yres < 8) > return -EINVAL; > > + /* Don't allow u32 * u32 to overflow. */ > + if ((u64) var->xres * var->yres > (u64) UINT_MAX || > + (u64) var->xres_virtual * var->yres_virtual > (u64) UINT_MAX) > + return -EINVAL; > + > ret = info->fbops->fb_check_var(var, info); > > if (ret) > >> >> But even if yres_virtual (aka vyres) is "only" 0x01000000, so no >> multiplication overflow occurs, the resulting value of vyres "seems" >> to still be too large and can cause an error [I'm not sure about this >> last part -- I need to use a new gcc so that KASAN will work.] > -- ~Randy ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH] fbmem: don't allow too huge resolutions 2021-08-30 2:31 ` Randy Dunlap @ 2021-08-30 16:05 ` Tetsuo Handa 2021-08-31 6:48 ` Geert Uytterhoeven 0 siblings, 1 reply; 38+ messages in thread From: Tetsuo Handa @ 2021-08-30 16:05 UTC (permalink / raw) To: Daniel Vetter, Geert Uytterhoeven Cc: syzbot, akpm, b.zolnierkie, colin.king, dri-devel, linux-fbdev, linux-kernel, masahiroy, syzkaller-bugs, Randy Dunlap syzbot is reporting page fault at vga16fb_fillrect() [1], for vga16fb_check_var() is failing to detect multiplication overflow. if (vxres * vyres > maxmem) { vyres = maxmem / vxres; if (vyres < yres) return -ENOMEM; } Since no module would accept too huge resolutions where multiplication overflow happens, let's reject in the common path. This patch does not use array_size(), for array_size() is allowed to return UINT_MAX on 32bits even if overflow did not happen. We want to detect only overflow here, for individual module will recheck with more strict limits as needed. Link: https://syzkaller.appspot.com/bug?extid=04168c8063cfdde1db5e [1] Reported-by: syzbot <syzbot+04168c8063cfdde1db5e@syzkaller.appspotmail.com> Debugged-by: Randy Dunlap <rdunlap@infradead.org> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Tested-by: syzbot <syzbot+04168c8063cfdde1db5e@syzkaller.appspotmail.com> --- drivers/video/fbdev/core/fbmem.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 1c855145711b..9f5075dc2345 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1008,6 +1008,11 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) if (var->xres < 8 || var->yres < 8) return -EINVAL; + /* Don't allow u32 * u32 to overflow. */ + if ((u64) var->xres * var->yres > UINT_MAX || + (u64) var->xres_virtual * var->yres_virtual > UINT_MAX) + return -EINVAL; + ret = info->fbops->fb_check_var(var, info); if (ret) -- 2.18.4 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH] fbmem: don't allow too huge resolutions 2021-08-30 16:05 ` [PATCH] fbmem: don't allow too huge resolutions Tetsuo Handa @ 2021-08-31 6:48 ` Geert Uytterhoeven 0 siblings, 0 replies; 38+ messages in thread From: Geert Uytterhoeven @ 2021-08-31 6:48 UTC (permalink / raw) To: Tetsuo Handa Cc: Daniel Vetter, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs, Randy Dunlap Hi Tetsuo, Thanks for your patch! On Mon, Aug 30, 2021 at 6:05 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > syzbot is reporting page fault at vga16fb_fillrect() [1], for > vga16fb_check_var() is failing to detect multiplication overflow. > > if (vxres * vyres > maxmem) { > vyres = maxmem / vxres; > if (vyres < yres) > return -ENOMEM; > } IMHO that should be fixed in vga16fb, too. > Since no module would accept too huge resolutions where multiplication > overflow happens, let's reject in the common path. > > This patch does not use array_size(), for array_size() is allowed to > return UINT_MAX on 32bits even if overflow did not happen. We want to > detect only overflow here, for individual module will recheck with more > strict limits as needed. Which is IMHO not really an issue, as I believe on 32-bit you cannot use a very large frame buffer, long before you reach UINT_MAX. > Link: https://syzkaller.appspot.com/bug?extid=04168c8063cfdde1db5e [1] > Reported-by: syzbot <syzbot+04168c8063cfdde1db5e@syzkaller.appspotmail.com> > Debugged-by: Randy Dunlap <rdunlap@infradead.org> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Tested-by: syzbot <syzbot+04168c8063cfdde1db5e@syzkaller.appspotmail.com> > --- > drivers/video/fbdev/core/fbmem.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index 1c855145711b..9f5075dc2345 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -1008,6 +1008,11 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) > if (var->xres < 8 || var->yres < 8) > return -EINVAL; > > + /* Don't allow u32 * u32 to overflow. */ > + if ((u64) var->xres * var->yres > UINT_MAX || > + (u64) var->xres_virtual * var->yres_virtual > UINT_MAX) > + return -EINVAL; > + I think it would still be better to use check_mul_overflow(), as that makes it clear and explicit what is being done, even without a comment. Furthermore, this restricts the virtual frame buffer size on 64-bit, too, while graphics cards can have much more than 4 GiB of RAM. > ret = info->fbops->fb_check_var(var, info); > > if (ret) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] fbmem: don't allow too huge resolutions @ 2021-08-31 6:48 ` Geert Uytterhoeven 0 siblings, 0 replies; 38+ messages in thread From: Geert Uytterhoeven @ 2021-08-31 6:48 UTC (permalink / raw) To: Tetsuo Handa Cc: Daniel Vetter, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs, Randy Dunlap Hi Tetsuo, Thanks for your patch! On Mon, Aug 30, 2021 at 6:05 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > syzbot is reporting page fault at vga16fb_fillrect() [1], for > vga16fb_check_var() is failing to detect multiplication overflow. > > if (vxres * vyres > maxmem) { > vyres = maxmem / vxres; > if (vyres < yres) > return -ENOMEM; > } IMHO that should be fixed in vga16fb, too. > Since no module would accept too huge resolutions where multiplication > overflow happens, let's reject in the common path. > > This patch does not use array_size(), for array_size() is allowed to > return UINT_MAX on 32bits even if overflow did not happen. We want to > detect only overflow here, for individual module will recheck with more > strict limits as needed. Which is IMHO not really an issue, as I believe on 32-bit you cannot use a very large frame buffer, long before you reach UINT_MAX. > Link: https://syzkaller.appspot.com/bug?extid=04168c8063cfdde1db5e [1] > Reported-by: syzbot <syzbot+04168c8063cfdde1db5e@syzkaller.appspotmail.com> > Debugged-by: Randy Dunlap <rdunlap@infradead.org> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Tested-by: syzbot <syzbot+04168c8063cfdde1db5e@syzkaller.appspotmail.com> > --- > drivers/video/fbdev/core/fbmem.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index 1c855145711b..9f5075dc2345 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -1008,6 +1008,11 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) > if (var->xres < 8 || var->yres < 8) > return -EINVAL; > > + /* Don't allow u32 * u32 to overflow. */ > + if ((u64) var->xres * var->yres > UINT_MAX || > + (u64) var->xres_virtual * var->yres_virtual > UINT_MAX) > + return -EINVAL; > + I think it would still be better to use check_mul_overflow(), as that makes it clear and explicit what is being done, even without a comment. Furthermore, this restricts the virtual frame buffer size on 64-bit, too, while graphics cards can have much more than 4 GiB of RAM. > ret = info->fbops->fb_check_var(var, info); > > if (ret) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] fbmem: don't allow too huge resolutions 2021-08-31 6:48 ` Geert Uytterhoeven (?) @ 2021-08-31 15:23 ` Tetsuo Handa 2021-08-31 16:20 ` Daniel Vetter 2021-08-31 17:19 ` Geert Uytterhoeven -1 siblings, 2 replies; 38+ messages in thread From: Tetsuo Handa @ 2021-08-31 15:23 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Daniel Vetter, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs, Randy Dunlap On 2021/08/31 15:48, Geert Uytterhoeven wrote: > Furthermore, this restricts the virtual frame buffer size on 64-bit, > too, while graphics cards can have much more than 4 GiB of RAM. Excuse me, but do you mean that some hardware allows allocating more than UINT_MAX bytes of memory for kernel frame buffer drivers? > IMHO that should be fixed in vga16fb, too. According to https://elixir.bootlin.com/linux/v5.14/A/ident/fb_check_var , there are 89 files. Randomly picking up drivers/video/fbdev/udlfb.c as an example. dlfb_is_valid_mode() from dlfb_ops_check_var() is doing if (mode->xres * mode->yres > dlfb->sku_pixel_limit) return 0; return 1; where max dlfb->sku_pixel_limit seems to be 2048 * 1152 but I think we need same overflow check. I want to avoid patching individual modules if possible. That depends on whether some hardware needs to allocate more than UINT_MAX bytes of memory. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] fbmem: don't allow too huge resolutions 2021-08-31 15:23 ` Tetsuo Handa @ 2021-08-31 16:20 ` Daniel Vetter 2021-08-31 17:19 ` Geert Uytterhoeven 1 sibling, 0 replies; 38+ messages in thread From: Daniel Vetter @ 2021-08-31 16:20 UTC (permalink / raw) To: Tetsuo Handa Cc: Geert Uytterhoeven, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs, Randy Dunlap On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > On 2021/08/31 15:48, Geert Uytterhoeven wrote: > > Furthermore, this restricts the virtual frame buffer size on 64-bit, > > too, while graphics cards can have much more than 4 GiB of RAM. > > Excuse me, but do you mean that some hardware allows allocating more than > UINT_MAX bytes of memory for kernel frame buffer drivers? > > > IMHO that should be fixed in vga16fb, too. > > According to https://elixir.bootlin.com/linux/v5.14/A/ident/fb_check_var , > there are 89 files. Randomly picking up drivers/video/fbdev/udlfb.c as > an example. dlfb_is_valid_mode() from dlfb_ops_check_var() is doing > > if (mode->xres * mode->yres > dlfb->sku_pixel_limit) > return 0; > return 1; > > where max dlfb->sku_pixel_limit seems to be 2048 * 1152 but I think we need > same overflow check. I want to avoid patching individual modules if possible. > That depends on whether some hardware needs to allocate more than UINT_MAX > bytes of memory. Yeah basic input validation makes no sense to push into each driver. That's just asking that most of the fbdev drivers will never be fixed. Same for not-so-basic input validation, if there's no driver that actually needs the flexibility (like the virtual vs physical size thing that's floating around maybe). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] fbmem: don't allow too huge resolutions @ 2021-08-31 16:20 ` Daniel Vetter 0 siblings, 0 replies; 38+ messages in thread From: Daniel Vetter @ 2021-08-31 16:20 UTC (permalink / raw) To: Tetsuo Handa Cc: Geert Uytterhoeven, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs, Randy Dunlap On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > On 2021/08/31 15:48, Geert Uytterhoeven wrote: > > Furthermore, this restricts the virtual frame buffer size on 64-bit, > > too, while graphics cards can have much more than 4 GiB of RAM. > > Excuse me, but do you mean that some hardware allows allocating more than > UINT_MAX bytes of memory for kernel frame buffer drivers? > > > IMHO that should be fixed in vga16fb, too. > > According to https://elixir.bootlin.com/linux/v5.14/A/ident/fb_check_var , > there are 89 files. Randomly picking up drivers/video/fbdev/udlfb.c as > an example. dlfb_is_valid_mode() from dlfb_ops_check_var() is doing > > if (mode->xres * mode->yres > dlfb->sku_pixel_limit) > return 0; > return 1; > > where max dlfb->sku_pixel_limit seems to be 2048 * 1152 but I think we need > same overflow check. I want to avoid patching individual modules if possible. > That depends on whether some hardware needs to allocate more than UINT_MAX > bytes of memory. Yeah basic input validation makes no sense to push into each driver. That's just asking that most of the fbdev drivers will never be fixed. Same for not-so-basic input validation, if there's no driver that actually needs the flexibility (like the virtual vs physical size thing that's floating around maybe). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] fbmem: don't allow too huge resolutions 2021-08-31 15:23 ` Tetsuo Handa @ 2021-08-31 17:19 ` Geert Uytterhoeven 2021-08-31 17:19 ` Geert Uytterhoeven 1 sibling, 0 replies; 38+ messages in thread From: Geert Uytterhoeven @ 2021-08-31 17:19 UTC (permalink / raw) To: Tetsuo Handa Cc: Daniel Vetter, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs, Randy Dunlap Hi Handa-san, On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > On 2021/08/31 15:48, Geert Uytterhoeven wrote: > > Furthermore, this restricts the virtual frame buffer size on 64-bit, > > too, while graphics cards can have much more than 4 GiB of RAM. > > Excuse me, but do you mean that some hardware allows allocating more than > UINT_MAX bytes of memory for kernel frame buffer drivers? While smem_len is u32 (there have been complaints about such limitations on 64-bit platforms as far as 10 years ago), I see no reason why a graphics card with more than 4 GiB of RAM would not be able to provide a very large virtual screen. Of course e.g. vga16fb cannot, as it is limited to 64 KiB. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] fbmem: don't allow too huge resolutions @ 2021-08-31 17:19 ` Geert Uytterhoeven 0 siblings, 0 replies; 38+ messages in thread From: Geert Uytterhoeven @ 2021-08-31 17:19 UTC (permalink / raw) To: Tetsuo Handa Cc: Daniel Vetter, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs, Randy Dunlap Hi Handa-san, On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > On 2021/08/31 15:48, Geert Uytterhoeven wrote: > > Furthermore, this restricts the virtual frame buffer size on 64-bit, > > too, while graphics cards can have much more than 4 GiB of RAM. > > Excuse me, but do you mean that some hardware allows allocating more than > UINT_MAX bytes of memory for kernel frame buffer drivers? While smem_len is u32 (there have been complaints about such limitations on 64-bit platforms as far as 10 years ago), I see no reason why a graphics card with more than 4 GiB of RAM would not be able to provide a very large virtual screen. Of course e.g. vga16fb cannot, as it is limited to 64 KiB. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] fbmem: don't allow too huge resolutions 2021-08-31 17:19 ` Geert Uytterhoeven @ 2021-08-31 18:53 ` Daniel Vetter -1 siblings, 0 replies; 38+ messages in thread From: Daniel Vetter @ 2021-08-31 18:53 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Tetsuo Handa, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs, Randy Dunlap On Tue, Aug 31, 2021 at 7:19 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Handa-san, > > On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: > > On 2021/08/31 15:48, Geert Uytterhoeven wrote: > > > Furthermore, this restricts the virtual frame buffer size on 64-bit, > > > too, while graphics cards can have much more than 4 GiB of RAM. > > > > Excuse me, but do you mean that some hardware allows allocating more than > > UINT_MAX bytes of memory for kernel frame buffer drivers? > > While smem_len is u32 (there have been complaints about such > limitations on 64-bit platforms as far as 10 years ago), I see no > reason why a graphics card with more than 4 GiB of RAM would not be > able to provide a very large virtual screen. > > Of course e.g. vga16fb cannot, as it is limited to 64 KiB. The first gpus with 4GB or more memory started shipping in 2012. We're not going to have fbdev drivers for these, so let's not invent code for use-cases that aren't please. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] fbmem: don't allow too huge resolutions @ 2021-08-31 18:53 ` Daniel Vetter 0 siblings, 0 replies; 38+ messages in thread From: Daniel Vetter @ 2021-08-31 18:53 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Tetsuo Handa, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs, Randy Dunlap On Tue, Aug 31, 2021 at 7:19 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Handa-san, > > On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: > > On 2021/08/31 15:48, Geert Uytterhoeven wrote: > > > Furthermore, this restricts the virtual frame buffer size on 64-bit, > > > too, while graphics cards can have much more than 4 GiB of RAM. > > > > Excuse me, but do you mean that some hardware allows allocating more than > > UINT_MAX bytes of memory for kernel frame buffer drivers? > > While smem_len is u32 (there have been complaints about such > limitations on 64-bit platforms as far as 10 years ago), I see no > reason why a graphics card with more than 4 GiB of RAM would not be > able to provide a very large virtual screen. > > Of course e.g. vga16fb cannot, as it is limited to 64 KiB. The first gpus with 4GB or more memory started shipping in 2012. We're not going to have fbdev drivers for these, so let's not invent code for use-cases that aren't please. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] fbmem: don't allow too huge resolutions 2021-08-31 18:53 ` Daniel Vetter @ 2021-08-31 18:56 ` Geert Uytterhoeven -1 siblings, 0 replies; 38+ messages in thread From: Geert Uytterhoeven @ 2021-08-31 18:56 UTC (permalink / raw) To: Daniel Vetter Cc: Tetsuo Handa, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs, Randy Dunlap Hi Daniel, On Tue, Aug 31, 2021 at 8:53 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > On Tue, Aug 31, 2021 at 7:19 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa > > <penguin-kernel@i-love.sakura.ne.jp> wrote: > > > On 2021/08/31 15:48, Geert Uytterhoeven wrote: > > > > Furthermore, this restricts the virtual frame buffer size on 64-bit, > > > > too, while graphics cards can have much more than 4 GiB of RAM. > > > > > > Excuse me, but do you mean that some hardware allows allocating more than > > > UINT_MAX bytes of memory for kernel frame buffer drivers? > > > > While smem_len is u32 (there have been complaints about such > > limitations on 64-bit platforms as far as 10 years ago), I see no > > reason why a graphics card with more than 4 GiB of RAM would not be > > able to provide a very large virtual screen. > > > > Of course e.g. vga16fb cannot, as it is limited to 64 KiB. > > The first gpus with 4GB or more memory started shipping in 2012. We're > not going to have fbdev drivers for these, so let's not invent code > for use-cases that aren't please. This code path is used with fbdev emulation for drm drivers, too, right? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] fbmem: don't allow too huge resolutions @ 2021-08-31 18:56 ` Geert Uytterhoeven 0 siblings, 0 replies; 38+ messages in thread From: Geert Uytterhoeven @ 2021-08-31 18:56 UTC (permalink / raw) To: Daniel Vetter Cc: Tetsuo Handa, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs, Randy Dunlap Hi Daniel, On Tue, Aug 31, 2021 at 8:53 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > On Tue, Aug 31, 2021 at 7:19 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa > > <penguin-kernel@i-love.sakura.ne.jp> wrote: > > > On 2021/08/31 15:48, Geert Uytterhoeven wrote: > > > > Furthermore, this restricts the virtual frame buffer size on 64-bit, > > > > too, while graphics cards can have much more than 4 GiB of RAM. > > > > > > Excuse me, but do you mean that some hardware allows allocating more than > > > UINT_MAX bytes of memory for kernel frame buffer drivers? > > > > While smem_len is u32 (there have been complaints about such > > limitations on 64-bit platforms as far as 10 years ago), I see no > > reason why a graphics card with more than 4 GiB of RAM would not be > > able to provide a very large virtual screen. > > > > Of course e.g. vga16fb cannot, as it is limited to 64 KiB. > > The first gpus with 4GB or more memory started shipping in 2012. We're > not going to have fbdev drivers for these, so let's not invent code > for use-cases that aren't please. This code path is used with fbdev emulation for drm drivers, too, right? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] fbmem: don't allow too huge resolutions 2021-08-31 18:56 ` Geert Uytterhoeven @ 2021-08-31 19:04 ` Daniel Vetter -1 siblings, 0 replies; 38+ messages in thread From: Daniel Vetter @ 2021-08-31 19:04 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Tetsuo Handa, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs, Randy Dunlap On Tue, Aug 31, 2021 at 8:56 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Daniel, > > On Tue, Aug 31, 2021 at 8:53 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > On Tue, Aug 31, 2021 at 7:19 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa > > > <penguin-kernel@i-love.sakura.ne.jp> wrote: > > > > On 2021/08/31 15:48, Geert Uytterhoeven wrote: > > > > > Furthermore, this restricts the virtual frame buffer size on 64-bit, > > > > > too, while graphics cards can have much more than 4 GiB of RAM. > > > > > > > > Excuse me, but do you mean that some hardware allows allocating more than > > > > UINT_MAX bytes of memory for kernel frame buffer drivers? > > > > > > While smem_len is u32 (there have been complaints about such > > > limitations on 64-bit platforms as far as 10 years ago), I see no > > > reason why a graphics card with more than 4 GiB of RAM would not be > > > able to provide a very large virtual screen. > > > > > > Of course e.g. vga16fb cannot, as it is limited to 64 KiB. > > > > The first gpus with 4GB or more memory started shipping in 2012. We're > > not going to have fbdev drivers for these, so let's not invent code > > for use-cases that aren't please. > > This code path is used with fbdev emulation for drm drivers, too, > right? Yeah, you get one buffer, with overallocating 2. That's all, you don't get the entire vram because we can't revoke that for fbdev users. We'd have fixed this long ago if it's a real limitations. 8k at 64bpp is still less than 256MB. Also due to pci bar limitations (which finally get lifted now because windows fixed their pci code, which motivates motherboard manufactures for desktop space to also fix theirs) we're limited to 256MB actually cpu visible anyway. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] fbmem: don't allow too huge resolutions @ 2021-08-31 19:04 ` Daniel Vetter 0 siblings, 0 replies; 38+ messages in thread From: Daniel Vetter @ 2021-08-31 19:04 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Tetsuo Handa, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs, Randy Dunlap On Tue, Aug 31, 2021 at 8:56 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Daniel, > > On Tue, Aug 31, 2021 at 8:53 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > On Tue, Aug 31, 2021 at 7:19 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa > > > <penguin-kernel@i-love.sakura.ne.jp> wrote: > > > > On 2021/08/31 15:48, Geert Uytterhoeven wrote: > > > > > Furthermore, this restricts the virtual frame buffer size on 64-bit, > > > > > too, while graphics cards can have much more than 4 GiB of RAM. > > > > > > > > Excuse me, but do you mean that some hardware allows allocating more than > > > > UINT_MAX bytes of memory for kernel frame buffer drivers? > > > > > > While smem_len is u32 (there have been complaints about such > > > limitations on 64-bit platforms as far as 10 years ago), I see no > > > reason why a graphics card with more than 4 GiB of RAM would not be > > > able to provide a very large virtual screen. > > > > > > Of course e.g. vga16fb cannot, as it is limited to 64 KiB. > > > > The first gpus with 4GB or more memory started shipping in 2012. We're > > not going to have fbdev drivers for these, so let's not invent code > > for use-cases that aren't please. > > This code path is used with fbdev emulation for drm drivers, too, > right? Yeah, you get one buffer, with overallocating 2. That's all, you don't get the entire vram because we can't revoke that for fbdev users. We'd have fixed this long ago if it's a real limitations. 8k at 64bpp is still less than 256MB. Also due to pci bar limitations (which finally get lifted now because windows fixed their pci code, which motivates motherboard manufactures for desktop space to also fix theirs) we're limited to 256MB actually cpu visible anyway. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2] fbmem: don't allow too huge resolutions 2021-08-31 19:04 ` Daniel Vetter (?) @ 2021-09-01 1:14 ` Tetsuo Handa 2021-09-01 7:12 ` Geert Uytterhoeven 2021-09-08 10:27 ` [PATCH v2 (repost)] " Tetsuo Handa -1 siblings, 2 replies; 38+ messages in thread From: Tetsuo Handa @ 2021-09-01 1:14 UTC (permalink / raw) To: Daniel Vetter, Geert Uytterhoeven Cc: syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs, Randy Dunlap syzbot is reporting page fault at vga16fb_fillrect() [1], for vga16fb_check_var() is failing to detect multiplication overflow. if (vxres * vyres > maxmem) { vyres = maxmem / vxres; if (vyres < yres) return -ENOMEM; } Since no module would accept too huge resolutions where multiplication overflow happens, let's reject in the common path. Link: https://syzkaller.appspot.com/bug?extid=04168c8063cfdde1db5e [1] Reported-by: syzbot <syzbot+04168c8063cfdde1db5e@syzkaller.appspotmail.com> Debugged-by: Randy Dunlap <rdunlap@infradead.org> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- Changes in v2: Use check_mul_overflow(), suggested by Geert Uytterhoeven <geert@linux-m68k.org>. drivers/video/fbdev/core/fbmem.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 1c855145711b..53d23b3d010c 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -962,6 +962,7 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) struct fb_var_screeninfo old_var; struct fb_videomode mode; struct fb_event event; + u32 unused; if (var->activate & FB_ACTIVATE_INV_MODE) { struct fb_videomode mode1, mode2; @@ -1008,6 +1009,11 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) if (var->xres < 8 || var->yres < 8) return -EINVAL; + /* Too huge resolution causes multiplication overflow. */ + if (check_mul_overflow(var->xres, var->yres, &unused) || + check_mul_overflow(var->xres_virtual, var->yres_virtual, &unused)) + return -EINVAL; + ret = info->fbops->fb_check_var(var, info); if (ret) -- 2.25.1 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2] fbmem: don't allow too huge resolutions 2021-09-01 1:14 ` [PATCH v2] " Tetsuo Handa @ 2021-09-01 7:12 ` Geert Uytterhoeven 2021-09-08 10:27 ` [PATCH v2 (repost)] " Tetsuo Handa 1 sibling, 0 replies; 38+ messages in thread From: Geert Uytterhoeven @ 2021-09-01 7:12 UTC (permalink / raw) To: Tetsuo Handa Cc: Daniel Vetter, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs, Randy Dunlap On Wed, Sep 1, 2021 at 3:15 AM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > syzbot is reporting page fault at vga16fb_fillrect() [1], for > vga16fb_check_var() is failing to detect multiplication overflow. > > if (vxres * vyres > maxmem) { > vyres = maxmem / vxres; > if (vyres < yres) > return -ENOMEM; > } > > Since no module would accept too huge resolutions where multiplication > overflow happens, let's reject in the common path. > > Link: https://syzkaller.appspot.com/bug?extid=04168c8063cfdde1db5e [1] > Reported-by: syzbot <syzbot+04168c8063cfdde1db5e@syzkaller.appspotmail.com> > Debugged-by: Randy Dunlap <rdunlap@infradead.org> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2] fbmem: don't allow too huge resolutions @ 2021-09-01 7:12 ` Geert Uytterhoeven 0 siblings, 0 replies; 38+ messages in thread From: Geert Uytterhoeven @ 2021-09-01 7:12 UTC (permalink / raw) To: Tetsuo Handa Cc: Daniel Vetter, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs, Randy Dunlap On Wed, Sep 1, 2021 at 3:15 AM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > syzbot is reporting page fault at vga16fb_fillrect() [1], for > vga16fb_check_var() is failing to detect multiplication overflow. > > if (vxres * vyres > maxmem) { > vyres = maxmem / vxres; > if (vyres < yres) > return -ENOMEM; > } > > Since no module would accept too huge resolutions where multiplication > overflow happens, let's reject in the common path. > > Link: https://syzkaller.appspot.com/bug?extid=04168c8063cfdde1db5e [1] > Reported-by: syzbot <syzbot+04168c8063cfdde1db5e@syzkaller.appspotmail.com> > Debugged-by: Randy Dunlap <rdunlap@infradead.org> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 (repost)] fbmem: don't allow too huge resolutions 2021-09-01 1:14 ` [PATCH v2] " Tetsuo Handa 2021-09-01 7:12 ` Geert Uytterhoeven @ 2021-09-08 10:27 ` Tetsuo Handa 2021-09-08 16:52 ` Daniel Vetter 1 sibling, 1 reply; 38+ messages in thread From: Tetsuo Handa @ 2021-09-08 10:27 UTC (permalink / raw) To: Daniel Vetter, Andrew Morton Cc: DRI Development, Linux Fbdev development list syzbot is reporting page fault at vga16fb_fillrect() [1], for vga16fb_check_var() is failing to detect multiplication overflow. if (vxres * vyres > maxmem) { vyres = maxmem / vxres; if (vyres < yres) return -ENOMEM; } Since no module would accept too huge resolutions where multiplication overflow happens, let's reject in the common path. Link: https://syzkaller.appspot.com/bug?extid=04168c8063cfdde1db5e [1] Reported-by: syzbot <syzbot+04168c8063cfdde1db5e@syzkaller.appspotmail.com> Debugged-by: Randy Dunlap <rdunlap@infradead.org> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> --- Changes in v2: Use check_mul_overflow(), suggested by Geert Uytterhoeven <geert@linux-m68k.org>. drivers/video/fbdev/core/fbmem.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 71fb710f1ce3..7420d2c16e47 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -962,6 +962,7 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) struct fb_var_screeninfo old_var; struct fb_videomode mode; struct fb_event event; + u32 unused; if (var->activate & FB_ACTIVATE_INV_MODE) { struct fb_videomode mode1, mode2; @@ -1008,6 +1009,11 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) if (var->xres < 8 || var->yres < 8) return -EINVAL; + /* Too huge resolution causes multiplication overflow. */ + if (check_mul_overflow(var->xres, var->yres, &unused) || + check_mul_overflow(var->xres_virtual, var->yres_virtual, &unused)) + return -EINVAL; + ret = info->fbops->fb_check_var(var, info); if (ret) -- 2.18.4 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2 (repost)] fbmem: don't allow too huge resolutions 2021-09-08 10:27 ` [PATCH v2 (repost)] " Tetsuo Handa @ 2021-09-08 16:52 ` Daniel Vetter 0 siblings, 0 replies; 38+ messages in thread From: Daniel Vetter @ 2021-09-08 16:52 UTC (permalink / raw) To: Tetsuo Handa Cc: Daniel Vetter, Andrew Morton, DRI Development, Linux Fbdev development list On Wed, Sep 08, 2021 at 07:27:49PM +0900, Tetsuo Handa wrote: > syzbot is reporting page fault at vga16fb_fillrect() [1], for > vga16fb_check_var() is failing to detect multiplication overflow. > > if (vxres * vyres > maxmem) { > vyres = maxmem / vxres; > if (vyres < yres) > return -ENOMEM; > } > > Since no module would accept too huge resolutions where multiplication > overflow happens, let's reject in the common path. > > Link: https://syzkaller.appspot.com/bug?extid=04168c8063cfdde1db5e [1] > Reported-by: syzbot <syzbot+04168c8063cfdde1db5e@syzkaller.appspotmail.com> > Debugged-by: Randy Dunlap <rdunlap@infradead.org> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > Changes in v2: > Use check_mul_overflow(), suggested by Geert Uytterhoeven <geert@linux-m68k.org>. Pushed to drm-misc-next-fixes so it should get into current merge window. I also added a cc: stable here, I htink it's needed. Thanks a lot to both you&Geert for handling this! -Daniel > > drivers/video/fbdev/core/fbmem.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index 71fb710f1ce3..7420d2c16e47 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -962,6 +962,7 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) > struct fb_var_screeninfo old_var; > struct fb_videomode mode; > struct fb_event event; > + u32 unused; > > if (var->activate & FB_ACTIVATE_INV_MODE) { > struct fb_videomode mode1, mode2; > @@ -1008,6 +1009,11 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) > if (var->xres < 8 || var->yres < 8) > return -EINVAL; > > + /* Too huge resolution causes multiplication overflow. */ > + if (check_mul_overflow(var->xres, var->yres, &unused) || > + check_mul_overflow(var->xres_virtual, var->yres_virtual, &unused)) > + return -EINVAL; > + > ret = info->fbops->fb_check_var(var, info); > > if (ret) > -- > 2.18.4 > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect 2021-08-30 2:27 ` Tetsuo Handa @ 2021-08-30 12:00 ` Geert Uytterhoeven 2021-08-30 12:00 ` Geert Uytterhoeven 1 sibling, 0 replies; 38+ messages in thread From: Geert Uytterhoeven @ 2021-08-30 12:00 UTC (permalink / raw) To: Tetsuo Handa Cc: Randy Dunlap, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs Hi Testsuo, On Mon, Aug 30, 2021 at 4:27 AM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > On 2021/08/30 9:24, Randy Dunlap wrote: > > Note that yres_virtual is set to 0x10000000. Is there no practical limit > > (hence limit check) that can be used here? > > > > Also, in vga16fb_check_var(), beginning at line 404: > > > > 404 if (yres > vyres) > > 405 vyres = yres; > > 406 if (vxres * vyres > maxmem) { > > 407 vyres = maxmem / vxres; > > 408 if (vyres < yres) > > 409 return -ENOMEM; > > 410 } > > > > At line 406, the product of vxres * vyres overflows 32 bits (is 0 in this > > case/example), so any protection from this block is lost. > > OK. Then, we can check overflow like below. > > diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c > index e2757ff1c23d..e483a3f5fd47 100644 > --- a/drivers/video/fbdev/vga16fb.c > +++ b/drivers/video/fbdev/vga16fb.c > @@ -403,7 +403,7 @@ static int vga16fb_check_var(struct fb_var_screeninfo *var, > > if (yres > vyres) > vyres = yres; > - if (vxres * vyres > maxmem) { > + if ((u64) vxres * vyres > (u64) maxmem) { Mindlessly changing the sizes is not the solution. Please use e.g. the array_size() helper from <linux/overflow.h> instead. > vyres = maxmem / vxres; > if (vyres < yres) > return -ENOMEM; > > But I think we can check overflow in the common code like below. (Both patch fixed the oops.) > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index 1c855145711b..8899679bbc46 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -1008,6 +1008,11 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) > if (var->xres < 8 || var->yres < 8) > return -EINVAL; > > + /* Don't allow u32 * u32 to overflow. */ > + if ((u64) var->xres * var->yres > (u64) UINT_MAX || > + (u64) var->xres_virtual * var->yres_virtual > (u64) UINT_MAX) > + return -EINVAL; > + Same comment here, of course. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect @ 2021-08-30 12:00 ` Geert Uytterhoeven 0 siblings, 0 replies; 38+ messages in thread From: Geert Uytterhoeven @ 2021-08-30 12:00 UTC (permalink / raw) To: Tetsuo Handa Cc: Randy Dunlap, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs Hi Testsuo, On Mon, Aug 30, 2021 at 4:27 AM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > On 2021/08/30 9:24, Randy Dunlap wrote: > > Note that yres_virtual is set to 0x10000000. Is there no practical limit > > (hence limit check) that can be used here? > > > > Also, in vga16fb_check_var(), beginning at line 404: > > > > 404 if (yres > vyres) > > 405 vyres = yres; > > 406 if (vxres * vyres > maxmem) { > > 407 vyres = maxmem / vxres; > > 408 if (vyres < yres) > > 409 return -ENOMEM; > > 410 } > > > > At line 406, the product of vxres * vyres overflows 32 bits (is 0 in this > > case/example), so any protection from this block is lost. > > OK. Then, we can check overflow like below. > > diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c > index e2757ff1c23d..e483a3f5fd47 100644 > --- a/drivers/video/fbdev/vga16fb.c > +++ b/drivers/video/fbdev/vga16fb.c > @@ -403,7 +403,7 @@ static int vga16fb_check_var(struct fb_var_screeninfo *var, > > if (yres > vyres) > vyres = yres; > - if (vxres * vyres > maxmem) { > + if ((u64) vxres * vyres > (u64) maxmem) { Mindlessly changing the sizes is not the solution. Please use e.g. the array_size() helper from <linux/overflow.h> instead. > vyres = maxmem / vxres; > if (vyres < yres) > return -ENOMEM; > > But I think we can check overflow in the common code like below. (Both patch fixed the oops.) > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index 1c855145711b..8899679bbc46 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -1008,6 +1008,11 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) > if (var->xres < 8 || var->yres < 8) > return -EINVAL; > > + /* Don't allow u32 * u32 to overflow. */ > + if ((u64) var->xres * var->yres > (u64) UINT_MAX || > + (u64) var->xres_virtual * var->yres_virtual > (u64) UINT_MAX) > + return -EINVAL; > + Same comment here, of course. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect 2021-08-30 12:00 ` Geert Uytterhoeven (?) @ 2021-08-30 13:00 ` Dan Carpenter 2021-08-30 13:37 ` Tetsuo Handa -1 siblings, 1 reply; 38+ messages in thread From: Dan Carpenter @ 2021-08-30 13:00 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Tetsuo Handa, Randy Dunlap, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs On Mon, Aug 30, 2021 at 02:00:21PM +0200, Geert Uytterhoeven wrote: > Hi Testsuo, > > On Mon, Aug 30, 2021 at 4:27 AM Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: > > On 2021/08/30 9:24, Randy Dunlap wrote: > > > Note that yres_virtual is set to 0x10000000. Is there no practical limit > > > (hence limit check) that can be used here? > > > > > > Also, in vga16fb_check_var(), beginning at line 404: > > > > > > 404 if (yres > vyres) > > > 405 vyres = yres; > > > 406 if (vxres * vyres > maxmem) { > > > 407 vyres = maxmem / vxres; > > > 408 if (vyres < yres) > > > 409 return -ENOMEM; > > > 410 } > > > > > > At line 406, the product of vxres * vyres overflows 32 bits (is 0 in this > > > case/example), so any protection from this block is lost. > > > > OK. Then, we can check overflow like below. > > > > diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c > > index e2757ff1c23d..e483a3f5fd47 100644 > > --- a/drivers/video/fbdev/vga16fb.c > > +++ b/drivers/video/fbdev/vga16fb.c > > @@ -403,7 +403,7 @@ static int vga16fb_check_var(struct fb_var_screeninfo *var, > > > > if (yres > vyres) > > vyres = yres; > > - if (vxres * vyres > maxmem) { > > + if ((u64) vxres * vyres > (u64) maxmem) { > > Mindlessly changing the sizes is not the solution. > Please use e.g. the array_size() helper from <linux/overflow.h> > instead. On a 64bit system the array_size() macro is going to do the exact same casts? But I do think this code would be easier to understand if the integer overflow check were pull out separately and done first: if (array_size(vxres, vyres) >= UINT_MAX) return -EINVAL; if (vxres * vyres > maxmem) { ... The UINT_MAX is because vxres and vyres are u32. This would maybe be the first time anyone ever did an integer overflow check like this in the kernel. It's a new idiom. regards, dan carpenter ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect 2021-08-30 13:00 ` Dan Carpenter @ 2021-08-30 13:37 ` Tetsuo Handa 2021-08-30 13:47 ` Dan Carpenter 0 siblings, 1 reply; 38+ messages in thread From: Tetsuo Handa @ 2021-08-30 13:37 UTC (permalink / raw) To: Dan Carpenter, Geert Uytterhoeven Cc: Randy Dunlap, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs On 2021/08/30 22:00, Dan Carpenter wrote: >>> diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c >>> index e2757ff1c23d..e483a3f5fd47 100644 >>> --- a/drivers/video/fbdev/vga16fb.c >>> +++ b/drivers/video/fbdev/vga16fb.c >>> @@ -403,7 +403,7 @@ static int vga16fb_check_var(struct fb_var_screeninfo *var, >>> >>> if (yres > vyres) >>> vyres = yres; >>> - if (vxres * vyres > maxmem) { >>> + if ((u64) vxres * vyres > (u64) maxmem) { >> >> Mindlessly changing the sizes is not the solution. >> Please use e.g. the array_size() helper from <linux/overflow.h> >> instead. > > On a 64bit system the array_size() macro is going to do the exact same > casts? But I do think this code would be easier to understand if the > integer overflow check were pull out separately and done first: > > if (array_size(vxres, vyres) >= UINT_MAX) > return -EINVAL; This is wrong. array_size() returns ULONG_MAX on 64bits upon overflow and returns UINT_MAX on 32bits upon overflow. However, UINT_MAX is a valid value without overflow (e.g. vxres == UINT_MAX / 15 && vyres == 15). Comparing like "> (u64) UINT_MAX" is to detect only overflow. array_size() would be helpful for forcing memory allocation to fail (instead of allocating smaller than actually required). > > if (vxres * vyres > maxmem) { > ... > > The UINT_MAX is because vxres and vyres are u32. > > This would maybe be the first time anyone ever did an integer overflow > check like this in the kernel. It's a new idiom. > > regards, > dan carpenter > ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect 2021-08-30 13:37 ` Tetsuo Handa @ 2021-08-30 13:47 ` Dan Carpenter 2021-08-30 14:25 ` Tetsuo Handa 0 siblings, 1 reply; 38+ messages in thread From: Dan Carpenter @ 2021-08-30 13:47 UTC (permalink / raw) To: Tetsuo Handa Cc: Geert Uytterhoeven, Randy Dunlap, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs On Mon, Aug 30, 2021 at 10:37:31PM +0900, Tetsuo Handa wrote: > On 2021/08/30 22:00, Dan Carpenter wrote: > >>> diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c > >>> index e2757ff1c23d..e483a3f5fd47 100644 > >>> --- a/drivers/video/fbdev/vga16fb.c > >>> +++ b/drivers/video/fbdev/vga16fb.c > >>> @@ -403,7 +403,7 @@ static int vga16fb_check_var(struct fb_var_screeninfo *var, > >>> > >>> if (yres > vyres) > >>> vyres = yres; > >>> - if (vxres * vyres > maxmem) { > >>> + if ((u64) vxres * vyres > (u64) maxmem) { > >> > >> Mindlessly changing the sizes is not the solution. > >> Please use e.g. the array_size() helper from <linux/overflow.h> > >> instead. > > > > On a 64bit system the array_size() macro is going to do the exact same > > casts? But I do think this code would be easier to understand if the > > integer overflow check were pull out separately and done first: > > > > if (array_size(vxres, vyres) >= UINT_MAX) > > return -EINVAL; > > This is wrong. array_size() returns ULONG_MAX on 64bits upon overflow and > returns UINT_MAX on 32bits upon overflow. However, UINT_MAX is a valid > value without overflow (e.g. vxres == UINT_MAX / 15 && vyres == 15). Huh... I just assumed we didn't allow resolutions that high. > Comparing like "> (u64) UINT_MAX" is to detect only overflow. > Of course, that doesn't work on 32 bit systems. Also the cast isn't required because of type promotion. regards, dan carpenter ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect 2021-08-30 13:47 ` Dan Carpenter @ 2021-08-30 14:25 ` Tetsuo Handa 2021-08-30 14:29 ` Dan Carpenter 2021-08-30 14:30 ` Geert Uytterhoeven 0 siblings, 2 replies; 38+ messages in thread From: Tetsuo Handa @ 2021-08-30 14:25 UTC (permalink / raw) To: Dan Carpenter Cc: Geert Uytterhoeven, Randy Dunlap, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs On 2021/08/30 22:47, Dan Carpenter wrote: > On Mon, Aug 30, 2021 at 10:37:31PM +0900, Tetsuo Handa wrote: >> On 2021/08/30 22:00, Dan Carpenter wrote: >>>>> diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c >>>>> index e2757ff1c23d..e483a3f5fd47 100644 >>>>> --- a/drivers/video/fbdev/vga16fb.c >>>>> +++ b/drivers/video/fbdev/vga16fb.c >>>>> @@ -403,7 +403,7 @@ static int vga16fb_check_var(struct fb_var_screeninfo *var, >>>>> >>>>> if (yres > vyres) >>>>> vyres = yres; >>>>> - if (vxres * vyres > maxmem) { >>>>> + if ((u64) vxres * vyres > (u64) maxmem) { >>>> >>>> Mindlessly changing the sizes is not the solution. >>>> Please use e.g. the array_size() helper from <linux/overflow.h> >>>> instead. >>> >>> On a 64bit system the array_size() macro is going to do the exact same >>> casts? But I do think this code would be easier to understand if the >>> integer overflow check were pull out separately and done first: >>> >>> if (array_size(vxres, vyres) >= UINT_MAX) >>> return -EINVAL; >> >> This is wrong. array_size() returns ULONG_MAX on 64bits upon overflow and >> returns UINT_MAX on 32bits upon overflow. However, UINT_MAX is a valid >> value without overflow (e.g. vxres == UINT_MAX / 15 && vyres == 15). > > Huh... I just assumed we didn't allow resolutions that high. Of course, we don't allow resolutions that high. ;-) Since I don't know possible max resolutions, I chose UINT_MAX + 1 as a common limit for returning -EINVAL. Unless overflow happens, vga16fb_check_var() will return -ENOMEM on such high resolutions. > >> Comparing like "> (u64) UINT_MAX" is to detect only overflow. >> > > Of course, that doesn't work on 32 bit systems. Also the cast isn't > required because of type promotion. Indeed, "> UINT_MAX" seems to work on both 32bits and 64bits. ---------- #include <stdio.h> #include <limits.h> int main(int argc, char *argv[]) { unsigned int w = 0x600; unsigned int h = 0x10000000; if ((unsigned long long) w * h > UINT_MAX) printf("Overflowed\n"); else printf("No overflow\n"); return 0; } ---------- ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect 2021-08-30 14:25 ` Tetsuo Handa @ 2021-08-30 14:29 ` Dan Carpenter 2021-08-30 14:30 ` Geert Uytterhoeven 1 sibling, 0 replies; 38+ messages in thread From: Dan Carpenter @ 2021-08-30 14:29 UTC (permalink / raw) To: Tetsuo Handa Cc: Geert Uytterhoeven, Randy Dunlap, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs On Mon, Aug 30, 2021 at 11:25:51PM +0900, Tetsuo Handa wrote: > On 2021/08/30 22:47, Dan Carpenter wrote: > > On Mon, Aug 30, 2021 at 10:37:31PM +0900, Tetsuo Handa wrote: > >> On 2021/08/30 22:00, Dan Carpenter wrote: > >>>>> diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c > >>>>> index e2757ff1c23d..e483a3f5fd47 100644 > >>>>> --- a/drivers/video/fbdev/vga16fb.c > >>>>> +++ b/drivers/video/fbdev/vga16fb.c > >>>>> @@ -403,7 +403,7 @@ static int vga16fb_check_var(struct fb_var_screeninfo *var, > >>>>> > >>>>> if (yres > vyres) > >>>>> vyres = yres; > >>>>> - if (vxres * vyres > maxmem) { > >>>>> + if ((u64) vxres * vyres > (u64) maxmem) { > >>>> > >>>> Mindlessly changing the sizes is not the solution. > >>>> Please use e.g. the array_size() helper from <linux/overflow.h> > >>>> instead. > >>> > >>> On a 64bit system the array_size() macro is going to do the exact same > >>> casts? But I do think this code would be easier to understand if the > >>> integer overflow check were pull out separately and done first: > >>> > >>> if (array_size(vxres, vyres) >= UINT_MAX) > >>> return -EINVAL; > >> > >> This is wrong. array_size() returns ULONG_MAX on 64bits upon overflow and > >> returns UINT_MAX on 32bits upon overflow. However, UINT_MAX is a valid > >> value without overflow (e.g. vxres == UINT_MAX / 15 && vyres == 15). > > > > Huh... I just assumed we didn't allow resolutions that high. > > Of course, we don't allow resolutions that high. ;-) > > Since I don't know possible max resolutions, I chose UINT_MAX + 1 as a common > limit for returning -EINVAL. Unless overflow happens, vga16fb_check_var() will > return -ENOMEM on such high resolutions. > > > > >> Comparing like "> (u64) UINT_MAX" is to detect only overflow. > >> > > > > Of course, that doesn't work on 32 bit systems. Also the cast isn't > > required because of type promotion. > > Indeed, "> UINT_MAX" seems to work on both 32bits and 64bits. Sorry, for the confusion. I'm talking about array_size() which is size_t. Your approach using unsigned long long works. regards, dan carpenter ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect 2021-08-30 14:25 ` Tetsuo Handa @ 2021-08-30 14:30 ` Geert Uytterhoeven 2021-08-30 14:30 ` Geert Uytterhoeven 1 sibling, 0 replies; 38+ messages in thread From: Geert Uytterhoeven @ 2021-08-30 14:30 UTC (permalink / raw) To: Tetsuo Handa Cc: Dan Carpenter, Randy Dunlap, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs Hi Tetsuo, On Mon, Aug 30, 2021 at 4:26 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > On 2021/08/30 22:47, Dan Carpenter wrote: > > On Mon, Aug 30, 2021 at 10:37:31PM +0900, Tetsuo Handa wrote: > >> On 2021/08/30 22:00, Dan Carpenter wrote: > >>>>> diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c > >>>>> index e2757ff1c23d..e483a3f5fd47 100644 > >>>>> --- a/drivers/video/fbdev/vga16fb.c > >>>>> +++ b/drivers/video/fbdev/vga16fb.c > >>>>> @@ -403,7 +403,7 @@ static int vga16fb_check_var(struct fb_var_screeninfo *var, > >>>>> > >>>>> if (yres > vyres) > >>>>> vyres = yres; > >>>>> - if (vxres * vyres > maxmem) { > >>>>> + if ((u64) vxres * vyres > (u64) maxmem) { > >>>> > >>>> Mindlessly changing the sizes is not the solution. > >>>> Please use e.g. the array_size() helper from <linux/overflow.h> > >>>> instead. > >>> > >>> On a 64bit system the array_size() macro is going to do the exact same > >>> casts? But I do think this code would be easier to understand if the > >>> integer overflow check were pull out separately and done first: > >>> > >>> if (array_size(vxres, vyres) >= UINT_MAX) > >>> return -EINVAL; > >> > >> This is wrong. array_size() returns ULONG_MAX on 64bits upon overflow and > >> returns UINT_MAX on 32bits upon overflow. However, UINT_MAX is a valid > >> value without overflow (e.g. vxres == UINT_MAX / 15 && vyres == 15). > > > > Huh... I just assumed we didn't allow resolutions that high. > > Of course, we don't allow resolutions that high. ;-) > > Since I don't know possible max resolutions, I chose UINT_MAX + 1 as a common > limit for returning -EINVAL. Unless overflow happens, vga16fb_check_var() will > return -ENOMEM on such high resolutions. The highest possible value of maxmem inside vga16fb_check_var() is 65536. So I believe if (array_size(vxres, vyres) > maxmem) should work fine. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect @ 2021-08-30 14:30 ` Geert Uytterhoeven 0 siblings, 0 replies; 38+ messages in thread From: Geert Uytterhoeven @ 2021-08-30 14:30 UTC (permalink / raw) To: Tetsuo Handa Cc: Dan Carpenter, Randy Dunlap, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs Hi Tetsuo, On Mon, Aug 30, 2021 at 4:26 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > On 2021/08/30 22:47, Dan Carpenter wrote: > > On Mon, Aug 30, 2021 at 10:37:31PM +0900, Tetsuo Handa wrote: > >> On 2021/08/30 22:00, Dan Carpenter wrote: > >>>>> diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c > >>>>> index e2757ff1c23d..e483a3f5fd47 100644 > >>>>> --- a/drivers/video/fbdev/vga16fb.c > >>>>> +++ b/drivers/video/fbdev/vga16fb.c > >>>>> @@ -403,7 +403,7 @@ static int vga16fb_check_var(struct fb_var_screeninfo *var, > >>>>> > >>>>> if (yres > vyres) > >>>>> vyres = yres; > >>>>> - if (vxres * vyres > maxmem) { > >>>>> + if ((u64) vxres * vyres > (u64) maxmem) { > >>>> > >>>> Mindlessly changing the sizes is not the solution. > >>>> Please use e.g. the array_size() helper from <linux/overflow.h> > >>>> instead. > >>> > >>> On a 64bit system the array_size() macro is going to do the exact same > >>> casts? But I do think this code would be easier to understand if the > >>> integer overflow check were pull out separately and done first: > >>> > >>> if (array_size(vxres, vyres) >= UINT_MAX) > >>> return -EINVAL; > >> > >> This is wrong. array_size() returns ULONG_MAX on 64bits upon overflow and > >> returns UINT_MAX on 32bits upon overflow. However, UINT_MAX is a valid > >> value without overflow (e.g. vxres == UINT_MAX / 15 && vyres == 15). > > > > Huh... I just assumed we didn't allow resolutions that high. > > Of course, we don't allow resolutions that high. ;-) > > Since I don't know possible max resolutions, I chose UINT_MAX + 1 as a common > limit for returning -EINVAL. Unless overflow happens, vga16fb_check_var() will > return -ENOMEM on such high resolutions. The highest possible value of maxmem inside vga16fb_check_var() is 65536. So I believe if (array_size(vxres, vyres) > maxmem) should work fine. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect 2021-08-30 14:30 ` Geert Uytterhoeven (?) @ 2021-08-30 14:38 ` Tetsuo Handa 2021-08-30 14:53 ` Geert Uytterhoeven -1 siblings, 1 reply; 38+ messages in thread From: Tetsuo Handa @ 2021-08-30 14:38 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Dan Carpenter, Randy Dunlap, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs On 2021/08/30 23:30, Geert Uytterhoeven wrote: > The highest possible value of maxmem inside vga16fb_check_var() > is 65536. Yes. > > So I believe > > if (array_size(vxres, vyres) > maxmem) > > should work fine. My intent is to check at common path than individual module so that we don't need to add same check to every module. Same approach is proposed at https://lkml.kernel.org/r/1630294223-7225-1-git-send-email-tcs_kernel@tencent.com . ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect 2021-08-30 14:38 ` Tetsuo Handa @ 2021-08-30 14:53 ` Geert Uytterhoeven 0 siblings, 0 replies; 38+ messages in thread From: Geert Uytterhoeven @ 2021-08-30 14:53 UTC (permalink / raw) To: Tetsuo Handa Cc: Dan Carpenter, Randy Dunlap, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs Hi Tetsuo, On Mon, Aug 30, 2021 at 4:38 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > On 2021/08/30 23:30, Geert Uytterhoeven wrote: > > The highest possible value of maxmem inside vga16fb_check_var() > > is 65536. > > Yes. > > > > > So I believe > > > > if (array_size(vxres, vyres) > maxmem) > > > > should work fine. > > My intent is to check at common path than individual module so that we don't > need to add same check to every module. Same approach is proposed at > https://lkml.kernel.org/r/1630294223-7225-1-git-send-email-tcs_kernel@tencent.com . Which I believe is wrong. Thanks for the pointer, I will reply to the actual patch... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect @ 2021-08-30 14:53 ` Geert Uytterhoeven 0 siblings, 0 replies; 38+ messages in thread From: Geert Uytterhoeven @ 2021-08-30 14:53 UTC (permalink / raw) To: Tetsuo Handa Cc: Dan Carpenter, Randy Dunlap, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs Hi Tetsuo, On Mon, Aug 30, 2021 at 4:38 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > On 2021/08/30 23:30, Geert Uytterhoeven wrote: > > The highest possible value of maxmem inside vga16fb_check_var() > > is 65536. > > Yes. > > > > > So I believe > > > > if (array_size(vxres, vyres) > maxmem) > > > > should work fine. > > My intent is to check at common path than individual module so that we don't > need to add same check to every module. Same approach is proposed at > https://lkml.kernel.org/r/1630294223-7225-1-git-send-email-tcs_kernel@tencent.com . Which I believe is wrong. Thanks for the pointer, I will reply to the actual patch... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect 2021-08-30 14:53 ` Geert Uytterhoeven @ 2021-08-30 15:00 ` Geert Uytterhoeven -1 siblings, 0 replies; 38+ messages in thread From: Geert Uytterhoeven @ 2021-08-30 15:00 UTC (permalink / raw) To: Tetsuo Handa Cc: Dan Carpenter, Randy Dunlap, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs Hi Tetsuo, On Mon, Aug 30, 2021 at 4:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Mon, Aug 30, 2021 at 4:38 PM Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: > > On 2021/08/30 23:30, Geert Uytterhoeven wrote: > > > The highest possible value of maxmem inside vga16fb_check_var() > > > is 65536. > > > > Yes. > > > > > > > > So I believe > > > > > > if (array_size(vxres, vyres) > maxmem) > > > > > > should work fine. > > > > My intent is to check at common path than individual module so that we don't > > need to add same check to every module. Same approach is proposed at > > https://lkml.kernel.org/r/1630294223-7225-1-git-send-email-tcs_kernel@tencent.com . > > Which I believe is wrong. > Thanks for the pointer, I will reply to the actual patch... Upon second look, that patch is not really wrong, as the check happens after calling into info->fbops->fb_check_var(). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect @ 2021-08-30 15:00 ` Geert Uytterhoeven 0 siblings, 0 replies; 38+ messages in thread From: Geert Uytterhoeven @ 2021-08-30 15:00 UTC (permalink / raw) To: Tetsuo Handa Cc: Dan Carpenter, Randy Dunlap, syzbot, Andrew Morton, Bartlomiej Zolnierkiewicz, Colin King, DRI Development, Linux Fbdev development list, Linux Kernel Mailing List, Masahiro Yamada, syzkaller-bugs Hi Tetsuo, On Mon, Aug 30, 2021 at 4:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Mon, Aug 30, 2021 at 4:38 PM Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: > > On 2021/08/30 23:30, Geert Uytterhoeven wrote: > > > The highest possible value of maxmem inside vga16fb_check_var() > > > is 65536. > > > > Yes. > > > > > > > > So I believe > > > > > > if (array_size(vxres, vyres) > maxmem) > > > > > > should work fine. > > > > My intent is to check at common path than individual module so that we don't > > need to add same check to every module. Same approach is proposed at > > https://lkml.kernel.org/r/1630294223-7225-1-git-send-email-tcs_kernel@tencent.com . > > Which I believe is wrong. > Thanks for the pointer, I will reply to the actual patch... Upon second look, that patch is not really wrong, as the check happens after calling into info->fbops->fb_check_var(). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2021-09-08 16:52 UTC | newest] Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-14 5:16 [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect syzbot 2021-07-14 5:16 ` syzbot 2021-08-30 0:24 ` Randy Dunlap 2021-08-30 2:27 ` Tetsuo Handa 2021-08-30 2:31 ` Randy Dunlap 2021-08-30 16:05 ` [PATCH] fbmem: don't allow too huge resolutions Tetsuo Handa 2021-08-31 6:48 ` Geert Uytterhoeven 2021-08-31 6:48 ` Geert Uytterhoeven 2021-08-31 15:23 ` Tetsuo Handa 2021-08-31 16:20 ` Daniel Vetter 2021-08-31 16:20 ` Daniel Vetter 2021-08-31 17:19 ` Geert Uytterhoeven 2021-08-31 17:19 ` Geert Uytterhoeven 2021-08-31 18:53 ` Daniel Vetter 2021-08-31 18:53 ` Daniel Vetter 2021-08-31 18:56 ` Geert Uytterhoeven 2021-08-31 18:56 ` Geert Uytterhoeven 2021-08-31 19:04 ` Daniel Vetter 2021-08-31 19:04 ` Daniel Vetter 2021-09-01 1:14 ` [PATCH v2] " Tetsuo Handa 2021-09-01 7:12 ` Geert Uytterhoeven 2021-09-01 7:12 ` Geert Uytterhoeven 2021-09-08 10:27 ` [PATCH v2 (repost)] " Tetsuo Handa 2021-09-08 16:52 ` Daniel Vetter 2021-08-30 12:00 ` [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect Geert Uytterhoeven 2021-08-30 12:00 ` Geert Uytterhoeven 2021-08-30 13:00 ` Dan Carpenter 2021-08-30 13:37 ` Tetsuo Handa 2021-08-30 13:47 ` Dan Carpenter 2021-08-30 14:25 ` Tetsuo Handa 2021-08-30 14:29 ` Dan Carpenter 2021-08-30 14:30 ` Geert Uytterhoeven 2021-08-30 14:30 ` Geert Uytterhoeven 2021-08-30 14:38 ` Tetsuo Handa 2021-08-30 14:53 ` Geert Uytterhoeven 2021-08-30 14:53 ` Geert Uytterhoeven 2021-08-30 15:00 ` Geert Uytterhoeven 2021-08-30 15:00 ` Geert Uytterhoeven
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.