| Message ID | 20220626102853.124108-3-deller@gmx.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | fbcon: Fixes for screen resolution changes | expand |
Hi Helge, On Sun, Jun 26, 2022 at 12:32 PM Helge Deller <deller@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@gmx.de> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: stable@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@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
Hi Geert, On 6/28/22 10:36, Geert Uytterhoeven wrote: > On Sun, Jun 26, 2022 at 12:32 PM Helge Deller <deller@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@gmx.de> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> Cc: stable@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. 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@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 afa2863670f3..5dfa4bbee642 100644 --- 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; + /* 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))