[PATCH v2 0/4] fbcon: Fixes for screen resolution changes

[PATCH v2 0/4] fbcon: Fixes for screen resolution changes

[Helge Deller]

This series fixes possible out-of-bound memory accesses when users trigger
screen resolutions changes with invalid input parameters, e.g.  configure
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: Fix input parameter checks for user-provided screen resolution
    changes
  fbmem: Catch possible driver bugs regarding too small virtual screen
    size

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

--
2.35.3

[PATCH v2 1/4] fbcon: Disallow setting font bigger than screen size

[Helge Deller]

Prevent that users set a font size which is bigger than the physical screen.
It's unlikely this may happen (because screens are usually much larger than the
fonts and each font char is limited to 32x32 pixels), but it may happen on
smaller screens/LCD displays.

Signed-off-by: Helge Deller <deller@gmx.de>
Cc: stable@vger.kernel.org # v4.14+
---
 drivers/video/fbdev/core/fbcon.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index c4e91715ef00..e162d5e753e5 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2469,6 +2469,11 @@ static int fbcon_set_font(struct vc_data *vc, struct console_font *font,
 	if (charcount != 256 && charcount != 512)
 		return -EINVAL;

+	/* font bigger than screen resolution ? */
+	if (font->width  > FBCON_SWAP(info->var.rotate, info->var.xres, info->var.yres) ||
+	    font->height > FBCON_SWAP(info->var.rotate, info->var.yres, info->var.xres))
+		return -EINVAL;
+
 	/* Make sure drawing engine can handle the font */
 	if (!(info->pixmap.blit_x & (1 << (font->width - 1))) ||
 	    !(info->pixmap.blit_y & (1 << (font->height - 1))))
--
2.35.3

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

[Helge Deller]

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;
+
+	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 || vc->vc_mode != KD_TEXT ||
+		    fbcon_info_from_console(i) != info)
+			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)
+			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

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

[Helge Deller]

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

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

[PATCH v2 4/4] fbmem: Catch possible driver bugs regarding too small virtual screen size

[Helge Deller]

Make sure that we catch, report and fix up fbdev and drm graphic drivers which
got the virtual screen resolution smaller than the physical screen resolution.

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

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 50fb66b954d6..6d262e341023 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1006,6 +1006,12 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
 	if (var->xres < 8 || var->yres < 8)
 		return -EINVAL;

+	/* make sure virtual resolution >= physical resolution */
+	if (WARN_ON(var->xres_virtual < var->xres))
+		var->xres_virtual = var->xres;
+	if (WARN_ON(var->yres_virtual < var->yres))
+		var->yres_virtual = var->yres;
+
 	/* Too huge resolution causes multiplication overflow. */
 	if (check_mul_overflow(var->xres, var->yres, &unused) ||
 	    check_mul_overflow(var->xres_virtual, var->yres_virtual, &unused))
--
2.35.3

Re: [PATCH v2 1/4] fbcon: Disallow setting font bigger than screen size

[Daniel Vetter]

