Re: [PATCH] fbmem: don't allow too huge resolutions
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: 2021-08-31 06:49:09
Also in:
dri-devel, lkml
Hi Tetsuo, Thanks for your patch! On Mon, Aug 30, 2021 at 6:05 PM Tetsuo Handa [off-list ref] 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.
quoted hunk ↗ jump to hunk
Link: https://syzkaller.appspot.com/bug?extid=04168c8063cfdde1db5e [1] Reported-by: syzbot <redacted> Debugged-by: Randy Dunlap [off-list ref] Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Tested-by: syzbot <redacted> --- 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