Hi Geert,
On 6/28/22 10:36, Geert Uytterhoeven wrote:
> On Sun, Jun 26, 2022 at 12:32 PM Helge Deller <deller@xxxxxx> 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@xxxxxx>
>> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>> Cc: stable@xxxxxxxxxxxxxxx # 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.
You are right, fbset doesn't change the virtual screen size (unless the value
was given), so indeed we need to round up vres values in FBIOPUT_VSCREENINFO.
> 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?
That was exactly how I implemented in the first round, but changed it
due to feedback.
I'll respin the patch.
Thanks for reviewing that series!
Helge
>> /* 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@xxxxxxxxxxxxxx
>
> 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