On Sat, Jun 25, 2022 at 02:24:59PM +0200, Helge Deller wrote:
> Prevent that users set a font size which is bigger than the physical screen.
> It's unlikely this may happen (because screens are usually much larger than the
> fonts and each font char is limited to 32x32 pixels), but it may happen on
> smaller screens/LCD displays.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> Cc: stable@vger.kernel.org # v4.14+
> ---
>  drivers/video/fbdev/core/fbcon.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index c4e91715ef00..e162d5e753e5 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -2469,6 +2469,11 @@ static int fbcon_set_font(struct vc_data *vc, struct console_font *font,
>  	if (charcount != 256 && charcount != 512)
>  		return -EINVAL;
> 
> +	/* font bigger than screen resolution ? */
> +	if (font->width  > FBCON_SWAP(info->var.rotate, info->var.xres, info->var.yres) ||
> +	    font->height > FBCON_SWAP(info->var.rotate, info->var.yres, info->var.xres))
> +		return -EINVAL;

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

Maybe as a safety follow up patch, we have a few copies of the below:

	cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
	rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
	cols /= vc->vc_font.width;
	rows /= vc->vc_font.height;

Might be good to extract that into a helper and also add

	WARN_ON(!cols);
	WARN_ON(!rows);

to make sure we really didn't screw this up and give syzkaller et all an
easier time finding bugs - it doesn't need to discover the full exploit,
only needs to get to this here.

Also maybe even check that cols/rows is within reasons, like smaller than
BIT(24) or so (so that we have a bunch of headroom for overflows).

Anyway just an idea.
-Daniel

> +
>  	/* Make sure drawing engine can handle the font */
>  	if (!(info->pixmap.blit_x & (1 << (font->width - 1))) ||
>  	    !(info->pixmap.blit_y & (1 << (font->height - 1))))
> --
> 2.35.3
> 

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

Re: [PATCH v2 1/4] fbcon: Disallow setting font bigger than screen size

[Daniel Vetter]

On Sat, Jun 25, 2022 at 02:24:59PM +0200, Helge Deller wrote:
> Prevent that users set a font size which is bigger than the physical screen.
> It's unlikely this may happen (because screens are usually much larger than the
> fonts and each font char is limited to 32x32 pixels), but it may happen on
> smaller screens/LCD displays.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> Cc: stable@vger.kernel.org # v4.14+
> ---
>  drivers/video/fbdev/core/fbcon.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index c4e91715ef00..e162d5e753e5 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -2469,6 +2469,11 @@ static int fbcon_set_font(struct vc_data *vc, struct console_font *font,
>  	if (charcount != 256 && charcount != 512)
>  		return -EINVAL;
> 
> +	/* font bigger than screen resolution ? */
> +	if (font->width  > FBCON_SWAP(info->var.rotate, info->var.xres, info->var.yres) ||
> +	    font->height > FBCON_SWAP(info->var.rotate, info->var.yres, info->var.xres))
> +		return -EINVAL;

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

Maybe as a safety follow up patch, we have a few copies of the below:

	cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
	rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
	cols /= vc->vc_font.width;
	rows /= vc->vc_font.height;

Might be good to extract that into a helper and also add

	WARN_ON(!cols);
	WARN_ON(!rows);

to make sure we really didn't screw this up and give syzkaller et all an
easier time finding bugs - it doesn't need to discover the full exploit,
only needs to get to this here.

Also maybe even check that cols/rows is within reasons, like smaller than
BIT(24) or so (so that we have a bunch of headroom for overflows).

Anyway just an idea.
-Daniel

> +
>  	/* Make sure drawing engine can handle the font */
>  	if (!(info->pixmap.blit_x & (1 << (font->width - 1))) ||
>  	    !(info->pixmap.blit_y & (1 << (font->height - 1))))
> --
> 2.35.3
> 

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

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

[Daniel Vetter]

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

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

[Daniel Vetter]

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

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

[Daniel Vetter]

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
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Daniel Vetter]

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
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Daniel Vetter]

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

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

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

[Daniel Vetter]

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

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

Re: [PATCH v2 4/4] fbmem: Catch possible driver bugs regarding too small virtual screen size

[Daniel Vetter]

On Sat, Jun 25, 2022 at 02:25:02PM +0200, Helge Deller wrote:
> Make sure that we catch, report and fix up fbdev and drm graphic drivers which
> got the virtual screen resolution smaller than the physical screen resolution.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> Cc: stable@vger.kernel.org # v5.4+

No cc: stable for this, it's not fixing any bugs just helps validate
driver code.

Also if you just move the check from the ioctl code to here we don't need
to duplicate anything (and drivers which don't check their set_var are
kinda busted no matter what).
-Daniel

