diff mbox series

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

Message ID 20220625122502.68095-4-deller@gmx.de (mailing list archive)
State Superseded
Headers show
Series fbcon: Fixes for screen resolution changes | expand

Commit Message

Helge Deller June 25, 2022, 12:25 p.m. UTC 
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

Comments

Daniel Vetter June 25, 2022, 12:56 p.m. UTC | #1 
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
>
Daniel Vetter June 25, 2022, 1 p.m. UTC | #2 
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.

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.
-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
Helge Deller June 25, 2022, 3:19 p.m. UTC | #3 
On 6/25/22 14:56, 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.

I do disagree on this.
In my experience it's often much easier for backporting to have a patch which
provides a new generic function and the patches with the callers of it in seperate patches.

I'm not religious about this opinion here, so if you REALLY want it, I'll change
it. But personally I think this isn't a good idea and would prefer to leave it in seperate patches.

Helge


> -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
>>
>
Helge Deller June 25, 2022, 3:36 p.m. UTC | #4 
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
>
diff mbox series

Patch

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