| Message ID | 20220626102853.124108-1-deller@gmx.de |
|---|---|
| Headers | show |
| Series | fbcon: Fixes for screen resolution changes | expand |
On 6/26/22 12:28, Helge Deller wrote: > This series fixes possible out-of-bound memory accesses when users trigger > screen resolutions changes with invalid input parameters, e.g. reconfigures > screen which is smaller than the current font size, or if the virtual screen > size is smaller than the physical screen size. I've pull this series into the for-next branch of the fbdev git tree: https://git.kernel.org/pub/scm/linux/kernel/git/deller/linux-fbdev.git/log/?h=for-next Helge
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 Helge, On Sun, Jun 26, 2022 at 12:33 PM Helge Deller <deller@gmx.de> wrote: > We need to prevent that users configure a screen size which is smaller than the > currently selected font size. Otherwise rendering chars on the screen will > access memory outside the graphics memory region. > > This patch adds a new function fbcon_modechange_possible() which > implements this check and which later may be extended with other checks > if necessary. The new function is called from the FBIOPUT_VSCREENINFO > ioctl handler in fbmem.c, which will return -EINVAL if userspace asked > for a too small screen size. > > 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 f0b6a66d33ca6e7e ("fbcon: Prevent that screen size is smaller than font size") in fbdev/for-next > --- a/drivers/video/fbdev/core/fbcon.c > +++ b/drivers/video/fbdev/core/fbcon.c > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -1112,7 +1112,9 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > return -EFAULT; > console_lock(); > lock_fb_info(info); > - ret = fb_set_var(info, &var); > + ret = fbcon_modechange_possible(info, &var); Again, this should be done (if done at all) after the call to fb_ops.check_var(), as it breaks the FBIOPUT_VSCREENINFO rounding rule. What if the user just wants to display graphics, not text? Can't the text console be disabled instead? > + if (!ret) > + ret = fb_set_var(info, &var); > if (!ret) > fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL); > unlock_fb_info(info); 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
On 6/28/22 10:39, Geert Uytterhoeven wrote: > Hi Helge, > > On Sun, Jun 26, 2022 at 12:33 PM Helge Deller <deller@gmx.de> wrote: >> We need to prevent that users configure a screen size which is smaller than the >> currently selected font size. Otherwise rendering chars on the screen will >> access memory outside the graphics memory region. >> >> This patch adds a new function fbcon_modechange_possible() which >> implements this check and which later may be extended with other checks >> if necessary. The new function is called from the FBIOPUT_VSCREENINFO >> ioctl handler in fbmem.c, which will return -EINVAL if userspace asked >> for a too small screen size. >> >> 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 f0b6a66d33ca6e7e ("fbcon: > Prevent that screen size is smaller than font size") in fbdev/for-next > >> --- a/drivers/video/fbdev/core/fbcon.c >> +++ b/drivers/video/fbdev/core/fbcon.c > >> --- a/drivers/video/fbdev/core/fbmem.c >> +++ b/drivers/video/fbdev/core/fbmem.c >> @@ -1112,7 +1112,9 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, >> return -EFAULT; >> console_lock(); >> lock_fb_info(info); >> - ret = fb_set_var(info, &var); >> + ret = fbcon_modechange_possible(info, &var); > > Again, this should be done (if done at all) after the call to > fb_ops.check_var(), as it breaks the FBIOPUT_VSCREENINFO rounding rule. > > What if the user just wants to display graphics, not text? Yes, I need to go back to an older version here too and check that the test is only run on text consoles. That check was dropped, due feedback that you could switch back from graphics (e.g. X11) to text console at any time....so the check for text-only is not correct. > Can't the text console be disabled instead? I think the solution is to return failure if switching back to text mode isn't possible if fonts are bigger than the screen resolution. That will be another patch. Thanks! Helge > >> + if (!ret) >> + ret = fb_set_var(info, &var); >> if (!ret) >> fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL); >> unlock_fb_info(info); > > 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 Helge, On Tue, Jun 28, 2022 at 10:52 PM Helge Deller <deller@gmx.de> wrote: > On 6/28/22 10:39, Geert Uytterhoeven wrote: > > On Sun, Jun 26, 2022 at 12:33 PM Helge Deller <deller@gmx.de> wrote: > >> We need to prevent that users configure a screen size which is smaller than the > >> currently selected font size. Otherwise rendering chars on the screen will > >> access memory outside the graphics memory region. > >> > >> This patch adds a new function fbcon_modechange_possible() which > >> implements this check and which later may be extended with other checks > >> if necessary. The new function is called from the FBIOPUT_VSCREENINFO > >> ioctl handler in fbmem.c, which will return -EINVAL if userspace asked > >> for a too small screen size. > >> > >> 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 f0b6a66d33ca6e7e ("fbcon: > > Prevent that screen size is smaller than font size") in fbdev/for-next > >> --- a/drivers/video/fbdev/core/fbmem.c > >> +++ b/drivers/video/fbdev/core/fbmem.c > >> @@ -1112,7 +1112,9 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > >> return -EFAULT; > >> console_lock(); > >> lock_fb_info(info); > >> - ret = fb_set_var(info, &var); > >> + ret = fbcon_modechange_possible(info, &var); > > > > Again, this should be done (if done at all) after the call to > > fb_ops.check_var(), as it breaks the FBIOPUT_VSCREENINFO rounding rule. > > > > What if the user just wants to display graphics, not text? > > Yes, I need to go back to an older version here too and check that > the test is only run on text consoles. > That check was dropped, due feedback that you could switch > back from graphics (e.g. X11) to text console at any time....so the > check for text-only is not correct. > > > Can't the text console be disabled instead? > > I think the solution is to return failure if switching back to text mode isn't possible if > fonts are bigger than the screen resolution. That will be another patch. Isn't the font a per-VC setting? Hence can't you change resolution, switch to a different VC, and run into this? I think the only real solution is to set the number of text columns and/or rows to zero, and make sure that is handled correctly. 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
On 6/29/22 09:03, Geert Uytterhoeven wrote: > Hi Helge, > > On Tue, Jun 28, 2022 at 10:52 PM Helge Deller <deller@gmx.de> wrote: >> On 6/28/22 10:39, Geert Uytterhoeven wrote: >>> On Sun, Jun 26, 2022 at 12:33 PM Helge Deller <deller@gmx.de> wrote: >>>> We need to prevent that users configure a screen size which is smaller than the >>>> currently selected font size. Otherwise rendering chars on the screen will >>>> access memory outside the graphics memory region. >>>> >>>> This patch adds a new function fbcon_modechange_possible() which >>>> implements this check and which later may be extended with other checks >>>> if necessary. The new function is called from the FBIOPUT_VSCREENINFO >>>> ioctl handler in fbmem.c, which will return -EINVAL if userspace asked >>>> for a too small screen size. >>>> >>>> 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 f0b6a66d33ca6e7e ("fbcon: >>> Prevent that screen size is smaller than font size") in fbdev/for-next > >>>> --- a/drivers/video/fbdev/core/fbmem.c >>>> +++ b/drivers/video/fbdev/core/fbmem.c >>>> @@ -1112,7 +1112,9 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, >>>> return -EFAULT; >>>> console_lock(); >>>> lock_fb_info(info); >>>> - ret = fb_set_var(info, &var); >>>> + ret = fbcon_modechange_possible(info, &var); >>> >>> Again, this should be done (if done at all) after the call to >>> fb_ops.check_var(), as it breaks the FBIOPUT_VSCREENINFO rounding rule. >>> >>> What if the user just wants to display graphics, not text? >> >> Yes, I need to go back to an older version here too and check that >> the test is only run on text consoles. >> That check was dropped, due feedback that you could switch >> back from graphics (e.g. X11) to text console at any time....so the >> check for text-only is not correct. >> >>> Can't the text console be disabled instead? >> >> I think the solution is to return failure if switching back to text mode isn't possible if >> fonts are bigger than the screen resolution. That will be another patch. > > Isn't the font a per-VC setting? Hence can't you change resolution, > switch to a different VC, and run into this? > > I think the only real solution is to set the number of text columns > and/or rows to zero, and make sure that is handled correctly. I agree, there doesn't seem to be a simple solution. On the other hand, such usecase seems very unlikely. If you have a proposal for a pacth I'd welcome it. Anyway, I've just sent out a new patch series. It does not include any patch for this theoretical problem yet. Helge