mbox series

[v3,0/4] fbcon: Fixes for screen resolution changes

Message ID 20220625220630.333705-1-deller@gmx.de
Headers show
Series fbcon: Fixes for screen resolution changes | expand

Message

Helge Deller June 25, 2022, 10:06 p.m. UTC 
This series fixes possible out-of-bound memory accesses when users trigger
screen resolutions changes with invalid input parameters, e.g. reconfigures
screen which is smaller than the current font size, or if the virtual screen
size is smaller than the physical screen size.

Helge Deller (4):
  fbcon: Disallow setting font bigger than screen size
  fbcon: Add fbcon_modechange_possible() check
  fbmem: Check screen resolution change against font size
  fbmem: Prevent invalid virtual screen sizes

 drivers/video/fbdev/core/fbcon.c | 32 ++++++++++++++++++++++++++++++++
 drivers/video/fbdev/core/fbmem.c | 10 +++++++++-
 include/linux/fbcon.h            |  4 ++++
 3 files changed, 45 insertions(+), 1 deletion(-)

--
2.35.3

Comments

Daniel Vetter June 25, 2022, 10:33 p.m. UTC | #1 
On Sun, Jun 26, 2022 at 12:06:29AM +0200, Helge Deller wrote:
> Enhance the check in the FBIOPUT_VSCREENINFO ioctl handler to verify if the
> user-provided new screen size is bigger than the current font size.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> Cc: stable@vger.kernel.org # v5.4+

Please squash with previous patch. You might also want to add a note there
that on older kernels backporters need to open-code
fbcon_info_from_console instead, since it only exists since 
409d6c95f9c6 ("fbcon: Introduce wrapper for console->fb_info lookup")

With these two nits: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/video/fbdev/core/fbmem.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index afa2863670f3..160389365a36 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1106,7 +1106,9 @@ 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);
> +		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, 10:37 p.m. UTC | #2 
On Sun, Jun 26, 2022 at 12:33:53AM +0200, Daniel Vetter wrote:
> On Sun, Jun 26, 2022 at 12:06:29AM +0200, Helge Deller wrote:
> > Enhance the check in the FBIOPUT_VSCREENINFO ioctl handler to verify if the
> > user-provided new screen size is bigger than the current font size.
> > 
> > Signed-off-by: Helge Deller <deller@gmx.de>
> > Cc: stable@vger.kernel.org # v5.4+
> 
> Please squash with previous patch. You might also want to add a note there
> that on older kernels backporters need to open-code
> fbcon_info_from_console instead, since it only exists since 
> 409d6c95f9c6 ("fbcon: Introduce wrapper for console->fb_info lookup")

Maybe easier would be to include that patch in the backports instead of
open coding. I think that's what Greg generally prefers at least, less
divergence between stable kernels.
-Daniel

> 
> With these two nits: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> > ---
> >  drivers/video/fbdev/core/fbmem.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> > index afa2863670f3..160389365a36 100644
> > --- a/drivers/video/fbdev/core/fbmem.c
> > +++ b/drivers/video/fbdev/core/fbmem.c
> > @@ -1106,7 +1106,9 @@ 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);
> > +		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, 11:12 p.m. UTC | #3 
On 6/26/22 00:37, Daniel Vetter wrote:
> On Sun, Jun 26, 2022 at 12:33:53AM +0200, Daniel Vetter wrote:
>> On Sun, Jun 26, 2022 at 12:06:29AM +0200, Helge Deller wrote:
>>> Enhance the check in the FBIOPUT_VSCREENINFO ioctl handler to verify if the
>>> user-provided new screen size is bigger than the current font size.
>>>
>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>> Cc: stable@vger.kernel.org # v5.4+
>>
>> Please squash with previous patch. You might also want to add a note there
>> that on older kernels backporters need to open-code
>> fbcon_info_from_console instead, since it only exists since
>> 409d6c95f9c6 ("fbcon: Introduce wrapper for console->fb_info lookup")
>
> Maybe easier would be to include that patch in the backports instead of
> open coding.

