mbox series

[v4,0/3] fbcon: Fixes for screen resolution changes

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

Message

Helge Deller June 25, 2022, 11:27 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.

v4:
- merged former patch #2 and #3
- added more Reviewed-by
- new patch #2 needs more testing. Note added to comit message that
  another patch needs to be backport as well.

Helge Deller (3):
  fbcon: Disallow setting font bigger than screen size
  fbcon: Prevent that screen size is smaller than 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 26, 2022, 8:50 a.m. UTC | #1 
On Sun, Jun 26, 2022 at 01:27:02AM +0200, Helge Deller wrote:
> We need to prevent that users configure a screen size which is smaller than the
> currently selected font size. Otherwise rendering chars on the screen will
> access memory outside the graphics memory region.
> 
> This patch adds a new function fbcon_modechange_possible() which
> implements this check and which later may be extended with other checks
> if necessary.  The new function is called from the FBIOPUT_VSCREENINFO
> ioctl handler in fbmem.c, which will return -EINVAL if userspace asked
> for a too small screen size.
> 
> This patch depends on commit 409d6c95f9c6 ("fbcon: Introduce wrapper for console->fb_info lookup"),
> so it needs to be backported as well.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> Cc: stable@vger.kernel.org # v5.4+

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/video/fbdev/core/fbcon.c | 27 +++++++++++++++++++++++++++
>  drivers/video/fbdev/core/fbmem.c |  4 +++-
>  include/linux/fbcon.h            |  4 ++++
>  3 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index e162d5e753e5..2ab7515ac842 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -2736,6 +2736,33 @@ void fbcon_update_vcs(struct fb_info *info, bool all)
>  }
>  EXPORT_SYMBOL(fbcon_update_vcs);
> 
> +/* let fbcon check if it supports a new screen resolution */
> +int fbcon_modechange_possible(struct fb_info *info, struct fb_var_screeninfo *var)
> +{
> +	struct fbcon_ops *ops = info->fbcon_par;
> +	struct vc_data *vc;
> +	int i;
> +
> +	WARN_CONSOLE_UNLOCKED();
> +
> +	if (!ops || ops->currcon < 0)
> +		return -EINVAL;
> +
> +	/* prevent setting a screen size which is smaller than font size */
> +	for (i = first_fb_vc; i <= last_fb_vc; i++) {
> +		vc = vc_cons[i].d;
> +		if (!vc || fbcon_info_from_console(i) != info)
> +			continue;
> +
> +		if (vc->vc_font.width  > FBCON_SWAP(var->rotate, var->xres, var->yres) ||
> +		    vc->vc_font.height > FBCON_SWAP(var->rotate, var->yres, var->xres))
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(fbcon_modechange_possible);
> +
>  int fbcon_mode_deleted(struct fb_info *info,
>  		       struct fb_videomode *mode)
>  {
> 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);
> diff --git a/include/linux/fbcon.h b/include/linux/fbcon.h
> index ff5596dd30f8..2382dec6d6ab 100644
> --- a/include/linux/fbcon.h
> +++ b/include/linux/fbcon.h
> @@ -15,6 +15,8 @@ void fbcon_new_modelist(struct fb_info *info);
>  void fbcon_get_requirement(struct fb_info *info,
>  			   struct fb_blit_caps *caps);
>  void fbcon_fb_blanked(struct fb_info *info, int blank);
> +int  fbcon_modechange_possible(struct fb_info *info,
> +			       struct fb_var_screeninfo *var);
>  void fbcon_update_vcs(struct fb_info *info, bool all);
>  void fbcon_remap_all(struct fb_info *info);
>  int fbcon_set_con2fb_map_ioctl(void __user *argp);
> @@ -33,6 +35,8 @@ static inline void fbcon_new_modelist(struct fb_info *info) {}
>  static inline void fbcon_get_requirement(struct fb_info *info,
>  					 struct fb_blit_caps *caps) {}
>  static inline void fbcon_fb_blanked(struct fb_info *info, int blank) {}
> +static inline int  fbcon_modechange_possible(struct fb_info *info,
> +				struct fb_var_screeninfo *var) { return 0; }
>  static inline void fbcon_update_vcs(struct fb_info *info, bool all) {}
>  static inline void fbcon_remap_all(struct fb_info *info) {}
>  static inline int fbcon_set_con2fb_map_ioctl(void __user *argp) { return 0; }
> --
> 2.35.3
>
Helge Deller June 26, 2022, 8:57 a.m. UTC | #2 
On 6/26/22 10:50, Daniel Vetter wrote:
> On Sun, Jun 26, 2022 at 01:27:02AM +0200, Helge Deller wrote:
>> We need to prevent that users configure a screen size which is smaller than the
>> currently selected font size. Otherwise rendering chars on the screen will
>> access memory outside the graphics memory region.
>>
>> This patch adds a new function fbcon_modechange_possible() which
>> implements this check and which later may be extended with other checks
>> if necessary.  The new function is called from the FBIOPUT_VSCREENINFO
>> ioctl handler in fbmem.c, which will return -EINVAL if userspace asked
>> for a too small screen size.
>>
>> This patch depends on commit 409d6c95f9c6 ("fbcon: Introduce wrapper for console->fb_info lookup"),
>> so it needs to be backported as well.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> Cc: stable@vger.kernel.org # v5.4+
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks, but sadly pushing commit 409d6c95f9c6 isn't easily possible.
I've just sent a new series v5 which fixes it up..

Helge

>
>> ---
>>  drivers/video/fbdev/core/fbcon.c | 27 +++++++++++++++++++++++++++
>>  drivers/video/fbdev/core/fbmem.c |  4 +++-
>>  include/linux/fbcon.h            |  4 ++++
>>  3 files changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
>> index e162d5e753e5..2ab7515ac842 100644
>> --- a/drivers/video/fbdev/core/fbcon.c
>> +++ b/drivers/video/fbdev/core/fbcon.c
>> @@ -2736,6 +2736,33 @@ void fbcon_update_vcs(struct fb_info *info, bool all)
>>  }
>>  EXPORT_SYMBOL(fbcon_update_vcs);
>>
>> +/* let fbcon check if it supports a new screen resolution */
>> +int fbcon_modechange_possible(struct fb_info *info, struct fb_var_screeninfo *var)
>> +{
>> +	struct fbcon_ops *ops = info->fbcon_par;
>> +	struct vc_data *vc;
>> +	int i;
>> +
>> +	WARN_CONSOLE_UNLOCKED();
>> +
>> +	if (!ops || ops->currcon < 0)
>> +		return -EINVAL;
>> +
>> +	/* prevent setting a screen size which is smaller than font size */
>> +	for (i = first_fb_vc; i <= last_fb_vc; i++) {
>> +		vc = vc_cons[i].d;
>> +		if (!vc || fbcon_info_from_console(i) != info)
>> +			continue;
>> +
>> +		if (vc->vc_font.width  > FBCON_SWAP(var->rotate, var->xres, var->yres) ||
>> +		    vc->vc_font.height > FBCON_SWAP(var->rotate, var->yres, var->xres))
>> +			return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(fbcon_modechange_possible);
>> +
>>  int fbcon_mode_deleted(struct fb_info *info,
>>  		       struct fb_videomode *mode)
>>  {
>> 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);
>> diff --git a/include/linux/fbcon.h b/include/linux/fbcon.h
>> index ff5596dd30f8..2382dec6d6ab 100644
>> --- a/include/linux/fbcon.h
>> +++ b/include/linux/fbcon.h
>> @@ -15,6 +15,8 @@ void fbcon_new_modelist(struct fb_info *info);
>>  void fbcon_get_requirement(struct fb_info *info,
>>  			   struct fb_blit_caps *caps);
>>  void fbcon_fb_blanked(struct fb_info *info, int blank);
>> +int  fbcon_modechange_possible(struct fb_info *info,
>> +			       struct fb_var_screeninfo *var);
>>  void fbcon_update_vcs(struct fb_info *info, bool all);
>>  void fbcon_remap_all(struct fb_info *info);
>>  int fbcon_set_con2fb_map_ioctl(void __user *argp);
>> @@ -33,6 +35,8 @@ static inline void fbcon_new_modelist(struct fb_info *info) {}
>>  static inline void fbcon_get_requirement(struct fb_info *info,
>>  					 struct fb_blit_caps *caps) {}
>>  static inline void fbcon_fb_blanked(struct fb_info *info, int blank) {}
>> +static inline int  fbcon_modechange_possible(struct fb_info *info,
>> +				struct fb_var_screeninfo *var) { return 0; }
>>  static inline void fbcon_update_vcs(struct fb_info *info, bool all) {}
>>  static inline void fbcon_remap_all(struct fb_info *info) {}
>>  static inline int fbcon_set_con2fb_map_ioctl(void __user *argp) { return 0; }
>> --
>> 2.35.3
>>
>