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
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
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.]
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.]
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)
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
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.
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
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
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
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
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
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)
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
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)
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
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
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
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
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
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; } ----------
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
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
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@tencen... .
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@tencen... .
Which I believe is wrong. Thanks for the pointer, I will reply to the actual patch...
Gr{oetje,eeting}s,
Geert
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@tencen... .
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
dri-devel@lists.freedesktop.org
-
Dan Carpenter -
Daniel Vetter -
Daniel Vetter -
Geert Uytterhoeven -
Randy Dunlap -
syzbot -
Tetsuo Handa