> ---
>  drivers/video/fbdev/core/fbmem.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 50fb66b954d6..6d262e341023 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1006,6 +1006,12 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>  	if (var->xres < 8 || var->yres < 8)
>  		return -EINVAL;
> 
> +	/* make sure virtual resolution >= physical resolution */
> +	if (WARN_ON(var->xres_virtual < var->xres))
> +		var->xres_virtual = var->xres;
> +	if (WARN_ON(var->yres_virtual < var->yres))
> +		var->yres_virtual = var->yres;
> +
>  	/* Too huge resolution causes multiplication overflow. */
>  	if (check_mul_overflow(var->xres, var->yres, &unused) ||
>  	    check_mul_overflow(var->xres_virtual, var->yres_virtual, &unused))
> --
> 2.35.3
> 

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

Re: [PATCH v2 4/4] fbmem: Catch possible driver bugs regarding too small virtual screen size

[Daniel Vetter]

On Sat, Jun 25, 2022 at 02:25:02PM +0200, Helge Deller wrote:
> Make sure that we catch, report and fix up fbdev and drm graphic drivers which
> got the virtual screen resolution smaller than the physical screen resolution.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> Cc: stable@vger.kernel.org # v5.4+

No cc: stable for this, it's not fixing any bugs just helps validate
driver code.

Also if you just move the check from the ioctl code to here we don't need
to duplicate anything (and drivers which don't check their set_var are
kinda busted no matter what).
-Daniel

> ---
>  drivers/video/fbdev/core/fbmem.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 50fb66b954d6..6d262e341023 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1006,6 +1006,12 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>  	if (var->xres < 8 || var->yres < 8)
>  		return -EINVAL;
> 
> +	/* make sure virtual resolution >= physical resolution */
> +	if (WARN_ON(var->xres_virtual < var->xres))
> +		var->xres_virtual = var->xres;
> +	if (WARN_ON(var->yres_virtual < var->yres))
> +		var->yres_virtual = var->yres;
> +
>  	/* Too huge resolution causes multiplication overflow. */
>  	if (check_mul_overflow(var->xres, var->yres, &unused) ||
>  	    check_mul_overflow(var->xres_virtual, var->yres_virtual, &unused))
> --
> 2.35.3
> 

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

Re: [PATCH v2 1/4] fbcon: Disallow setting font bigger than screen size

[Helge Deller]

