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

[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:
> > Hi Daniel,
> > 
> > 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?

Note that at least efifb, vesafb and offb seem to get this wrong. I didn't
bother checking any of the non-fw drivers. Iow there is a _lot_ of work in
your nack.
-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
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch