| Message ID | 535e404d-03bf-8e7a-b296-132a2a98c599@i-love.sakura.ne.jp |
|---|---|
| State | New |
| Headers | show |
| Series | fbmem: don't allow too huge resolutions | expand |
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
On 2021/08/31 15:48, Geert Uytterhoeven wrote: > Furthermore, this restricts the virtual frame buffer size on 64-bit, > too, while graphics cards can have much more than 4 GiB of RAM. Excuse me, but do you mean that some hardware allows allocating more than UINT_MAX bytes of memory for kernel frame buffer drivers? > IMHO that should be fixed in vga16fb, too. According to https://elixir.bootlin.com/linux/v5.14/A/ident/fb_check_var , there are 89 files. Randomly picking up drivers/video/fbdev/udlfb.c as an example. dlfb_is_valid_mode() from dlfb_ops_check_var() is doing if (mode->xres * mode->yres > dlfb->sku_pixel_limit) return 0; return 1; where max dlfb->sku_pixel_limit seems to be 2048 * 1152 but I think we need same overflow check. I want to avoid patching individual modules if possible. That depends on whether some hardware needs to allocate more than UINT_MAX bytes of memory.
On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > On 2021/08/31 15:48, Geert Uytterhoeven wrote: > > Furthermore, this restricts the virtual frame buffer size on 64-bit, > > too, while graphics cards can have much more than 4 GiB of RAM. > > Excuse me, but do you mean that some hardware allows allocating more than > UINT_MAX bytes of memory for kernel frame buffer drivers? > > > IMHO that should be fixed in vga16fb, too. > > According to https://elixir.bootlin.com/linux/v5.14/A/ident/fb_check_var , > there are 89 files. Randomly picking up drivers/video/fbdev/udlfb.c as > an example. dlfb_is_valid_mode() from dlfb_ops_check_var() is doing > > if (mode->xres * mode->yres > dlfb->sku_pixel_limit) > return 0; > return 1; > > where max dlfb->sku_pixel_limit seems to be 2048 * 1152 but I think we need > same overflow check. I want to avoid patching individual modules if possible. > That depends on whether some hardware needs to allocate more than UINT_MAX > bytes of memory. Yeah basic input validation makes no sense to push into each driver. That's just asking that most of the fbdev drivers will never be fixed. Same for not-so-basic input validation, if there's no driver that actually needs the flexibility (like the virtual vs physical size thing that's floating around maybe). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
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
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
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
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
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)
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(+)