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

Tue Apr 4 14:41:38 UTC 2023

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 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.

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 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
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230404/29dcd137/attachment.sig>