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@xxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx > > > 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@xxxxxx> > > > Cc: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> > > > Cc: stable@xxxxxxxxxxxxxxx # v5.4+ > > > Cc: Daniel Vetter <daniel@xxxxxxxx> > > > Cc: Javier Martinez Canillas <javierm@xxxxxxxxxx> > > > Cc: Thomas Zimmermann <tzimmermann@xxxxxxx> > > > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> > > > > NAKed-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxx > > > > 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