On 6/25/22 14:45, Daniel Vetter wrote:
> On Sat, Jun 25, 2022 at 02:24:59PM +0200, Helge Deller wrote:
>> Prevent that users set a font size which is bigger than the physical screen.
>> It's unlikely this may happen (because screens are usually much larger than the
>> fonts and each font char is limited to 32x32 pixels), but it may happen on
>> smaller screens/LCD displays.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> Cc: stable@vger.kernel.org # v4.14+
>> ---
>>  drivers/video/fbdev/core/fbcon.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
>> index c4e91715ef00..e162d5e753e5 100644
>> --- a/drivers/video/fbdev/core/fbcon.c
>> +++ b/drivers/video/fbdev/core/fbcon.c
>> @@ -2469,6 +2469,11 @@ static int fbcon_set_font(struct vc_data *vc, struct console_font *font,
>>  	if (charcount != 256 && charcount != 512)
>>  		return -EINVAL;
>>
>> +	/* font bigger than screen resolution ? */
>> +	if (font->width  > FBCON_SWAP(info->var.rotate, info->var.xres, info->var.yres) ||
>> +	    font->height > FBCON_SWAP(info->var.rotate, info->var.yres, info->var.xres))
>> +		return -EINVAL;
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks!

> Maybe as a safety follow up patch, we have a few copies of the below:
>
> 	cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
> 	rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
> 	cols /= vc->vc_font.width;
> 	rows /= vc->vc_font.height;
>
> Might be good to extract that into a helper and also add
>
> 	WARN_ON(!cols);
> 	WARN_ON(!rows);

That's not needed then.
The checks I added above will ensure that cols and rows are both greater than 0.

> to make sure we really didn't screw this up and give syzkaller et all an
> easier time finding bugs - it doesn't need to discover the full exploit,
> only needs to get to this here.
>
> Also maybe even check that cols/rows is within reasons, like smaller than
> BIT(24) or so (so that we have a bunch of headroom for overflows).

Not needed either.
cols and rows is the screen size divided by an value between 1-32 (the max
font size). So, since screen size is already the higest limit, cols&rows
will always be smaller than screen size (and > 0).

Helge

Re: [PATCH v2 1/4] fbcon: Disallow setting font bigger than screen size

[Helge Deller]

On 6/25/22 14:45, Daniel Vetter wrote:
> On Sat, Jun 25, 2022 at 02:24:59PM +0200, Helge Deller wrote:
>> Prevent that users set a font size which is bigger than the physical screen.
>> It's unlikely this may happen (because screens are usually much larger than the
>> fonts and each font char is limited to 32x32 pixels), but it may happen on
>> smaller screens/LCD displays.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> Cc: stable@vger.kernel.org # v4.14+
>> ---
>>  drivers/video/fbdev/core/fbcon.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
>> index c4e91715ef00..e162d5e753e5 100644
>> --- a/drivers/video/fbdev/core/fbcon.c
>> +++ b/drivers/video/fbdev/core/fbcon.c
>> @@ -2469,6 +2469,11 @@ static int fbcon_set_font(struct vc_data *vc, struct console_font *font,
>>  	if (charcount != 256 && charcount != 512)
>>  		return -EINVAL;
>>
>> +	/* font bigger than screen resolution ? */
>> +	if (font->width  > FBCON_SWAP(info->var.rotate, info->var.xres, info->var.yres) ||
>> +	    font->height > FBCON_SWAP(info->var.rotate, info->var.yres, info->var.xres))
>> +		return -EINVAL;
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks!

> Maybe as a safety follow up patch, we have a few copies of the below:
>
> 	cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
> 	rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
> 	cols /= vc->vc_font.width;
> 	rows /= vc->vc_font.height;
>
> Might be good to extract that into a helper and also add
>
> 	WARN_ON(!cols);
> 	WARN_ON(!rows);

That's not needed then.
The checks I added above will ensure that cols and rows are both greater than 0.

> to make sure we really didn't screw this up and give syzkaller et all an
> easier time finding bugs - it doesn't need to discover the full exploit,
> only needs to get to this here.
>
> Also maybe even check that cols/rows is within reasons, like smaller than
> BIT(24) or so (so that we have a bunch of headroom for overflows).

Not needed either.
cols and rows is the screen size divided by an value between 1-32 (the max
font size). So, since screen size is already the higest limit, cols&rows
will always be smaller than screen size (and > 0).

Helge

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

[Helge Deller]

On 6/25/22 14:55, Daniel Vetter wrote:
> 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.

Yes, good idea.

>> +
>> +	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.

Yes, that would be possible later on.
Right now I'd like to limit changes to minimum to make backporting easy.


>> +		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.

Yes, probably right.
Will drop that.

>> +		    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.

Since we now added the WARN_CONSOLE_UNLOCKED() as suggested by you above
I don't think more additional checks are needed.

>
>> +			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.

Fixed in upcoming series now.

Helge

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

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

[Helge Deller]

On 6/25/22 14:55, Daniel Vetter wrote:
> 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.

Yes, good idea.

>> +
>> +	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.

Yes, that would be possible later on.
Right now I'd like to limit changes to minimum to make backporting easy.


>> +		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.

Yes, probably right.
Will drop that.

>> +		    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.

Since we now added the WARN_CONSOLE_UNLOCKED() as suggested by you above
I don't think more additional checks are needed.

>
>> +			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.

Fixed in upcoming series now.

Helge

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

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

[Helge Deller]

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

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

[Helge Deller]

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

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

[Helge Deller]

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
>

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

[Helge Deller]

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
>

Re: [PATCH v2 4/4] fbmem: Catch possible driver bugs regarding too small virtual screen size

[Helge Deller]

On 6/25/22 15:03, Daniel Vetter wrote:
> On Sat, Jun 25, 2022 at 02:25:02PM +0200, Helge Deller wrote:
>> Make sure that we catch, report and fix up fbdev and drm graphic drivers which
>> got the virtual screen resolution smaller than the physical screen resolution.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> Cc: stable@vger.kernel.org # v5.4+
>
> No cc: stable for this, it's not fixing any bugs just helps validate
> driver code.

Ok.

> Also if you just move the check from the ioctl code to here we don't need
> to duplicate anything (and drivers which don't check their set_var are
> kinda busted no matter what).

See previous mail. We need to decide which way we go, then I'll change it.

Thanks!
Helge

> -Daniel
>
>> ---
>>  drivers/video/fbdev/core/fbmem.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
>> index 50fb66b954d6..6d262e341023 100644
>> --- a/drivers/video/fbdev/core/fbmem.c
>> +++ b/drivers/video/fbdev/core/fbmem.c
>> @@ -1006,6 +1006,12 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>>  	if (var->xres < 8 || var->yres < 8)
>>  		return -EINVAL;
>>
>> +	/* make sure virtual resolution >= physical resolution */
>> +	if (WARN_ON(var->xres_virtual < var->xres))
>> +		var->xres_virtual = var->xres;
>> +	if (WARN_ON(var->yres_virtual < var->yres))
>> +		var->yres_virtual = var->yres;
>> +
>>  	/* Too huge resolution causes multiplication overflow. */
>>  	if (check_mul_overflow(var->xres, var->yres, &unused) ||
>>  	    check_mul_overflow(var->xres_virtual, var->yres_virtual, &unused))
>> --
>> 2.35.3
>>
>

Re: [PATCH v2 4/4] fbmem: Catch possible driver bugs regarding too small virtual screen size

[Helge Deller]

On 6/25/22 15:03, Daniel Vetter wrote:
> On Sat, Jun 25, 2022 at 02:25:02PM +0200, Helge Deller wrote:
>> Make sure that we catch, report and fix up fbdev and drm graphic drivers which
>> got the virtual screen resolution smaller than the physical screen resolution.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> Cc: stable@vger.kernel.org # v5.4+
>
> No cc: stable for this, it's not fixing any bugs just helps validate
> driver code.

Ok.

> Also if you just move the check from the ioctl code to here we don't need
> to duplicate anything (and drivers which don't check their set_var are
> kinda busted no matter what).

See previous mail. We need to decide which way we go, then I'll change it.

Thanks!
Helge

> -Daniel
>
>> ---
>>  drivers/video/fbdev/core/fbmem.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
>> index 50fb66b954d6..6d262e341023 100644
>> --- a/drivers/video/fbdev/core/fbmem.c
>> +++ b/drivers/video/fbdev/core/fbmem.c
>> @@ -1006,6 +1006,12 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>>  	if (var->xres < 8 || var->yres < 8)
>>  		return -EINVAL;
>>
>> +	/* make sure virtual resolution >= physical resolution */
>> +	if (WARN_ON(var->xres_virtual < var->xres))
>> +		var->xres_virtual = var->xres;
>> +	if (WARN_ON(var->yres_virtual < var->yres))
>> +		var->yres_virtual = var->yres;
>> +
>>  	/* Too huge resolution causes multiplication overflow. */
>>  	if (check_mul_overflow(var->xres, var->yres, &unused) ||
>>  	    check_mul_overflow(var->xres_virtual, var->yres_virtual, &unused))
>> --
>> 2.35.3
>>
>

Re: [PATCH v2 1/4] fbcon: Disallow setting font bigger than screen size

[Daniel Vetter]

On Sat, Jun 25, 2022 at 04:53:25PM +0200, Helge Deller wrote:
> On 6/25/22 14:45, Daniel Vetter wrote:
> > On Sat, Jun 25, 2022 at 02:24:59PM +0200, Helge Deller wrote:
> >> Prevent that users set a font size which is bigger than the physical screen.
> >> It's unlikely this may happen (because screens are usually much larger than the
> >> fonts and each font char is limited to 32x32 pixels), but it may happen on
> >> smaller screens/LCD displays.
> >>
> >> Signed-off-by: Helge Deller <deller@gmx.de>
> >> Cc: stable@vger.kernel.org # v4.14+
> >> ---
> >>  drivers/video/fbdev/core/fbcon.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> >> index c4e91715ef00..e162d5e753e5 100644
> >> --- a/drivers/video/fbdev/core/fbcon.c
> >> +++ b/drivers/video/fbdev/core/fbcon.c
> >> @@ -2469,6 +2469,11 @@ static int fbcon_set_font(struct vc_data *vc, struct console_font *font,
> >>  	if (charcount != 256 && charcount != 512)
> >>  		return -EINVAL;
> >>
> >> +	/* font bigger than screen resolution ? */
> >> +	if (font->width  > FBCON_SWAP(info->var.rotate, info->var.xres, info->var.yres) ||
> >> +	    font->height > FBCON_SWAP(info->var.rotate, info->var.yres, info->var.xres))
> >> +		return -EINVAL;
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Thanks!
> 
> > Maybe as a safety follow up patch, we have a few copies of the below:
> >
> > 	cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
> > 	rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
> > 	cols /= vc->vc_font.width;
> > 	rows /= vc->vc_font.height;
> >
> > Might be good to extract that into a helper and also add
> >
> > 	WARN_ON(!cols);
> > 	WARN_ON(!rows);
> 
> That's not needed then.
> The checks I added above will ensure that cols and rows are both greater than 0.

Yeah I reviewed it too, but I don't trust review all over the place.
Especially with something like fbcon with entry points from everywhere.
-Daniel

> > to make sure we really didn't screw this up and give syzkaller et all an
> > easier time finding bugs - it doesn't need to discover the full exploit,
> > only needs to get to this here.
> >
> > Also maybe even check that cols/rows is within reasons, like smaller than
> > BIT(24) or so (so that we have a bunch of headroom for overflows).
> 
> Not needed either.
> cols and rows is the screen size divided by an value between 1-32 (the max
> font size). So, since screen size is already the higest limit, cols&rows
> will always be smaller than screen size (and > 0).
> 
> Helge

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

Re: [PATCH v2 1/4] fbcon: Disallow setting font bigger than screen size

[Daniel Vetter]

On Sat, Jun 25, 2022 at 04:53:25PM +0200, Helge Deller wrote:
> On 6/25/22 14:45, Daniel Vetter wrote:
> > On Sat, Jun 25, 2022 at 02:24:59PM +0200, Helge Deller wrote:
> >> Prevent that users set a font size which is bigger than the physical screen.
> >> It's unlikely this may happen (because screens are usually much larger than the
> >> fonts and each font char is limited to 32x32 pixels), but it may happen on
> >> smaller screens/LCD displays.
> >>
> >> Signed-off-by: Helge Deller <deller@gmx.de>
> >> Cc: stable@vger.kernel.org # v4.14+
> >> ---
> >>  drivers/video/fbdev/core/fbcon.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> >> index c4e91715ef00..e162d5e753e5 100644
> >> --- a/drivers/video/fbdev/core/fbcon.c
> >> +++ b/drivers/video/fbdev/core/fbcon.c
> >> @@ -2469,6 +2469,11 @@ static int fbcon_set_font(struct vc_data *vc, struct console_font *font,
> >>  	if (charcount != 256 && charcount != 512)
> >>  		return -EINVAL;
> >>
> >> +	/* font bigger than screen resolution ? */
> >> +	if (font->width  > FBCON_SWAP(info->var.rotate, info->var.xres, info->var.yres) ||
> >> +	    font->height > FBCON_SWAP(info->var.rotate, info->var.yres, info->var.xres))
> >> +		return -EINVAL;
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Thanks!
> 
> > Maybe as a safety follow up patch, we have a few copies of the below:
> >
> > 	cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
> > 	rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
> > 	cols /= vc->vc_font.width;
> > 	rows /= vc->vc_font.height;
> >
> > Might be good to extract that into a helper and also add
> >
> > 	WARN_ON(!cols);
> > 	WARN_ON(!rows);
> 
> That's not needed then.
> The checks I added above will ensure that cols and rows are both greater than 0.

Yeah I reviewed it too, but I don't trust review all over the place.
Especially with something like fbcon with entry points from everywhere.
-Daniel

> > to make sure we really didn't screw this up and give syzkaller et all an
> > easier time finding bugs - it doesn't need to discover the full exploit,
> > only needs to get to this here.
> >
> > Also maybe even check that cols/rows is within reasons, like smaller than
> > BIT(24) or so (so that we have a bunch of headroom for overflows).
> 
> Not needed either.
> cols and rows is the screen size divided by an value between 1-32 (the max
> font size). So, since screen size is already the higest limit, cols&rows
> will always be smaller than screen size (and > 0).
> 
> Helge

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

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

[Daniel Vetter]

On Sat, Jun 25, 2022 at 05:14:15PM +0200, Helge Deller wrote:
> On 6/25/22 14:55, Daniel Vetter wrote:
> > 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.
> 
> Yes, good idea.
> 
> >> +
> >> +	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.
> 
> Yes, that would be possible later on.
> Right now I'd like to limit changes to minimum to make backporting easy.
> 
> 
> >> +		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.
> 
> Yes, probably right.
> Will drop that.
> 
> >> +		    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.
> 
> Since we now added the WARN_CONSOLE_UNLOCKED() as suggested by you above
> I don't think more additional checks are needed.

These are orthogonal checks. The locking check is to make sure we hold the
right locks. The info consistency check here is to make sure we never
managed to make our data structures inconsistent, which should be
impossible after my recent locking changes. Before that lockig change you
need to make sure you are still operating on a valid fb_info struct, hence
this check.

tldr; Entirely separate things.
-Daniel

> 

> >
> >> +			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.
> 
> Fixed in upcoming series now.
> 
> Helge
> 
> > 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

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

[Daniel Vetter]

On Sat, Jun 25, 2022 at 05:14:15PM +0200, Helge Deller wrote:
> On 6/25/22 14:55, Daniel Vetter wrote:
> > 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.
> 
> Yes, good idea.
> 
> >> +
> >> +	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.
> 
> Yes, that would be possible later on.
> Right now I'd like to limit changes to minimum to make backporting easy.
> 
> 
> >> +		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.
> 
> Yes, probably right.
> Will drop that.
> 
> >> +		    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.
> 
> Since we now added the WARN_CONSOLE_UNLOCKED() as suggested by you above
> I don't think more additional checks are needed.

These are orthogonal checks. The locking check is to make sure we hold the
right locks. The info consistency check here is to make sure we never
managed to make our data structures inconsistent, which should be
impossible after my recent locking changes. Before that lockig change you
need to make sure you are still operating on a valid fb_info struct, hence
this check.

tldr; Entirely separate things.
-Daniel

> 

> >
> >> +			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.
> 
> Fixed in upcoming series now.
> 
> Helge
> 
> > 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

Re: [PATCH v2 1/4] fbcon: Disallow setting font bigger than screen size

[Daniel Vetter]

On Sun, Jun 26, 2022 at 12:27:51AM +0200, Daniel Vetter wrote:
> On Sat, Jun 25, 2022 at 04:53:25PM +0200, Helge Deller wrote:
> > On 6/25/22 14:45, Daniel Vetter wrote:
> > > On Sat, Jun 25, 2022 at 02:24:59PM +0200, Helge Deller wrote:
> > >> Prevent that users set a font size which is bigger than the physical screen.
> > >> It's unlikely this may happen (because screens are usually much larger than the
> > >> fonts and each font char is limited to 32x32 pixels), but it may happen on
> > >> smaller screens/LCD displays.
> > >>
> > >> Signed-off-by: Helge Deller <deller@gmx.de>
> > >> Cc: stable@vger.kernel.org # v4.14+
> > >> ---
> > >>  drivers/video/fbdev/core/fbcon.c | 5 +++++
> > >>  1 file changed, 5 insertions(+)
> > >>
> > >> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> > >> index c4e91715ef00..e162d5e753e5 100644
> > >> --- a/drivers/video/fbdev/core/fbcon.c
> > >> +++ b/drivers/video/fbdev/core/fbcon.c
> > >> @@ -2469,6 +2469,11 @@ static int fbcon_set_font(struct vc_data *vc, struct console_font *font,
> > >>  	if (charcount != 256 && charcount != 512)
> > >>  		return -EINVAL;
> > >>
> > >> +	/* font bigger than screen resolution ? */
> > >> +	if (font->width  > FBCON_SWAP(info->var.rotate, info->var.xres, info->var.yres) ||
> > >> +	    font->height > FBCON_SWAP(info->var.rotate, info->var.yres, info->var.xres))
> > >> +		return -EINVAL;
> > >
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > Thanks!
> > 
> > > Maybe as a safety follow up patch, we have a few copies of the below:
> > >
> > > 	cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
> > > 	rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
> > > 	cols /= vc->vc_font.width;
> > > 	rows /= vc->vc_font.height;
> > >
> > > Might be good to extract that into a helper and also add
> > >
> > > 	WARN_ON(!cols);
> > > 	WARN_ON(!rows);
> > 
> > That's not needed then.
> > The checks I added above will ensure that cols and rows are both greater than 0.
> 
> Yeah I reviewed it too, but I don't trust review all over the place.
> Especially with something like fbcon with entry points from everywhere.

Also the other motivation is that a bit of common code extraction refactor
for non-trivial math like the above is simply the right thing to do.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 1/4] fbcon: Disallow setting font bigger than screen size

