Re: [PATCH] drm/fb-helper: Make set_var validation stricter

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

Am 21.06.22 um 12:20 schrieb Daniel Vetter:
On Tue, 21 Jun 2022 at 12:15, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:

Hi

Am 21.06.22 um 11:23 schrieb Daniel Vetter:
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.

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

This looks reasonable. We effectively only support a single resolution here.

+         var->xres_virtual != fb->width || var->yres_virtual != fb->height) {

AFAIU this change would require that all userspace always uses maximum
values for {xres,yres}_virtual. Seems unrealistic to me.

The thing is, they kinda have to, because that's always going to be
the actually visible part :-) Otoh I guess we could also allow virtual
size to match the fbdev real size, but maybe that kind of sanity check
should be done in fbmem.c?

I'm not sure I understand. I'd expect that fbdev userspace allocates xres_virtual to be twice the size of xres. So it can do double buffering. In many cases fb->height would be larger than that.


Tbh for these kind of things I'm leaning towards "let's wait until we
get a regression report", since there's simply too many random bugs

Not that I disagree, but in this case a regression report seems inevitable.

all over in the fbcon/vc/vt code when sou start resizing stuff. So I'm
very heavily leaning towards rejecting everything (and e.g. we should
have fixed this all up already in 2020 when the bugfix for x/yres
related underflows landed in 2020 imo).

Do the fbdev igt tests work with this change if overalloc has been set to more than 200?

Best regards
Thomas

-Daniel

Best regards
Thomas


               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,

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




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

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux