On 6/21/22 17:40, Daniel Vetter wrote:
> On Tue, 21 Jun 2022 at 17:37, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote:
>>
>> 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.
>
> Like I'm pretty sure if you make the font big enough and yres small
> enough this can all blow up right away again, but I also really don't
> want to find out.
Yes, the font thing is something which came to my mind as well.
But that's probably another patch...
> And once you fix that you get to fix the races of changing fonts
> through vt against changing resolution through fbdev ioctls, and at
> that point you have a neat locking problem to solve.
Yes, maybe.
But people need/want to be able to change screen resolutions and fonts, so
it has to be made working somehow, e.g. by returning -EAGAIN temporarily.
Helge
> -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
>
>
>
