On Tue, 21 Jun 2022 at 16:57, Helge Deller <deller@xxxxxx> wrote:
>
> On 6/21/22 11:23, Daniel Vetter wrote:
> > The drm fbdev emulation does not forward mode changes to the driver,
> > and hence all changes where rejected in 865afb11949e ("drm/fb-helper:
> > reject any changes to the fbdev").
> >
> > Unfortunately this resulted in bugs on multiple monitor systems with
> > different resolutions. In that case the fbdev emulation code sizes the
> > underlying framebuffer for the largest screen (which dictates
> > x/yres_virtual), but adjust the fbdev x/yres to match the smallest
> > resolution. The above mentioned patch failed to realize that, and
> > errornously validated x/yres against the fb dimensions.
> >
> > This was fixed by just dropping the validation for too small sizes,
> > which restored vt switching with 12ffed96d436 ("drm/fb-helper: Allow
> > var->x/yres(_virtual) < fb->width/height again").
> >
> > But this also restored all kinds of validation issues and their
> > fallout in the notoriously buggy fbcon code for too small sizes. Since
> > no one is volunteering to really make fbcon and vc/vt fully robust
> > against these math issues make sure this barn door is closed for good
> > again.
>
> I don't understand why you are blaming fbcon here (again)...
>
> The real problem is that user-provided input (virt/physical screen sizes)
> isn't correctly validated.
> And that's even what your patch below does.
I don't want to play whack-a-mole in here. And what I tried to do here
(but oh well, too many things would break) is outright disallow any
changes, not just try to validate (and probably in vain) that the
changes look decent. Because making stuff invariant also solves all
the locking fun. And sure even then we could have bugs that break
stuff, but since everything would be invariant people would notice
when booting, instead of trying to hit corner cases using syzkaller
for stuff that mostly only syzkaller exercises.
And I'm pretty sure syzkaller isn't good enough to really hit
concurrency issues, it has a pretty hard time just finding basic
validation bugs like this.
-Daniel
> Helge
>
> > Since it's a bit tricky to remember the x/yres we picked across both
> > the newer generic fbdev emulation and the older code with more driver
> > involvement, we simply check that it doesn't change. This relies on
> > drm_fb_helper_fill_var() having done things correctly, and nothing
> > having trampled it yet.
> >
> > Note that this leaves all the other fbdev drivers out in the rain.
> > Given that distros have finally started to move away from those
> > completely for real I think that's good enough. The code it spaghetti
> > enough that I do not feel confident to even review fixes for it.
> >
> > What might help fbdev is doing something similar to what was done in
> > a49145acfb97 ("fbmem: add margin check to fb_check_caps()") and ensure
> > x/yres_virtual aren't too small, for some value of "too small". Maybe
> > checking that they're at least x/yres makes sense?
> >
> > Fixes: 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again")
> > Cc: Michel Dänzer <michel.daenzer@xxxxxxx>
> > Cc: Daniel Stone <daniels@xxxxxxxxxxxxx>
> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> > Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx> # v4.11+
> > Cc: Helge Deller <deller@xxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: openeuler-security@xxxxxxxxxxxxx
> > Cc: guodaxing@xxxxxxxxxx
> > Cc: Weigang (Jimmy) <weigang12@xxxxxxxxxx>
> > Reported-by: Weigang (Jimmy) <weigang12@xxxxxxxxxx>
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > ---
> > Note: Weigang asked for this to stay under embargo until it's all
> > review and tested.
> > -Daniel
> > ---
> > drivers/gpu/drm/drm_fb_helper.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index 695997ae2a7c..5664a177a404 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -1355,8 +1355,8 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> > * to KMS, hence fail if different settings are requested.
> > */
> > if (var->bits_per_pixel > fb->format->cpp[0] * 8 ||
> > - var->xres > fb->width || var->yres > fb->height ||
> > - var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
> > + var->xres != info->var.xres || var->yres != info->var.yres ||
> > + var->xres_virtual != fb->width || var->yres_virtual != fb->height) {
> > drm_dbg_kms(dev, "fb requested width/height/bpp can't fit in current fb "
> > "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
> > var->xres, var->yres, var->bits_per_pixel,
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
