[v2,3/4] fbmem: Fix input parameter checks for user-provided screen resolution changes

Enhance the checks in the FBIOPUT_VSCREENINFO ioctl handler to verify
the user-provided new screen size for:

a) virtual screen size >= physical screen size, and

b) new screen size is bigger than currently configured console font size.

Return -EINVAL on invalid input.

Signed-off-by: Helge Deller <deller@gmx.de>
Cc: stable@vger.kernel.org # v5.4+
---
 drivers/video/fbdev/core/fbmem.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

--
2.35.3

On Sat, Jun 25, 2022 at 02:25:01PM +0200, Helge Deller wrote:
> Enhance the checks in the FBIOPUT_VSCREENINFO ioctl handler to verify
> the user-provided new screen size for:
> 
> a) virtual screen size >= physical screen size, and
> 
> b) new screen size is bigger than currently configured console font size.
> 
> Return -EINVAL on invalid input.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> Cc: stable@vger.kernel.org # v5.4+

Imo squash this into the previous one please. Doesn't make sense to split
the patch which adds a function from it's callsite.
-Daniel

> ---
>  drivers/video/fbdev/core/fbmem.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index afa2863670f3..50fb66b954d6 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1106,7 +1106,13 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>  			return -EFAULT;
>  		console_lock();
>  		lock_fb_info(info);
> -		ret = fb_set_var(info, &var);
> +		if (var.xres_virtual < var.xres ||
> +		    var.yres_virtual < var.yres)
> +			ret = -EINVAL;
> +		if (!ret)
> +			ret = fbcon_modechange_possible(info, &var);
> +		if (!ret)
> +			ret = fb_set_var(info, &var);
>  		if (!ret)
>  			fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);
>  		unlock_fb_info(info);
> --
> 2.35.3
>

On 6/25/22 15:00, Daniel Vetter wrote:
> On Sat, Jun 25, 2022 at 02:56:42PM +0200, Daniel Vetter wrote:
>> On Sat, Jun 25, 2022 at 02:25:01PM +0200, Helge Deller wrote:
>>> Enhance the checks in the FBIOPUT_VSCREENINFO ioctl handler to verify
>>> the user-provided new screen size for:
>>>
>>> a) virtual screen size >= physical screen size, and
>>>
>>> b) new screen size is bigger than currently configured console font size.
>>>
>>> Return -EINVAL on invalid input.
>>>
>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>> Cc: stable@vger.kernel.org # v5.4+
>>
>> Imo squash this into the previous one please. Doesn't make sense to split
>> the patch which adds a function from it's callsite.
>
> Correction. The part to add the fbcon_modechange_possible call should be
> squashed into the previos patch.

Ok... based on my last mail, I then suggest to split that part out as another
follow-up patch. :-)

> The check for x/yres_virtaul < x/yres should imo be moved into fb_set_var,
> next to the other existing checks that have been added over time.

That was exactly the way I had coded it in the first round.
But you then suggested to move it to the ioctl code path...

I can easily change it back accordingly, but then we need to drop the
WARN_ON() [which means to drop PATCH 4] because otherwise you possibly
trigger the WARN_ON() if the user calls with wrong input values.
So, insted of
        if (WARN_ON(var->xres_virtual < var->xres)) ...
it will become:
        if (var->xres_virtual < var->xres) ...

I'll leave it up to you to decide...

Helge


> -Daniel
>
>> -Daniel
>>
>>> ---
>>>  drivers/video/fbdev/core/fbmem.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
>>> index afa2863670f3..50fb66b954d6 100644
>>> --- a/drivers/video/fbdev/core/fbmem.c
>>> +++ b/drivers/video/fbdev/core/fbmem.c
>>> @@ -1106,7 +1106,13 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>>>  			return -EFAULT;
>>>  		console_lock();
>>>  		lock_fb_info(info);
>>> -		ret = fb_set_var(info, &var);
>>> +		if (var.xres_virtual < var.xres ||
>>> +		    var.yres_virtual < var.yres)
>>> +			ret = -EINVAL;
>>> +		if (!ret)
>>> +			ret = fbcon_modechange_possible(info, &var);
>>> +		if (!ret)
>>> +			ret = fb_set_var(info, &var);
>>>  		if (!ret)
>>>  			fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);
>>>  		unlock_fb_info(info);
>>> --
>>> 2.35.3
>>>
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>