I was afraid that WARN_CONSOLE_UNLOCKED() hadn't been backported.
But it seems it's in v4.19+ (from patch 56e6c104e4f15), so that's ok.

So, yes, it seems pushing 409d6c95f9c6 backwards is probably best.

Will try that approach now.

Helge

 I think that's what Greg generally prefers at least, less
> divergence between stable kernels.
> -Daniel
>
>>
>> With these two nits: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>>> ---
>>>  drivers/video/fbdev/core/fbmem.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
>>> index afa2863670f3..160389365a36 100644
>>> --- a/drivers/video/fbdev/core/fbmem.c
>>> +++ b/drivers/video/fbdev/core/fbmem.c
>>> @@ -1106,7 +1106,9 @@ 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);
>>> +		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, 11:56 p.m. UTC | #4 
On 6/26/22 01:12, Helge Deller wrote:
> On 6/26/22 00:37, Daniel Vetter wrote:
>> On Sun, Jun 26, 2022 at 12:33:53AM +0200, Daniel Vetter wrote:
>>> On Sun, Jun 26, 2022 at 12:06:29AM +0200, Helge Deller wrote:
>>>> Enhance the check in the FBIOPUT_VSCREENINFO ioctl handler to verify if the
>>>> user-provided new screen size is bigger than the current font size.
>>>>
>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>> Cc: stable@vger.kernel.org # v5.4+
>>>
>>> Please squash with previous patch. You might also want to add a note there
>>> that on older kernels backporters need to open-code
>>> fbcon_info_from_console instead, since it only exists since
>>> 409d6c95f9c6 ("fbcon: Introduce wrapper for console->fb_info lookup")
>>
>> Maybe easier would be to include that patch in the backports instead of
>> open coding.
>
> I was afraid that WARN_CONSOLE_UNLOCKED() hadn't been backported.
> But it seems it's in v4.19+ (from patch 56e6c104e4f15), so that's ok.
>
> So, yes, it seems pushing 409d6c95f9c6 backwards is probably best.

It would be the best solution, but sadly 409d6c95f9c6 can't easily be backported.
So, probably my other approach (fix up afterwards with extra patch) is
the way to go.

What's your thought on this ?

Helge



> Will try that approach now.
>
> Helge
>
>  I think that's what Greg generally prefers at least, less
>> divergence between stable kernels.
>> -Daniel
>>
>>>
>>> With these two nits: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>
>>>> ---
>>>>  drivers/video/fbdev/core/fbmem.c | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
>>>> index afa2863670f3..160389365a36 100644
>>>> --- a/drivers/video/fbdev/core/fbmem.c
>>>> +++ b/drivers/video/fbdev/core/fbmem.c
>>>> @@ -1106,7 +1106,9 @@ 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);
>>>> +		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
>>
>
Daniel Vetter June 26, 2022, 8:49 a.m. UTC | #5 
On Sun, Jun 26, 2022 at 01:56:18AM +0200, Helge Deller wrote:
> On 6/26/22 01:12, Helge Deller wrote:
> > On 6/26/22 00:37, Daniel Vetter wrote:
> >> On Sun, Jun 26, 2022 at 12:33:53AM +0200, Daniel Vetter wrote:
> >>> On Sun, Jun 26, 2022 at 12:06:29AM +0200, Helge Deller wrote:
> >>>> Enhance the check in the FBIOPUT_VSCREENINFO ioctl handler to verify if the
> >>>> user-provided new screen size is bigger than the current font size.
> >>>>
> >>>> Signed-off-by: Helge Deller <deller@gmx.de>
> >>>> Cc: stable@vger.kernel.org # v5.4+
> >>>
> >>> Please squash with previous patch. You might also want to add a note there
> >>> that on older kernels backporters need to open-code
> >>> fbcon_info_from_console instead, since it only exists since
> >>> 409d6c95f9c6 ("fbcon: Introduce wrapper for console->fb_info lookup")
> >>
> >> Maybe easier would be to include that patch in the backports instead of
> >> open coding.
> >
> > I was afraid that WARN_CONSOLE_UNLOCKED() hadn't been backported.
> > But it seems it's in v4.19+ (from patch 56e6c104e4f15), so that's ok.
> >
> > So, yes, it seems pushing 409d6c95f9c6 backwards is probably best.
> 
> It would be the best solution, but sadly 409d6c95f9c6 can't easily be backported.
> So, probably my other approach (fix up afterwards with extra patch) is
> the way to go.

