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=c5faf983bfa4a607de530cd3bb008888bf06cef...
WARNING: fbcon: Driver 'vkmsdrmfb' missed to adjust virtual screensize (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_ZOjhk7Q4vjuqvt... 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