[PATCH] fbdev: Don't spam dmesg on bad userspace ioctl input

[Geert Uytterhoeven]

CC linux-fbdev

On Tue, Apr 4, 2023 at 4:41 PM Thomas Zimmermann <tzimmermann at suse.de> wrote:
> Am 04.04.23 um 16:19 schrieb Daniel Vetter:
> > On Tue, Apr 04, 2023 at 03:59:12PM +0200, Daniel Vetter wrote:
> >> On Tue, Apr 04, 2023 at 03:53:09PM +0200, Geert Uytterhoeven wrote:
> >>> CC vkmsdrm maintainer
> >>>
> >>> Thanks for your patch!
> >>>
> >>> On Tue, Apr 4, 2023 at 2:36 PM Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> >>>> There's a few reasons the kernel should not spam dmesg on bad
> >>>> userspace ioctl input:
> >>>> - at warning level it results in CI false positives
> >>>> - it allows userspace to drown dmesg output, potentially hiding real
> >>>>    issues.
> >>>>
> >>>> None of the other generic EINVAL checks report in the
> >>>> FBIOPUT_VSCREENINFO ioctl do this, so it's also inconsistent.
> >>>>
> >>>> I guess the intent of the patch which introduced this warning was that
> >>>> the drivers ->fb_check_var routine should fail in that case. Reality
> >>>> is that there's too many fbdev drivers and not enough people
> >>>> maintaining them by far, and so over the past few years we've simply
> >>>> handled all these validation gaps by tighning the checks in the core,
> >>>> because that's realistically really all that will ever happen.
> >>>>
> >>>> Reported-by: syzbot+20dcf81733d43ddff661 at syzkaller.appspotmail.com
> >>>> Link: https://syzkaller.appspot.com/bug?id=c5faf983bfa4a607de530cd3bb008888bf06cefc
> >>>
> >>>      WARNING: fbcon: Driver 'vkmsdrmfb' missed to adjust virtual screen
> >>> size (0x0 vs. 64x768)
> >>>
> >>> This is a bug in the vkmsdrmfb driver and/or DRM helpers.
> >>>
> >>> The message was added to make sure the individual drivers are fixed.
> >>> Perhaps it should be changed to BUG() instead, so dmesg output
> >>> cannot be drown?
> >>
> >> So you're solution is to essentially force us to replicate this check over
> >> all the drivers which cannot change the virtual size?
> >>
> >> Are you volunteering to field that audit and type all the patches?

The message is there to invite people to look at .fb_check_var()
implementations, and fix the issues.

> As most of us really only care about DRM, we can add this test to
> drm_fb_helper_check_var() [1] and that's it. No need to fix all of the
> fbdev drivers.

Either drm_fb_helper_check_var() (which is the most used buggy
implementation) has to be fixed, or it should be removed, cfr.
https://lore.kernel.org/all/20220629105658.1373770-1-geert@linux-m68k.org

> Having said that, I think the few remaining fbdev devs should decide if
> they want to actually put effort into fbdev, or accept it to bitrot
> away. The current state of 'non-maintenance' is the worst situation.
> I've been working on the console emulation and it is hard to get
> qualified reviews of the related fbdev code. At the same time, it's also
> not possible to get Ack-bys rubber-stamped.
>
> Best regards
> Thomas
>
> [1]
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_fb_helper.c#L1514
>
> > -Daniel
> >
> >>>> Fixes: 6c11df58fd1a ("fbmem: Check virtual screen sizes in fb_set_var()")
> >>>> Cc: Helge Deller <deller at gmx.de>
> >>>> Cc: Geert Uytterhoeven <geert at linux-m68k.org>
> >>>> Cc: stable at vger.kernel.org # v5.4+
> >>>> Cc: Daniel Vetter <daniel at ffwll.ch>
> >>>> Cc: Javier Martinez Canillas <javierm at redhat.com>
> >>>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
> >>>> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> >>>
> >>> NAKed-by: Geert Uytterhoeven <geert at linux-m68k.org>
> >>
> >> Yes I know it's not pretty, but realistically unless someone starts typing
> >> a _lot_ of patches this is the solution. It's exactly the same solution
> >> we've implemented for all other gaps syzcaller has find in fbdev input
> >> validation. Unless you can show that this is papering over a more severe
> >> bug somewhere, but then I guess it really should be a BUG to prevent worse
> >> things from happening.
> >> -Daniel
> >>
> >>>
> >>>> --- a/drivers/video/fbdev/core/fbmem.c
> >>>> +++ b/drivers/video/fbdev/core/fbmem.c
> >>>> @@ -1021,10 +1021,6 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
> >>>>          /* verify that virtual resolution >= physical resolution */
> >>>>          if (var->xres_virtual < var->xres ||
> >>>>              var->yres_virtual < var->yres) {
> >>>> -               pr_warn("WARNING: fbcon: Driver '%s' missed to adjust virtual screen size (%ux%u vs. %ux%u)\n",
> >>>> -                       info->fix.id,
> >>>> -                       var->xres_virtual, var->yres_virtual,
> >>>> -                       var->xres, var->yres);
> >>>>                  return -EINVAL;
> >>>>          }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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