[PATCH v2 3/4] fbmem: Prevent invalid virtual screen sizes in fb_set_var()

[Helge Deller]

Hi Daniel & DRM developers,

could you please comment on the change below?


On 7/3/22 16:41, Geert Uytterhoeven wrote:
> Hi Helge,
>
> On Sun, Jul 3, 2022 at 3:54 PM Helge Deller <deller at gmx.de> wrote:
>> * Geert Uytterhoeven <geert at linux-m68k.org>:
>>>> --- a/drivers/video/fbdev/core/fbmem.c
>>>> +++ b/drivers/video/fbdev/core/fbmem.c
>>>> @@ -1016,6 +1016,18 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>>>>         if (ret)
>>>>                 return ret;
>>>>
>>>> +       /* make sure virtual resolution >= physical resolution */
>>>> +       if (WARN_ON(var->xres_virtual < var->xres)) {
>>>
>>> WARN_ON_ONCE()?
>>> This does mean we would miss two or more buggy drivers in a single system.
>>>
>>>> +               pr_warn("fbcon: Fix up invalid xres %d for %s\n",
>>>
>>> xres_virtual?
>>>
>>>> +                       var->xres_virtual, info->fix.id);
>>>> +               var->xres_virtual = var->xres;
>>>
>>> I think it's better to not fix this up, but return -EINVAL instead.
>>> After all if we get here, we have a buggy driver that needs to be fixed.
>>>
>>>> +       }
>>>> +       if (WARN_ON(var->yres_virtual < var->yres)) {
>>>> +               pr_warn("fbcon: Fix up invalid yres %d for %s\n",
>>>> +                       var->yres_virtual, info->fix.id);
>>>> +               var->yres_virtual = var->yres;
>>>> +       }
>>>
>>> Same for yres.
>>
>> Geert, thanks for your valuable feedback!
>>
>> In general I don't like for this case any of the WARN_ON* functions.
>> They don't provide any useful info for us, dumps unneccessarily the
>> stack backtrace and would annoy existing users.
>
> Without the stack trace, most people won't notice...
>
>> We know, that DRM drivers can't change the resolution. If we would leave
>> in any kind of warning, all DRM users will ask back - and we don't have
>> a solution for them. It's also no regression, because it didn't worked
>> before either.
>
> The warning will only be triggered when the requested virtual
> resolution is smaller than the requested physical resolution.  As
> long as the requested virtual and physical resolution match what
> was returned by FBIOGET_VSCREENINFO before, the warning won't
> be triggered.  So in normal use cases, the user won't be impacted.
> Fuzzers will see the warning, but the kernel will no longer crash
> on invalid values.

Still, fuzzers will crash if they have panic_on_warn enabled, which
was the case for the reproducer I got.

>> But we want to detect fbdev drivers which behave badly - so we should
>> print that info with the driver name.
>>
>> Below is a new patch which addresses this. The search for drm drivers
>> looks somewhat hackish - maybe someone can propose a better approach?
>>
>> Thoughts?
>>
>> Helge
>>
>>
>> From 0f33e2a3730342ab85df372f80b4f61762fb1130 Mon Sep 17 00:00:00 2001
>> From: Helge Deller <deller at gmx.de>
>> Date: Wed, 29 Jun 2022 15:53:55 +0200
>> Subject: [PATCH] fbmem: Check virtual screen sizes in fb_set_var()
>>
>> Verify that the fbdev or drm driver correctly adjusted the virtual screen
>> sizes. On failure report (in case of fbdev drivers) the failing driver.
>>
>> Signed-off-by: Helge Deller <deller at gmx.de>
>> Cc: stable at vger.kernel.org # v5.4+
>>
>> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
>> index 160389365a36..9b75aa4405ee 100644
>> --- a/drivers/video/fbdev/core/fbmem.c
>> +++ b/drivers/video/fbdev/core/fbmem.c
>> @@ -1016,6 +1016,21 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>>         if (ret)
>>                 return ret;
>>
>> +       /* verify that virtual resolution >= physical resolution */
>> +       if (var->xres_virtual < var->xres ||
>> +           var->yres_virtual < var->yres) {

This is the part I'd like to get feedback from DRM on:
Shall we leave that in, or drop it as Geert suggested?

>> +               /* drm drivers don't support mode changes yet. Do not report. */
>> +               if (strstr(info->fix.id, "drm"))
>> +                       return -ENOTSUPP;





>> +
>> +               pr_warn("WARNING: fbcon: Driver %s missed to adjust virtual"
>> +                       " screen size (%dx%d vs. %dx%d)\n",
>> +                       info->fix.id,
>> +                       var->xres_virtual, var->yres_virtual,
>> +                       var->xres, var->yres);
>> +               return -EINVAL;
>> +       }
>> +
>>         if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
>>                 return 0;
>
> Hence I think there is no need for ignoring drm.