Re: [PATCH] drm/fb-helper: Remove helpers to change frame buffer config

[Geert Uytterhoeven]

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