fbmem: don't allow too huge resolutions

Commit Message

Tetsuo Handa Aug. 30, 2021, 4:05 p.m. UTC

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 <[e-mail]>
Debugged-by: Randy Dunlap <[e-mail]>
Signed-off-by: Tetsuo Handa <[e-mail]>
Tested-by: syzbot <[e-mail]>
---
 drivers/video/fbdev/core/fbmem.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Geert Uytterhoeven Aug. 31, 2021, 6:48 a.m. UTC | #1

Hi Tetsuo,

Thanks for your patch!

On Mon, Aug 30, 2021 at 6:05 PM Tetsuo Handa
<[e-mail]> 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 <[e-mail]>
> Debugged-by: Randy Dunlap <[e-mail]>
> Signed-off-by: Tetsuo Handa <[e-mail]>
> Tested-by: syzbot <[e-mail]>
> ---
>  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

Tetsuo Handa Aug. 31, 2021, 3:23 p.m. UTC | #2

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 Aug. 31, 2021, 4:20 p.m. UTC | #3

On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa
<[e-mail]> 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

Geert Uytterhoeven Aug. 31, 2021, 5:19 p.m. UTC | #4

Hi Handa-san,

On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa
<[e-mail]> 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

Daniel Vetter Aug. 31, 2021, 6:53 p.m. UTC | #5

On Tue, Aug 31, 2021 at 7:19 PM Geert Uytterhoeven <[e-mail]> wrote:
>
> Hi Handa-san,
>
> On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa
> <[e-mail]> 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 Aug. 31, 2021, 6:56 p.m. UTC | #6

Hi Daniel,

On Tue, Aug 31, 2021 at 8:53 PM Daniel Vetter <[e-mail]> wrote:
> On Tue, Aug 31, 2021 at 7:19 PM Geert Uytterhoeven <[e-mail]> wrote:
> > On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa
> > <[e-mail]> 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

Daniel Vetter Aug. 31, 2021, 7:04 p.m. UTC | #7

On Tue, Aug 31, 2021 at 8:56 PM Geert Uytterhoeven <[e-mail]> wrote:
>
> Hi Daniel,
>
> On Tue, Aug 31, 2021 at 8:53 PM Daniel Vetter <[e-mail]> wrote:
> > On Tue, Aug 31, 2021 at 7:19 PM Geert Uytterhoeven <[e-mail]> wrote:
> > > On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa
> > > <[e-mail]> 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

Patch

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)