| Message ID | 20230404123624.360384-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fbdev: Don't spam dmesg on bad userspace ioctl input | expand |
Hi Daniel, CC vkmsdrm maintainer Thanks for your patch! On Tue, Apr 4, 2023 at 2:36 PM Daniel Vetter <daniel.vetter@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@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? > Fixes: 6c11df58fd1a ("fbmem: Check virtual screen sizes in fb_set_var()") > Cc: Helge Deller <deller@gmx.de> > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Cc: stable@vger.kernel.org # v5.4+ > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Javier Martinez Canillas <javierm@redhat.com> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> NAKed-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- 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@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
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@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@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? > > Fixes: 6c11df58fd1a ("fbmem: Check virtual screen sizes in fb_set_var()") > > Cc: Helge Deller <deller@gmx.de> > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > Cc: stable@vger.kernel.org # v5.4+ > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: Javier Martinez Canillas <javierm@redhat.com> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > NAKed-by: Geert Uytterhoeven <geert@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@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
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@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@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@gmx.de> > > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > > Cc: stable@vger.kernel.org # v5.4+ > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > Cc: Javier Martinez Canillas <javierm@redhat.com> > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > > NAKed-by: Geert Uytterhoeven <geert@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@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
Hi 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: >>> Hi Daniel, >>> >>> CC vkmsdrm maintainer >>> >>> Thanks for your patch! >>> >>> On Tue, Apr 4, 2023 at 2:36 PM Daniel Vetter <daniel.vetter@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@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. 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. 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@gmx.de> >>>> Cc: Geert Uytterhoeven <geert@linux-m68k.org> >>>> Cc: stable@vger.kernel.org # v5.4+ >>>> Cc: Daniel Vetter <daniel@ffwll.ch> >>>> Cc: Javier Martinez Canillas <javierm@redhat.com> >>>> Cc: Thomas Zimmermann <tzimmermann@suse.de> >>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >>> >>> NAKed-by: Geert Uytterhoeven <geert@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@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 >
CC linux-fbdev On Tue, Apr 4, 2023 at 4:19 PM Daniel Vetter <daniel@ffwll.ch> wrote: > 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: > > > On Tue, Apr 4, 2023 at 2:36 PM Daniel Vetter <daniel.vetter@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@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. Please don't spread FUD: efifb, vesafb and offb do not implement fb_ops.fb_check_var(), so they are not affected. Gr{oetje,eeting}s, Geert
CC linux-fbdev On Tue, Apr 4, 2023 at 4:41 PM Thomas Zimmermann <tzimmermann@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@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@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@gmx.de> > >>>> Cc: Geert Uytterhoeven <geert@linux-m68k.org> > >>>> Cc: stable@vger.kernel.org # v5.4+ > >>>> Cc: Daniel Vetter <daniel@ffwll.ch> > >>>> Cc: Javier Martinez Canillas <javierm@redhat.com> > >>>> Cc: Thomas Zimmermann <tzimmermann@suse.de> > >>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > >>> > >>> NAKed-by: Geert Uytterhoeven <geert@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
On Tue, 4 Apr 2023 at 16:51, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > CC linux-fbdev > > On Tue, Apr 4, 2023 at 4:19 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > 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: > > > > On Tue, Apr 4, 2023 at 2:36 PM Daniel Vetter <daniel.vetter@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@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. > > Please don't spread FUD: efifb, vesafb and offb do not implement > fb_ops.fb_check_var(), so they are not affected. Hm I missed that early out. I'll do a patch to fix the drm fb helpers, as mentioned in the other thread I don't think we can actually just delete that because it would short-circuit out the fb_set_par call too. -Daniel > 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
Hi Daniel, On Tue, Apr 4, 2023 at 5:55 PM Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, 4 Apr 2023 at 16:51, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Tue, Apr 4, 2023 at 4:19 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > > 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: > > > > > On Tue, Apr 4, 2023 at 2:36 PM Daniel Vetter <daniel.vetter@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@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. > > > > Please don't spread FUD: efifb, vesafb and offb do not implement > > fb_ops.fb_check_var(), so they are not affected. > > Hm I missed that early out. I'll do a patch to fix the drm fb helpers, > as mentioned in the other thread I don't think we can actually just > delete that because it would short-circuit out the fb_set_par call > too. As I said to the other thread earlier today[1], I think we can keep the .fb_set_par() implementation. There's just no point in providing a .fb_check_var() callback if you don't support changing the video mode. [1] https://lore.kernel.org/all/CAMuHMdUaHd1jgrsCSxCqF-HP2rAo2ODM_ZOjhk7Q4vjuqvt36w@mail.gmail.com 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
On Tue, 4 Apr 2023 at 19:04, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Daniel, > > On Tue, Apr 4, 2023 at 5:55 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Tue, 4 Apr 2023 at 16:51, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Tue, Apr 4, 2023 at 4:19 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > 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: > > > > > > On Tue, Apr 4, 2023 at 2:36 PM Daniel Vetter <daniel.vetter@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@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. > > > > > > Please don't spread FUD: efifb, vesafb and offb do not implement > > > fb_ops.fb_check_var(), so they are not affected. > > > > Hm I missed that early out. I'll do a patch to fix the drm fb helpers, > > as mentioned in the other thread I don't think we can actually just > > delete that because it would short-circuit out the fb_set_par call > > too. > > As I said to the other thread earlier today[1], I think we can keep > the .fb_set_par() implementation. > There's just no point in providing a .fb_check_var() callback if > you don't support changing the video mode. If you don't have check_var then set_par just isn't called. Or am blind once more, not the first time today. And see the big comment in the drm set_par implementation, we can't ditch that because uabi fun in how the fbdev vs kms interactions are handled. -Daniel > [1] https://lore.kernel.org/all/CAMuHMdUaHd1jgrsCSxCqF-HP2rAo2ODM_ZOjhk7Q4vjuqvt36w@mail.gmail.com > 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
On 4/4/23 14:36, Daniel Vetter 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@syzkaller.appspotmail.com > Link: https://syzkaller.appspot.com/bug?id=c5faf983bfa4a607de530cd3bb008888bf06cefc > Fixes: 6c11df58fd1a ("fbmem: Check virtual screen sizes in fb_set_var()") > Cc: Helge Deller <deller@gmx.de> > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Cc: stable@vger.kernel.org # v5.4+ > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Javier Martinez Canillas <javierm@redhat.com> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/video/fbdev/core/fbmem.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index 875541ff185b..9757f4bcdf57 100644 > --- 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; > } > Make it pr_warn_once? 99.9...% of the benefit, without spam.
Hi Michel, On Wed, Apr 5, 2023 at 10:50 AM Michel Dänzer <michel.daenzer@mailbox.org> wrote: > On 4/4/23 14:36, Daniel Vetter 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@syzkaller.appspotmail.com > > Link: https://syzkaller.appspot.com/bug?id=c5faf983bfa4a607de530cd3bb008888bf06cefc > > Fixes: 6c11df58fd1a ("fbmem: Check virtual screen sizes in fb_set_var()") > > Cc: Helge Deller <deller@gmx.de> > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > Cc: stable@vger.kernel.org # v5.4+ > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: Javier Martinez Canillas <javierm@redhat.com> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/video/fbdev/core/fbmem.c | 4 ---- > > 1 file changed, 4 deletions(-) > > > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > > index 875541ff185b..9757f4bcdf57 100644 > > --- 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; > > } > > > > Make it pr_warn_once? 99.9...% of the benefit, without spam. Except that it should be pr_warn_once_per_fb_info, which requires a flag in fb_info. If fb_info.fbops wasn't const, we could also nuke info->fbops->check_var() ;-) Gr{oetje,eeting}s, Geert
On 4/11/23 11:10, Geert Uytterhoeven wrote: > Hi Michel, > > On Wed, Apr 5, 2023 at 10:50 AM Michel Dänzer > <michel.daenzer@mailbox.org> wrote: >> On 4/4/23 14:36, Daniel Vetter 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@syzkaller.appspotmail.com >>> Link: https://syzkaller.appspot.com/bug?id=c5faf983bfa4a607de530cd3bb008888bf06cefc >>> Fixes: 6c11df58fd1a ("fbmem: Check virtual screen sizes in fb_set_var()") >>> Cc: Helge Deller <deller@gmx.de> >>> Cc: Geert Uytterhoeven <geert@linux-m68k.org> >>> Cc: stable@vger.kernel.org # v5.4+ >>> Cc: Daniel Vetter <daniel@ffwll.ch> >>> Cc: Javier Martinez Canillas <javierm@redhat.com> >>> Cc: Thomas Zimmermann <tzimmermann@suse.de> >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >>> --- >>> drivers/video/fbdev/core/fbmem.c | 4 ---- >>> 1 file changed, 4 deletions(-) >>> >>> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c >>> index 875541ff185b..9757f4bcdf57 100644 >>> --- 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; >>> } >>> >> >> Make it pr_warn_once? 99.9...% of the benefit, without spam. > > Except that it should be pr_warn_once_per_fb_info, [...] Not really, that's what I mean by 99.9...% of the benefit. Even if a broken driver is masked on some systems, eventually the other driver masking it should get fixed, at which point the previously masked driver will be reported again.
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 875541ff185b..9757f4bcdf57 100644 --- 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; }
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@syzkaller.appspotmail.com Link: https://syzkaller.appspot.com/bug?id=c5faf983bfa4a607de530cd3bb008888bf06cefc Fixes: 6c11df58fd1a ("fbmem: Check virtual screen sizes in fb_set_var()") Cc: Helge Deller <deller@gmx.de> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: stable@vger.kernel.org # v5.4+ Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Javier Martinez Canillas <javierm@redhat.com> Cc: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/video/fbdev/core/fbmem.c | 4 ---- 1 file changed, 4 deletions(-)