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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux