CC linux-fbdev On Tue, Apr 4, 2023 at 4:41 PM Thomas Zimmermann <tzimmermann@xxxxxxx> 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@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? 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@xxxxxxxxxxxxxx > 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@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