Ah right there's some conflicts with the restoration/removal of scroll
accel.

> What's your thought on this ?

I guess just open code in a separate backport is simplest.
-Daniel

> 
> Helge
> 
> 
> 
> > Will try that approach now.
> >
> > Helge
> >
> >  I think that's what Greg generally prefers at least, less
> >> divergence between stable kernels.
> >> -Daniel
> >>
> >>>
> >>> With these two nits: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>
> >>>> ---
> >>>>  drivers/video/fbdev/core/fbmem.c | 4 +++-
> >>>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> >>>> index afa2863670f3..160389365a36 100644
> >>>> --- a/drivers/video/fbdev/core/fbmem.c
> >>>> +++ b/drivers/video/fbdev/core/fbmem.c
> >>>> @@ -1106,7 +1106,9 @@ 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);
> >>>> +		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 26, 2022, 9 a.m. UTC | #6 
On 6/26/22 10:49, Daniel Vetter wrote:
> On Sun, Jun 26, 2022 at 01:56:18AM +0200, Helge Deller wrote:
>> On 6/26/22 01:12, Helge Deller wrote:
>>> On 6/26/22 00:37, Daniel Vetter wrote:
>>>> On Sun, Jun 26, 2022 at 12:33:53AM +0200, Daniel Vetter wrote:
>>>>> On Sun, Jun 26, 2022 at 12:06:29AM +0200, Helge Deller wrote:
>>>>>> Enhance the check in the FBIOPUT_VSCREENINFO ioctl handler to verify if the
>>>>>> user-provided new screen size is bigger than the current font size.
>>>>>>
>>>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>>>> Cc: stable@vger.kernel.org # v5.4+
>>>>>
>>>>> Please squash with previous patch. You might also want to add a note there
>>>>> that on older kernels backporters need to open-code
>>>>> fbcon_info_from_console instead, since it only exists since
>>>>> 409d6c95f9c6 ("fbcon: Introduce wrapper for console->fb_info lookup")
>>>>
>>>> Maybe easier would be to include that patch in the backports instead of
>>>> open coding.
>>>
>>> I was afraid that WARN_CONSOLE_UNLOCKED() hadn't been backported.
>>> But it seems it's in v4.19+ (from patch 56e6c104e4f15), so that's ok.
>>>
>>> So, yes, it seems pushing 409d6c95f9c6 backwards is probably best.
>>
>> It would be the best solution, but sadly 409d6c95f9c6 can't easily be backported.
>> So, probably my other approach (fix up afterwards with extra patch) is
>> the way to go.
>
> Ah right there's some conflicts with the restoration/removal of scroll
> accel.
>
>> What's your thought on this ?
>
> I guess just open code in a separate backport is simplest.

I think my just-sent series is somewhat smarter... use old open-coding
in patch which goes backwards, and then just fix up in v5.19 (where
commit 409d6c95f9c6 was added).

Helge



> -Daniel
>
>>
>> Helge
>>
>>
>>
>>> Will try that approach now.
>>>
>>> Helge
>>>
>>>  I think that's what Greg generally prefers at least, less
>>>> divergence between stable kernels.
>>>> -Daniel
>>>>
>>>>>
>>>>> With these two nits: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>>
>>>>>> ---
>>>>>>  drivers/video/fbdev/core/fbmem.c | 4 +++-
>>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
>>>>>> index afa2863670f3..160389365a36 100644
>>>>>> --- a/drivers/video/fbdev/core/fbmem.c
>>>>>> +++ b/drivers/video/fbdev/core/fbmem.c
>>>>>> @@ -1106,7 +1106,9 @@ 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);
>>>>>> +		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
>>>>
>>>
>>
>