[PATCH v6 2/4] fbmem: Prevent invalid virtual screen sizes

[Geert Uytterhoeven]

Hi Helge,

On Sun, Jun 26, 2022 at 12:32 PM Helge Deller <deller at gmx.de> wrote:
> Prevent that drivers or the user sets the virtual screen resolution
> smaller than the physical screen resolution.  This is important, because
> otherwise we may access memory outside of the graphics memory area.
>
> Signed-off-by: Helge Deller <deller at gmx.de>
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: stable at vger.kernel.org # v5.4+

Thanks for your patch, which is now commit fe04405ce5de13a5 ("fbmem:
Prevent invalid virtual screen sizes") in fbdev/for-next.

> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1006,6 +1006,12 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>         if (var->xres < 8 || var->yres < 8)
>                 return -EINVAL;
>
> +       /* make sure virtual resolution >= physical resolution */
> +       if (var->xres_virtual < var->xres)
> +               return -EINVAL;
> +       if (var->yres_virtual < var->yres)
> +               return -EINVAL;

This breaks valid use cases (e.g. "fbset -xres <larger-value-than-before>") ,
as the FBIOPUT_VSCREENINFO rule is to round up invalid values,
if possible.

Individual drivers may not follow that rule, so you could indeed end up
with a virtual resolution here if such a driver fails to sanitize var.
So either you have to move this after the call to fbops.fb_check_var()
below, and/or change the code to enlarge virtual resolution to match
physical resolution (at the risk of introducing another regression
with an obscure driver?).

So I'd go for moving it below.  And perhaps add a WARN(), as this
is a driver bug?

> +
>         /* 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))

Note that doing the multiplication overflow check before calling
fbops.fb_check_var() is fine, as too large values can never be
rounded up to a valid value.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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