[syzbot] BUG: unable to handle kernel paging request in vga16fb_fillrect

[syzbot]

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+04168c...@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 syzk...@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

[Randy Dunlap]

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

~Randy

[Tetsuo Handa]

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)

[Randy Dunlap]

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

[Geert Uytterhoeven]

Hi Testsuo,

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 -- ge...@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

[Dan Carpenter]

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

[Tetsuo Handa]

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

[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

[Tetsuo Handa]

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

[Dan Carpenter]

Sorry, for the confusion. I'm talking about array_size() which is
size_t. Your approach using unsigned long long works.

regards,
dan carpenter

[Geert Uytterhoeven]

Hi Tetsuo,

The highest possible value of maxmem inside vga16fb_check_var()
is 65536.

So I believe

if (array_size(vxres, vyres) > maxmem)

should work fine.

[Tetsuo Handa]

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...@tencent.com .

[Geert Uytterhoeven]

Hi Tetsuo,

Which I believe is wrong.
Thanks for the pointer, I will reply to the actual patch...

[Geert Uytterhoeven]

Hi Tetsuo,

On Mon, Aug 30, 2021 at 4:53 PM Geert Uytterhoeven <ge...@linux-m68k.org> wrote:
> On Mon, Aug 30, 2021 at 4:38 PM Tetsuo Handa
> <penguin...@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...@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().

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+04168c...@syzkaller.appspotmail.com

Tested on:

commit: 7d2a07b7 Linux 5.14
git tree: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git v5.14
kernel config: https://syzkaller.appspot.com/x/.config?x=b04081cf516e2565
dashboard link: https://syzkaller.appspot.com/bug?extid=04168c8063cfdde1db5e
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.1
patch: https://syzkaller.appspot.com/x/patch.diff?x=10b4d775300000

Note: testing is done by a robot and is best-effort only.

[Tetsuo Handa]

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+04168c...@syzkaller.appspotmail.com>
Debugged-by: Randy Dunlap <rdu...@infradead.org>
Signed-off-by: Tetsuo Handa <penguin...@I-love.SAKURA.ne.jp>
Tested-by: syzbot <syzbot+04168c...@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

[Geert Uytterhoeven]

Hi Tetsuo,

Thanks for your patch!

On Mon, Aug 30, 2021 at 6:05 PM Tetsuo Handa
<penguin...@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+04168c...@syzkaller.appspotmail.com>
> Debugged-by: Randy Dunlap <rdu...@infradead.org>
> Signed-off-by: Tetsuo Handa <penguin...@I-love.SAKURA.ne.jp>
> Tested-by: syzbot <syzbot+04168c...@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)

[Tetsuo Handa]

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.

[Daniel Vetter]

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

[Geert Uytterhoeven]

Hi Handa-san,

On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa
<penguin...@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.

[Daniel Vetter]

On Tue, Aug 31, 2021 at 7:19 PM Geert Uytterhoeven <ge...@linux-m68k.org> wrote:
>
> Hi Handa-san,
>
> On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa
> <penguin...@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

[Geert Uytterhoeven]

Hi Daniel,

This code path is used with fbdev emulation for drm drivers, too,
right?

[Daniel Vetter]

On Tue, Aug 31, 2021 at 8:56 PM Geert Uytterhoeven <ge...@linux-m68k.org> wrote:
>
> Hi Daniel,
>
> On Tue, Aug 31, 2021 at 8:53 PM Daniel Vetter <daniel...@ffwll.ch> wrote:
> > On Tue, Aug 31, 2021 at 7:19 PM Geert Uytterhoeven <ge...@linux-m68k.org> wrote:
> > > On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa
> > > <penguin...@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

[Tetsuo Handa]

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+04168c...@syzkaller.appspotmail.com>
Debugged-by: Randy Dunlap <rdu...@infradead.org>
Signed-off-by: Tetsuo Handa <penguin...@I-love.SAKURA.ne.jp>

---
Changes in v2:
Use check_mul_overflow(), suggested by Geert Uytterhoeven <ge...@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

[Geert Uytterhoeven]

On Wed, Sep 1, 2021 at 3:15 AM Tetsuo Handa
<penguin...@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+04168c...@syzkaller.appspotmail.com>
> Debugged-by: Randy Dunlap <rdu...@infradead.org>
> Signed-off-by: Tetsuo Handa <penguin...@I-love.SAKURA.ne.jp>

Reviewed-by: Geert Uytterhoeven <geert+...@glider.be>