Re: [PATCH 2/5] fbcon: Fix up user-provided virtual screen size

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Helge Deller <deller@gmx.de>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Linux Fbdev development list <linux-fbdev@vger.kernel.org>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 2/5] fbcon: Fix up user-provided virtual screen size
Date: Fri, 1 Jul 2022 16:52:00 +0200	[thread overview]
Message-ID: <CAMuHMdV_8+Cp=BCWhADQ-2B9o3sk6GSdmxxVLLiAnWV-Nwncug@mail.gmail.com> (raw)
In-Reply-To: <4580aa35-5a34-0d81-56d1-1f10218375d2@gmx.de>

Hi Helge,

On Fri, Jul 1, 2022 at 11:30 AM Helge Deller <deller@gmx.de> wrote:
> On 7/1/22 09:28, Geert Uytterhoeven wrote:
> > On Thu, Jun 30, 2022 at 10:10 PM Helge Deller <deller@gmx.de> wrote:
> >> On 6/30/22 22:00, Geert Uytterhoeven wrote:
> >>> On Thu, Jun 30, 2022 at 9:46 PM Helge Deller <deller@gmx.de> wrote:
> >>>> On 6/30/22 21:36, Geert Uytterhoeven wrote:
> >>>>> On Thu, Jun 30, 2022 at 9:31 PM Helge Deller <deller@gmx.de> wrote:
> >>>>>> On 6/30/22 21:00, Geert Uytterhoeven wrote:
> >>>>>>> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@gmx.de> wrote:
> >>>>>>>> The virtual screen size can't be smaller than the physical screen size.
> >>>>>>>> Based on the general rule that we round up user-provided input if
> >>>>>>>> neccessary, adjust the virtual screen size as well if needed.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Helge Deller <deller@gmx.de>
> >>>>>>>> Cc: stable@vger.kernel.org # v5.4+
> >>>>>>>
> >>>>>>> Thanks for your patch!
> >>>>>>>
> >>>>>>>> --- a/drivers/video/fbdev/core/fbmem.c
> >>>>>>>> +++ b/drivers/video/fbdev/core/fbmem.c
> >>>>>>>> @@ -1106,6 +1106,11 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
> >>>>>>>>                         return -EFAULT;
> >>>>>>>>                 console_lock();
> >>>>>>>>                 lock_fb_info(info);
> >>>>>>>> +               /* adjust virtual screen size if user missed it */
> >>>>>>>> +               if (var.xres_virtual < var.xres)
> >>>>>>>> +                       var.xres_virtual = var.xres;
> >>>>>>>> +               if (var.yres_virtual < var.yres)
> >>>>>>>> +                       var.yres_virtual = var.yres;
> >>>>>>>>                 ret = fb_set_var(info, &var);
> >>>>>>>>                 if (!ret)
> >>>>>>>>                         fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);
> >>>>>>>
> >>>>>>> Given "[PATCH 4/5] fbmem: Prevent invalid virtual screen sizes in
> >>>>>>> fb_set_var", I don't think we need this patch.
> >>>>>>
> >>>>>> We do.
> >>>>>
> >>>>> Why? It will be caught by [PATCH 4/5].
> >>>>
> >>>> Right, it will be caught by patch #4.
> >>>> But if you drop this part, then everytime a user runs
> >>>>         fbset -xres 800 -yres 600 -xvres 200
> >>>> users will get the KERNEL BUG WARNING (from patch #4) including
> >>>> a kernel backtrace in their syslogs.
> >>>
> >>> No, they will only see that warning if they are using a broken fbdev
> >>> driver that implements .fb_check_var(), but fails to validate or
> >>> update the passed geometry.
> >>
> >> IMHO this argument is mood.
> >> That way you put pressure on and need such simple code in
> >> each single driver to fix it up, instead of cleaning it up at a central
> >> place.
> >
> > Most hardware has restrictions on resolution (e.g. xres must be a
> > multiple of N), so the driver has to round up the resolution to make
> > it fit.  And after that the driver has to validate and update the
> > virtual resolution again anyway...
> >
> > If a driver does not support changing the video mode, it can leave
> > out the .fb_check_var() and .fb_set_par() callbacks, so the fbdev
> > core will ignore the userspace-supplied parameters, and reinstate
> > the single supported mode. See e.g. "[PATCH] drm/fb-helper:
> > Remove helpers to change frame buffer config"
> > (https://lore.kernel.org/all/20220629105658.1373770-1-geert@linux-m68k.org).
>
> I implemented all of your suggested changes - from this mail and the others.
> I've committed a new testing tree to the fbcon-fix-testing branch at:
> https://github.com/hdeller/linux/tree/fbcon-fix-testing
> The diff is here:
> https://github.com/torvalds/linux/compare/master...hdeller:linux:fbcon-fix-testing
>
> Although your idea is good since we now would find issues in the drivers,
> I don't think we want to commit it, since the testcase from
> the bug report then immediately crashes the kernel - see below.

That is expected behavior with panic_on_warn?
The right fix is to fix the broken .fb_check_var() implementation.

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