Hi Daniel, On Tue, Aug 9, 2022 at 5:03 PM Daniel Vetter <daniel@xxxxxxxx> wrote: > On Sat, Jul 02, 2022 at 08:05:54PM +0200, Helge Deller wrote: > > On 6/29/22 12:56, Geert Uytterhoeven wrote: > > > The DRM fbdev emulation layer does not support pushing back > > > changes to fb_var_screeninfo to KMS. > > > > > > However, drm_fb_helper still implements the fb_ops.fb_check_var() and > > > fb_ops.fb_set_par() callbacks, but the former fails to validate various > > > parameters passed from userspace. Hence unsanitized and possible > > > invaled values are passed up through the fbdev, fbcon, and console > > > stack, which has become an endless source of security issues reported > > > against fbdev. > > > > > > Fix this by not populating the fb_ops.fb_check_var() and > > > fb_ops.fb_set_par() callbacks, as there is no point in providing them if > > > the frame buffer config cannot be changed anyway. This makes the fbdev > > > core aware that making changes to the frame buffer config is not > > > supported, so it will always return the current config. > > > > > > Signed-off-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> > > > > It's unfortunate that DRM currently isn't able to switch resolutions > > at runtime. > > > > With that in mind I agree with Geert that it's probably better to > > disable (or drop) that code until DRM can cope with fbdev's > > interface to switch resolutions. > > > > Furthermore, with the upcoming patches to fbcon (which prevents crashes on > > invalid userspace input), you will face a kernel WARNING if you call fbset > > to switch screen resolutions with DRM drivers. > > > > So, from my side (although I'd prefer a better fix for DRM): > > > > Acked-by: Helge Deller <deller@xxxxxx> > > So maybe I'm missing something, but I think this breaks a lot of stuff. > The issue is that fbdev is only a subordinate owner of the kms device, if > there's a real drm kms owner around that wins. > > Which means when you switch back then set_par needs to restore the fbdev > framebuffer. So that might break some recovery/use-cases. Upon closer look, I think I was a bit too over-eager, and we can keep the implementation of fb_ops.fb_set_par(). > The other thing is that I think this also breaks the scanout offset, and > people do use that for double-buffering on top of fbdev on top of drm > drivers. So we can't just nuke it completely. You mean panning? That does not need fb_ops.fb_check_var(), as it should be done through fb_ops.fb_pan_display(). > For better or worse I think we need to keep playing the whack-a-mole game. > Or do I miss something here? With the above fixed, we can continue whacking the drm bugs in implementing the fbdev API? 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