[Daniel Vetter]

On Sun, Jun 26, 2022 at 12:27:51AM +0200, Daniel Vetter wrote:
> On Sat, Jun 25, 2022 at 04:53:25PM +0200, Helge Deller wrote:
> > On 6/25/22 14:45, Daniel Vetter wrote:
> > > On Sat, Jun 25, 2022 at 02:24:59PM +0200, Helge Deller wrote:
> > >> Prevent that users set a font size which is bigger than the physical screen.
> > >> It's unlikely this may happen (because screens are usually much larger than the
> > >> fonts and each font char is limited to 32x32 pixels), but it may happen on
> > >> smaller screens/LCD displays.
> > >>
> > >> Signed-off-by: Helge Deller <deller@gmx.de>
> > >> Cc: stable@vger.kernel.org # v4.14+
> > >> ---
> > >>  drivers/video/fbdev/core/fbcon.c | 5 +++++
> > >>  1 file changed, 5 insertions(+)
> > >>
> > >> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> > >> index c4e91715ef00..e162d5e753e5 100644
> > >> --- a/drivers/video/fbdev/core/fbcon.c
> > >> +++ b/drivers/video/fbdev/core/fbcon.c
> > >> @@ -2469,6 +2469,11 @@ static int fbcon_set_font(struct vc_data *vc, struct console_font *font,
> > >>  	if (charcount != 256 && charcount != 512)
> > >>  		return -EINVAL;
> > >>
> > >> +	/* font bigger than screen resolution ? */
> > >> +	if (font->width  > FBCON_SWAP(info->var.rotate, info->var.xres, info->var.yres) ||
> > >> +	    font->height > FBCON_SWAP(info->var.rotate, info->var.yres, info->var.xres))
> > >> +		return -EINVAL;
> > >
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > Thanks!
> > 
> > > Maybe as a safety follow up patch, we have a few copies of the below:
> > >
> > > 	cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
> > > 	rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
> > > 	cols /= vc->vc_font.width;
> > > 	rows /= vc->vc_font.height;
> > >
> > > Might be good to extract that into a helper and also add
> > >
> > > 	WARN_ON(!cols);
> > > 	WARN_ON(!rows);
> > 
> > That's not needed then.
> > The checks I added above will ensure that cols and rows are both greater than 0.
> 
> Yeah I reviewed it too, but I don't trust review all over the place.
> Especially with something like fbcon with entry points from everywhere.

Also the other motivation is that a bit of common code extraction refactor
for non-trivial math like the above is simply the right thing to do.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch