linux-fbdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect
@ 2021-07-14  5:16 syzbot
  2021-08-30  0:24 ` Randy Dunlap
  0 siblings, 1 reply; 26+ 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] 26+ messages in thread

* Re: [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect
  2021-07-14  5:16 [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect syzbot
@ 2021-08-30  0:24 ` Randy Dunlap
  2021-08-30  2:27   ` Tetsuo Handa
  0 siblings, 1 reply; 26+ 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] 26+ 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     ` [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect Geert Uytterhoeven
  0 siblings, 2 replies; 26+ 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] 26+ 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     ` [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect Geert Uytterhoeven
  1 sibling, 1 reply; 26+ 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] 26+ 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 12:00     ` Geert Uytterhoeven
  2021-08-30 13:00       ` Dan Carpenter
  1 sibling, 1 reply; 26+ 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] 26+ messages in thread

* Re: [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect
  2021-08-30 12:00     ` [syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect Geert Uytterhoeven
@ 2021-08-30 13:00       ` Dan Carpenter
  2021-08-30 13:37         ` Tetsuo Handa
  0 siblings, 1 reply; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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
  2021-08-30 14:38                 ` Tetsuo Handa
  1 sibling, 1 reply; 26+ 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] 26+ 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
  0 siblings, 1 reply; 26+ 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] 26+ 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
  2021-08-30 15:00                     ` Geert Uytterhoeven
  0 siblings, 1 reply; 26+ 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] 26+ 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
  0 siblings, 0 replies; 26+ 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] 26+ 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; 26+ 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] 26+ 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
  2021-08-31 15:23           ` Tetsuo Handa
  0 siblings, 1 reply; 26+ 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] 26+ 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
  0 siblings, 2 replies; 26+ 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] 26+ 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; 26+ 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] 26+ 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
  2021-08-31 18:53               ` Daniel Vetter
  1 sibling, 1 reply; 26+ 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] 26+ 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
  2021-08-31 18:56                 ` Geert Uytterhoeven
  0 siblings, 1 reply; 26+ 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] 26+ 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
  2021-08-31 19:04                   ` Daniel Vetter
  0 siblings, 1 reply; 26+ 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] 26+ 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
  2021-09-01  1:14                     ` [PATCH v2] " Tetsuo Handa
  0 siblings, 1 reply; 26+ 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] 26+ 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
  0 siblings, 2 replies; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ messages in thread

end of thread, other threads:[~2021-09-08 16:52 UTC | newest]

Thread overview: 26+ 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-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 15:23           ` Tetsuo Handa
2021-08-31 16:20             ` Daniel Vetter
2021-08-31 17:19             ` Geert Uytterhoeven
2021-08-31 18:53               ` Daniel Vetter
2021-08-31 18:56                 ` Geert Uytterhoeven
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-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 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:38                 ` Tetsuo Handa
2021-08-30 14:53                   ` Geert Uytterhoeven
2021-08-30 15:00                     ` Geert Uytterhoeven

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).