Re: [PATCH v2 2/4] fbcon: Add fbcon_modechange_possible() check

[Daniel Vetter <daniel@ffwll.ch>]

On Sat, Jun 25, 2022 at 02:25:00PM +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 may be extended with other checks later if necessary.  The
> new function will be called from the FBIOPUT_VSCREENINFO ioctl handler in
> fbmem.c, which will then return -EINVAL to the user if the new screen size is
> too small.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> Cc: stable@vger.kernel.org # v5.4+
> ---
>  drivers/video/fbdev/core/fbcon.c | 26 ++++++++++++++++++++++++++
>  include/linux/fbcon.h            |  4 ++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index e162d5e753e5..e4cc4841ed7f 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -2736,6 +2736,32 @@ 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();

here please.

> +
> +	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++) {

Maybe a follow up patch to make this an interator? Kinda like what I've
done for fbcon_for_each_registered_fb, maybe call it fbcon_for_each_fb_vc
or so.

> +		vc = vc_cons[i].d;
> +		if (!vc || vc->vc_mode != KD_TEXT ||

I don't think it's good to filter for !KD_TEXT here, because then we'd
need to recheck fonts when Xorg would try to switch back to text mode, and
if that then fails we kinda broke the system.

I can't think of a use-case where you'd want to upload a font which breaks
your console that Xorg is using right now, so best to just drop this
check.

> +		    fbcon_info_from_console(i) != info)

So I think, but not 100% sure, that with my recent rework for
fbcon_info_from_console this should be impossible, since the races are
gone. I guess it doesn't hurt to cargo-cult this, but a follow up patch to
roll out fbcon_for_each_fb_vc and then delete this check from all of the
loop iterations would be really good to make this clear.

If you're not sure this is safe we could add this consistency check in a
if (WARN_ON(fbcon_info_from_console(i))!= info) continue; into the loop
iterator itself.

> +			continue;
> +
> +		if (FBCON_SWAP(var->rotate, var->xres, var->yres) < vc->vc_font.width ||
> +		    FBCON_SWAP(var->rotate, var->yres, var->xres) < vc->vc_font.height)

Bit a bikeshed, but please do the check the same way around as in the
other place. Maybe even extract a helper that you pass the vc and the var
struct too and it checks that it fits, and then use it here and in the
previous patch.

Cheers, Daniel

> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(fbcon_modechange_possible);
> +
>  int fbcon_mode_deleted(struct fb_info *info,
>  		       struct fb_videomode *mode)
>  {
> 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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch