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

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

[Helge Deller]

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.

Changes in v2:
- don't fixup wrong xy_vres values, but instead print warning.
- add Reviewed-by tags, minor variable name fixes

Helge Deller (4):
  fbcon: Disallow setting font bigger than screen size
  fbcon: Prevent that screen size is smaller than font size
  fbmem: Prevent invalid virtual screen sizes in fb_set_var()
  fbcon: Use fbcon_info_from_console() in fbcon_modechange_possible()

 drivers/video/fbdev/core/fbcon.c | 33 ++++++++++++++++++++++++++++++++
 drivers/video/fbdev/core/fbmem.c | 16 +++++++++++++++-
 include/linux/fbcon.h            |  4 ++++
 3 files changed, 52 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>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
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..a33532564393 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 (w > FBCON_SWAP(info->var.rotate, info->var.xres, info->var.yres) ||
+	    h > 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: Prevent that screen size is smaller than font size

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

Signed-off-by: Helge Deller <deller@gmx.de>
Cc: stable@vger.kernel.org # v5.4+
---
 drivers/video/fbdev/core/fbcon.c | 28 ++++++++++++++++++++++++++++
 drivers/video/fbdev/core/fbmem.c |  4 +++-
 include/linux/fbcon.h            |  4 ++++
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index a33532564393..14b0ab51744f 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2736,6 +2736,34 @@ 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)
+		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 ||
+			   registered_fb[con2fb_map[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

[PATCH v2 3/4] fbmem: Prevent invalid virtual screen sizes in fb_set_var()

[Helge Deller]

Prevent that drivers configure a virtual screen resolution smaller than
the physical screen resolution.  This is important, because otherwise we
may access memory outside of the graphics memory area.

Give a kernel WARNing and show the driver name to help locating the buggy
driver.

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

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 160389365a36..e8f06d26803c 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1016,6 +1016,18 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
 	if (ret)
 		return ret;

+	/* make sure virtual resolution >= physical resolution */
+	if (WARN_ON(var->xres_virtual < var->xres)) {
+		pr_warn("fbcon: Fix up invalid xres %d for %s\n",
+			var->xres_virtual, info->fix.id);
+		var->xres_virtual = var->xres;
+	}
+	if (WARN_ON(var->yres_virtual < var->yres)) {
+		pr_warn("fbcon: Fix up invalid yres %d for %s\n",
+			var->yres_virtual, info->fix.id);
+		var->yres_virtual = var->yres;
+	}
+
 	if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
 		return 0;

--
2.35.3

[PATCH v2 4/4] fbcon: Use fbcon_info_from_console() in fbcon_modechange_possible()

[Helge Deller]

Use the fbcon_info_from_console() wrapper which was added to kernel
v5.19 with commit 409d6c95f9c6 ("fbcon: Introduce wrapper for console->fb_info lookup").

Signed-off-by: Helge Deller <deller@gmx.de>
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/video/fbdev/core/fbcon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 14b0ab51744f..7f2ebfa8a80a 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2752,7 +2752,7 @@ int fbcon_modechange_possible(struct fb_info *info, struct fb_var_screeninfo *va
 	for (i = first_fb_vc; i <= last_fb_vc; i++) {
 		vc = vc_cons[i].d;
 		if (!vc || vc->vc_mode != KD_TEXT ||
-			   registered_fb[con2fb_map[i]] != info)
+			   fbcon_info_from_console(i) != info)
 			continue;

 		if (vc->vc_font.width  > FBCON_SWAP(var->rotate, var->xres, var->yres) ||
--
2.35.3

Re: [PATCH v2 0/4] fbcon: Fixes for screen resolution changes - round 2

[Helge Deller]

I resent this v2 patch series - this time including patch 4/4.

Re: [PATCH v2 2/4] fbcon: Prevent that screen size is smaller than font size

[Geert Uytterhoeven]

Hi Helge,

On Fri, Jul 1, 2022 at 10:23 PM Helge Deller <deller@gmx.de> 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.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
> Cc: stable@vger.kernel.org # v5.4+

Thanks for your patch!

> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -2736,6 +2736,34 @@ 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;

unsigned int i

> +
> +       WARN_CONSOLE_UNLOCKED();
> +
> +       if (!ops)
> +               return -EINVAL;

This means the frame buffer device is not used as a text console
(i.e. the text console is mapped to a different frame buffer device),
hence it should return success.

> +
> +       /* 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 ||
> +                          registered_fb[con2fb_map[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);

EXPORT_SYMBOL_GPL()?
No idea why most of fbcon uses the non-GPL variant.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/4] fbcon: Prevent that screen size is smaller than font size

[Geert Uytterhoeven]

Hi Helge,

On Fri, Jul 1, 2022 at 10:23 PM Helge Deller <deller@gmx.de> 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.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
> Cc: stable@vger.kernel.org # v5.4+

Thanks for your patch!

> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -2736,6 +2736,34 @@ 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;

unsigned int i

> +
> +       WARN_CONSOLE_UNLOCKED();
> +
> +       if (!ops)
> +               return -EINVAL;

This means the frame buffer device is not used as a text console
(i.e. the text console is mapped to a different frame buffer device),
hence it should return success.

> +
> +       /* 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 ||
> +                          registered_fb[con2fb_map[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);

EXPORT_SYMBOL_GPL()?
No idea why most of fbcon uses the non-GPL variant.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/4] fbmem: Prevent invalid virtual screen sizes in fb_set_var()

[Geert Uytterhoeven]

Hi Helge,

On Fri, Jul 1, 2022 at 10:23 PM Helge Deller <deller@gmx.de> wrote:
> Prevent that drivers configure a virtual screen resolution smaller than
> the physical screen resolution.  This is important, because otherwise we
> may access memory outside of the graphics memory area.
>
> Give a kernel WARNing and show the driver name to help locating the buggy
> driver.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
> Cc: stable@vger.kernel.org # v5.4+

Thanks for your patch!

> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1016,6 +1016,18 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>         if (ret)
>                 return ret;
>
> +       /* make sure virtual resolution >= physical resolution */
> +       if (WARN_ON(var->xres_virtual < var->xres)) {

WARN_ON_ONCE()?
This does mean we would miss two or more buggy drivers in a single system.

> +               pr_warn("fbcon: Fix up invalid xres %d for %s\n",

xres_virtual?

> +                       var->xres_virtual, info->fix.id);
> +               var->xres_virtual = var->xres;

I think it's better to not fix this up, but return -EINVAL instead.
After all if we get here, we have a buggy driver that needs to be fixed.

> +       }
> +       if (WARN_ON(var->yres_virtual < var->yres)) {
> +               pr_warn("fbcon: Fix up invalid yres %d for %s\n",
> +                       var->yres_virtual, info->fix.id);
> +               var->yres_virtual = var->yres;
> +       }

Same for yres.

> +
>         if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
>                 return 0;
>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/4] fbmem: Prevent invalid virtual screen sizes in fb_set_var()

[Geert Uytterhoeven]

Hi Helge,

On Fri, Jul 1, 2022 at 10:23 PM Helge Deller <deller@gmx.de> wrote:
> Prevent that drivers configure a virtual screen resolution smaller than
> the physical screen resolution.  This is important, because otherwise we
> may access memory outside of the graphics memory area.
>
> Give a kernel WARNing and show the driver name to help locating the buggy
> driver.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
> Cc: stable@vger.kernel.org # v5.4+

Thanks for your patch!

> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1016,6 +1016,18 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>         if (ret)
>                 return ret;
>
> +       /* make sure virtual resolution >= physical resolution */
> +       if (WARN_ON(var->xres_virtual < var->xres)) {

WARN_ON_ONCE()?
This does mean we would miss two or more buggy drivers in a single system.

> +               pr_warn("fbcon: Fix up invalid xres %d for %s\n",

xres_virtual?

> +                       var->xres_virtual, info->fix.id);
> +               var->xres_virtual = var->xres;

I think it's better to not fix this up, but return -EINVAL instead.
After all if we get here, we have a buggy driver that needs to be fixed.

> +       }
> +       if (WARN_ON(var->yres_virtual < var->yres)) {
> +               pr_warn("fbcon: Fix up invalid yres %d for %s\n",
> +                       var->yres_virtual, info->fix.id);
> +               var->yres_virtual = var->yres;
> +       }

Same for yres.

> +
>         if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
>                 return 0;
>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/4] fbmem: Prevent invalid virtual screen sizes in fb_set_var()

[Helge Deller]

* Geert Uytterhoeven <geert@linux-m68k.org>:
> > --- a/drivers/video/fbdev/core/fbmem.c
> > +++ b/drivers/video/fbdev/core/fbmem.c
> > @@ -1016,6 +1016,18 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
> >         if (ret)
> >                 return ret;
> >
> > +       /* make sure virtual resolution >= physical resolution */
> > +       if (WARN_ON(var->xres_virtual < var->xres)) {
>
> WARN_ON_ONCE()?
> This does mean we would miss two or more buggy drivers in a single system.
>
> > +               pr_warn("fbcon: Fix up invalid xres %d for %s\n",
>
> xres_virtual?
>
> > +                       var->xres_virtual, info->fix.id);
> > +               var->xres_virtual = var->xres;
>
> I think it's better to not fix this up, but return -EINVAL instead.
> After all if we get here, we have a buggy driver that needs to be fixed.
>
> > +       }
> > +       if (WARN_ON(var->yres_virtual < var->yres)) {
> > +               pr_warn("fbcon: Fix up invalid yres %d for %s\n",
> > +                       var->yres_virtual, info->fix.id);
> > +               var->yres_virtual = var->yres;
> > +       }
>
> Same for yres.

Geert, thanks for your valuable feedback!

In general I don't like for this case any of the WARN_ON* functions.
They don't provide any useful info for us, dumps unneccessarily the
stack backtrace and would annoy existing users.

We know, that DRM drivers can't change the resolution. If we would leave
in any kind of warning, all DRM users will ask back - and we don't have
a solution for them. It's also no regression, because it didn't worked
before either.

But we want to detect fbdev drivers which behave badly - so we should
print that info with the driver name.

Below is a new patch which addresses this. The search for drm drivers
looks somewhat hackish - maybe someone can propose a better approach?

Thoughts?

Helge


From 0f33e2a3730342ab85df372f80b4f61762fb1130 Mon Sep 17 00:00:00 2001
From: Helge Deller <deller@gmx.de>
Date: Wed, 29 Jun 2022 15:53:55 +0200
Subject: [PATCH] fbmem: Check virtual screen sizes in fb_set_var()

Verify that the fbdev or drm driver correctly adjusted the virtual screen
sizes. On failure report (in case of fbdev drivers) the failing driver.

Signed-off-by: Helge Deller <deller@gmx.de>
Cc: stable@vger.kernel.org # v5.4+

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 160389365a36..9b75aa4405ee 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1016,6 +1016,21 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
 	if (ret)
 		return ret;

+	/* verify that virtual resolution >= physical resolution */
+	if (var->xres_virtual < var->xres ||
+	    var->yres_virtual < var->yres) {
+		/* drm drivers don't support mode changes yet. Do not report. */
+		if (strstr(info->fix.id, "drm"))
+			return -ENOTSUPP;
+
+		pr_warn("WARNING: fbcon: Driver %s missed to adjust virtual"
+			" screen size (%dx%d vs. %dx%d)\n",
+			info->fix.id,
+			var->xres_virtual, var->yres_virtual,
+			var->xres, var->yres);
+		return -EINVAL;
+	}
+
 	if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
 		return 0;

Re: [PATCH v2 3/4] fbmem: Prevent invalid virtual screen sizes in fb_set_var()

[Helge Deller]

* Geert Uytterhoeven <geert@linux-m68k.org>:
> > --- a/drivers/video/fbdev/core/fbmem.c
> > +++ b/drivers/video/fbdev/core/fbmem.c
> > @@ -1016,6 +1016,18 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
> >         if (ret)
> >                 return ret;
> >
> > +       /* make sure virtual resolution >= physical resolution */
> > +       if (WARN_ON(var->xres_virtual < var->xres)) {
>
> WARN_ON_ONCE()?
> This does mean we would miss two or more buggy drivers in a single system.
>
> > +               pr_warn("fbcon: Fix up invalid xres %d for %s\n",
>
> xres_virtual?
>
> > +                       var->xres_virtual, info->fix.id);
> > +               var->xres_virtual = var->xres;
>
> I think it's better to not fix this up, but return -EINVAL instead.
> After all if we get here, we have a buggy driver that needs to be fixed.
>
> > +       }
> > +       if (WARN_ON(var->yres_virtual < var->yres)) {
> > +               pr_warn("fbcon: Fix up invalid yres %d for %s\n",
> > +                       var->yres_virtual, info->fix.id);
> > +               var->yres_virtual = var->yres;
> > +       }
>
> Same for yres.

Geert, thanks for your valuable feedback!

In general I don't like for this case any of the WARN_ON* functions.
They don't provide any useful info for us, dumps unneccessarily the
stack backtrace and would annoy existing users.

We know, that DRM drivers can't change the resolution. If we would leave
in any kind of warning, all DRM users will ask back - and we don't have
a solution for them. It's also no regression, because it didn't worked
before either.

But we want to detect fbdev drivers which behave badly - so we should
print that info with the driver name.

Below is a new patch which addresses this. The search for drm drivers
looks somewhat hackish - maybe someone can propose a better approach?

Thoughts?

Helge


From 0f33e2a3730342ab85df372f80b4f61762fb1130 Mon Sep 17 00:00:00 2001
From: Helge Deller <deller@gmx.de>
Date: Wed, 29 Jun 2022 15:53:55 +0200
Subject: [PATCH] fbmem: Check virtual screen sizes in fb_set_var()

Verify that the fbdev or drm driver correctly adjusted the virtual screen
sizes. On failure report (in case of fbdev drivers) the failing driver.

Signed-off-by: Helge Deller <deller@gmx.de>
Cc: stable@vger.kernel.org # v5.4+

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 160389365a36..9b75aa4405ee 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1016,6 +1016,21 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
 	if (ret)
 		return ret;

+	/* verify that virtual resolution >= physical resolution */
+	if (var->xres_virtual < var->xres ||
+	    var->yres_virtual < var->yres) {
+		/* drm drivers don't support mode changes yet. Do not report. */
+		if (strstr(info->fix.id, "drm"))
+			return -ENOTSUPP;
+
+		pr_warn("WARNING: fbcon: Driver %s missed to adjust virtual"
+			" screen size (%dx%d vs. %dx%d)\n",
+			info->fix.id,
+			var->xres_virtual, var->yres_virtual,
+			var->xres, var->yres);
+		return -EINVAL;
+	}
+
 	if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
 		return 0;

Re: [PATCH v2 3/4] fbmem: Prevent invalid virtual screen sizes in fb_set_var()

[Geert Uytterhoeven]

Hi Helge,

On Sun, Jul 3, 2022 at 3:54 PM Helge Deller <deller@gmx.de> wrote:
> * Geert Uytterhoeven <geert@linux-m68k.org>:
> > > --- a/drivers/video/fbdev/core/fbmem.c
> > > +++ b/drivers/video/fbdev/core/fbmem.c
> > > @@ -1016,6 +1016,18 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
> > >         if (ret)
> > >                 return ret;
> > >
> > > +       /* make sure virtual resolution >= physical resolution */
> > > +       if (WARN_ON(var->xres_virtual < var->xres)) {
> >
> > WARN_ON_ONCE()?
> > This does mean we would miss two or more buggy drivers in a single system.
> >
> > > +               pr_warn("fbcon: Fix up invalid xres %d for %s\n",
> >
> > xres_virtual?
> >
> > > +                       var->xres_virtual, info->fix.id);
> > > +               var->xres_virtual = var->xres;
> >
> > I think it's better to not fix this up, but return -EINVAL instead.
> > After all if we get here, we have a buggy driver that needs to be fixed.
> >
> > > +       }
> > > +       if (WARN_ON(var->yres_virtual < var->yres)) {
> > > +               pr_warn("fbcon: Fix up invalid yres %d for %s\n",
> > > +                       var->yres_virtual, info->fix.id);
> > > +               var->yres_virtual = var->yres;
> > > +       }
> >
> > Same for yres.
>
> Geert, thanks for your valuable feedback!
>
> In general I don't like for this case any of the WARN_ON* functions.
> They don't provide any useful info for us, dumps unneccessarily the
> stack backtrace and would annoy existing users.

Without the stack trace, most people won't notice...

> We know, that DRM drivers can't change the resolution. If we would leave
> in any kind of warning, all DRM users will ask back - and we don't have
> a solution for them. It's also no regression, because it didn't worked
> before either.

The warning will only be triggered when the requested virtual
resolution is smaller than the requested physical resolution.  As
long as the requested virtual and physical resolution match what
was returned by FBIOGET_VSCREENINFO before, the warning won't
be triggered.  So in normal use cases, the user won't be impacted.
Fuzzers will see the warning, but the kernel will no longer crash
on invalid values.

> But we want to detect fbdev drivers which behave badly - so we should
> print that info with the driver name.
>
> Below is a new patch which addresses this. The search for drm drivers
> looks somewhat hackish - maybe someone can propose a better approach?
>
> Thoughts?
>
> Helge
>
>
> From 0f33e2a3730342ab85df372f80b4f61762fb1130 Mon Sep 17 00:00:00 2001
> From: Helge Deller <deller@gmx.de>
> Date: Wed, 29 Jun 2022 15:53:55 +0200
> Subject: [PATCH] fbmem: Check virtual screen sizes in fb_set_var()
>
> Verify that the fbdev or drm driver correctly adjusted the virtual screen
> sizes. On failure report (in case of fbdev drivers) the failing driver.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
> Cc: stable@vger.kernel.org # v5.4+
>
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 160389365a36..9b75aa4405ee 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1016,6 +1016,21 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>         if (ret)
>                 return ret;
>
> +       /* verify that virtual resolution >= physical resolution */
> +       if (var->xres_virtual < var->xres ||
> +           var->yres_virtual < var->yres) {
> +               /* drm drivers don't support mode changes yet. Do not report. */
> +               if (strstr(info->fix.id, "drm"))
> +                       return -ENOTSUPP;
> +
> +               pr_warn("WARNING: fbcon: Driver %s missed to adjust virtual"
> +                       " screen size (%dx%d vs. %dx%d)\n",
> +                       info->fix.id,
> +                       var->xres_virtual, var->yres_virtual,
> +                       var->xres, var->yres);
> +               return -EINVAL;
> +       }
> +
>         if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
>                 return 0;

Hence I think there is no need for ignoring drm.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/4] fbmem: Prevent invalid virtual screen sizes in fb_set_var()

[Geert Uytterhoeven]

Hi Helge,

On Sun, Jul 3, 2022 at 3:54 PM Helge Deller <deller@gmx.de> wrote:
> * Geert Uytterhoeven <geert@linux-m68k.org>:
> > > --- a/drivers/video/fbdev/core/fbmem.c
> > > +++ b/drivers/video/fbdev/core/fbmem.c
> > > @@ -1016,6 +1016,18 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
> > >         if (ret)
> > >                 return ret;
> > >
> > > +       /* make sure virtual resolution >= physical resolution */
> > > +       if (WARN_ON(var->xres_virtual < var->xres)) {
> >
> > WARN_ON_ONCE()?
> > This does mean we would miss two or more buggy drivers in a single system.
> >
> > > +               pr_warn("fbcon: Fix up invalid xres %d for %s\n",
> >
> > xres_virtual?
> >
> > > +                       var->xres_virtual, info->fix.id);
> > > +               var->xres_virtual = var->xres;
> >
> > I think it's better to not fix this up, but return -EINVAL instead.
> > After all if we get here, we have a buggy driver that needs to be fixed.
> >
> > > +       }
> > > +       if (WARN_ON(var->yres_virtual < var->yres)) {
> > > +               pr_warn("fbcon: Fix up invalid yres %d for %s\n",
> > > +                       var->yres_virtual, info->fix.id);
> > > +               var->yres_virtual = var->yres;
> > > +       }
> >
> > Same for yres.
>
> Geert, thanks for your valuable feedback!
>
> In general I don't like for this case any of the WARN_ON* functions.
> They don't provide any useful info for us, dumps unneccessarily the
> stack backtrace and would annoy existing users.

Without the stack trace, most people won't notice...

> We know, that DRM drivers can't change the resolution. If we would leave
> in any kind of warning, all DRM users will ask back - and we don't have
> a solution for them. It's also no regression, because it didn't worked
> before either.

The warning will only be triggered when the requested virtual
resolution is smaller than the requested physical resolution.  As
long as the requested virtual and physical resolution match what
was returned by FBIOGET_VSCREENINFO before, the warning won't
be triggered.  So in normal use cases, the user won't be impacted.
Fuzzers will see the warning, but the kernel will no longer crash
on invalid values.

> But we want to detect fbdev drivers which behave badly - so we should
> print that info with the driver name.
>
> Below is a new patch which addresses this. The search for drm drivers
> looks somewhat hackish - maybe someone can propose a better approach?
>
> Thoughts?
>
> Helge
>
>
> From 0f33e2a3730342ab85df372f80b4f61762fb1130 Mon Sep 17 00:00:00 2001
> From: Helge Deller <deller@gmx.de>
> Date: Wed, 29 Jun 2022 15:53:55 +0200
> Subject: [PATCH] fbmem: Check virtual screen sizes in fb_set_var()
>
> Verify that the fbdev or drm driver correctly adjusted the virtual screen
> sizes. On failure report (in case of fbdev drivers) the failing driver.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
> Cc: stable@vger.kernel.org # v5.4+
>
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 160389365a36..9b75aa4405ee 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1016,6 +1016,21 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>         if (ret)
>                 return ret;
>
> +       /* verify that virtual resolution >= physical resolution */
> +       if (var->xres_virtual < var->xres ||
> +           var->yres_virtual < var->yres) {
> +               /* drm drivers don't support mode changes yet. Do not report. */
> +               if (strstr(info->fix.id, "drm"))
> +                       return -ENOTSUPP;
> +
> +               pr_warn("WARNING: fbcon: Driver %s missed to adjust virtual"
> +                       " screen size (%dx%d vs. %dx%d)\n",
> +                       info->fix.id,
> +                       var->xres_virtual, var->yres_virtual,
> +                       var->xres, var->yres);
> +               return -EINVAL;
> +       }
> +
>         if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
>                 return 0;

Hence I think there is no need for ignoring drm.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/4] fbmem: Prevent invalid virtual screen sizes in fb_set_var()

[Helge Deller]

Hi Daniel & DRM developers,

could you please comment on the change below?


On 7/3/22 16:41, Geert Uytterhoeven wrote:
> Hi Helge,
>
> On Sun, Jul 3, 2022 at 3:54 PM Helge Deller <deller@gmx.de> wrote:
>> * Geert Uytterhoeven <geert@linux-m68k.org>:
>>>> --- a/drivers/video/fbdev/core/fbmem.c
>>>> +++ b/drivers/video/fbdev/core/fbmem.c
>>>> @@ -1016,6 +1016,18 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>>>>         if (ret)
>>>>                 return ret;
>>>>
>>>> +       /* make sure virtual resolution >= physical resolution */
>>>> +       if (WARN_ON(var->xres_virtual < var->xres)) {
>>>
>>> WARN_ON_ONCE()?
>>> This does mean we would miss two or more buggy drivers in a single system.
>>>
>>>> +               pr_warn("fbcon: Fix up invalid xres %d for %s\n",
>>>
>>> xres_virtual?
>>>
>>>> +                       var->xres_virtual, info->fix.id);
>>>> +               var->xres_virtual = var->xres;
>>>
>>> I think it's better to not fix this up, but return -EINVAL instead.
>>> After all if we get here, we have a buggy driver that needs to be fixed.
>>>
>>>> +       }
>>>> +       if (WARN_ON(var->yres_virtual < var->yres)) {
>>>> +               pr_warn("fbcon: Fix up invalid yres %d for %s\n",
>>>> +                       var->yres_virtual, info->fix.id);
>>>> +               var->yres_virtual = var->yres;
>>>> +       }
>>>
>>> Same for yres.
>>
>> Geert, thanks for your valuable feedback!
>>
>> In general I don't like for this case any of the WARN_ON* functions.
>> They don't provide any useful info for us, dumps unneccessarily the
>> stack backtrace and would annoy existing users.
>
> Without the stack trace, most people won't notice...
>
>> We know, that DRM drivers can't change the resolution. If we would leave
>> in any kind of warning, all DRM users will ask back - and we don't have
>> a solution for them. It's also no regression, because it didn't worked
>> before either.
>
> The warning will only be triggered when the requested virtual
> resolution is smaller than the requested physical resolution.  As
> long as the requested virtual and physical resolution match what
> was returned by FBIOGET_VSCREENINFO before, the warning won't
> be triggered.  So in normal use cases, the user won't be impacted.
> Fuzzers will see the warning, but the kernel will no longer crash
> on invalid values.

Still, fuzzers will crash if they have panic_on_warn enabled, which
was the case for the reproducer I got.

>> But we want to detect fbdev drivers which behave badly - so we should
>> print that info with the driver name.
>>
>> Below is a new patch which addresses this. The search for drm drivers
>> looks somewhat hackish - maybe someone can propose a better approach?
>>
>> Thoughts?
>>
>> Helge
>>
>>
>> From 0f33e2a3730342ab85df372f80b4f61762fb1130 Mon Sep 17 00:00:00 2001
>> From: Helge Deller <deller@gmx.de>
>> Date: Wed, 29 Jun 2022 15:53:55 +0200
>> Subject: [PATCH] fbmem: Check virtual screen sizes in fb_set_var()
>>
>> Verify that the fbdev or drm driver correctly adjusted the virtual screen
>> sizes. On failure report (in case of fbdev drivers) the failing driver.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> Cc: stable@vger.kernel.org # v5.4+
>>
>> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
>> index 160389365a36..9b75aa4405ee 100644
>> --- a/drivers/video/fbdev/core/fbmem.c
>> +++ b/drivers/video/fbdev/core/fbmem.c
>> @@ -1016,6 +1016,21 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>>         if (ret)
>>                 return ret;
>>
>> +       /* verify that virtual resolution >= physical resolution */
>> +       if (var->xres_virtual < var->xres ||
>> +           var->yres_virtual < var->yres) {

This is the part I'd like to get feedback from DRM on:
Shall we leave that in, or drop it as Geert suggested?

>> +               /* drm drivers don't support mode changes yet. Do not report. */
>> +               if (strstr(info->fix.id, "drm"))
>> +                       return -ENOTSUPP;





>> +
>> +               pr_warn("WARNING: fbcon: Driver %s missed to adjust virtual"
>> +                       " screen size (%dx%d vs. %dx%d)\n",
>> +                       info->fix.id,
>> +                       var->xres_virtual, var->yres_virtual,
>> +                       var->xres, var->yres);
>> +               return -EINVAL;
>> +       }
>> +
>>         if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
>>                 return 0;
>
> Hence I think there is no need for ignoring drm.

Re: [PATCH v2 3/4] fbmem: Prevent invalid virtual screen sizes in fb_set_var()

[Helge Deller]

Hi Daniel & DRM developers,

could you please comment on the change below?


On 7/3/22 16:41, Geert Uytterhoeven wrote:
> Hi Helge,
>
> On Sun, Jul 3, 2022 at 3:54 PM Helge Deller <deller@gmx.de> wrote:
>> * Geert Uytterhoeven <geert@linux-m68k.org>:
>>>> --- a/drivers/video/fbdev/core/fbmem.c
>>>> +++ b/drivers/video/fbdev/core/fbmem.c
>>>> @@ -1016,6 +1016,18 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>>>>         if (ret)
>>>>                 return ret;
>>>>
>>>> +       /* make sure virtual resolution >= physical resolution */
>>>> +       if (WARN_ON(var->xres_virtual < var->xres)) {
>>>
>>> WARN_ON_ONCE()?
>>> This does mean we would miss two or more buggy drivers in a single system.
>>>
>>>> +               pr_warn("fbcon: Fix up invalid xres %d for %s\n",
>>>
>>> xres_virtual?
>>>
>>>> +                       var->xres_virtual, info->fix.id);
>>>> +               var->xres_virtual = var->xres;
>>>
>>> I think it's better to not fix this up, but return -EINVAL instead.
>>> After all if we get here, we have a buggy driver that needs to be fixed.
>>>
>>>> +       }
>>>> +       if (WARN_ON(var->yres_virtual < var->yres)) {
>>>> +               pr_warn("fbcon: Fix up invalid yres %d for %s\n",
>>>> +                       var->yres_virtual, info->fix.id);
>>>> +               var->yres_virtual = var->yres;
>>>> +       }
>>>
>>> Same for yres.
>>
>> Geert, thanks for your valuable feedback!
>>
>> In general I don't like for this case any of the WARN_ON* functions.
>> They don't provide any useful info for us, dumps unneccessarily the
>> stack backtrace and would annoy existing users.
>
> Without the stack trace, most people won't notice...
>
>> We know, that DRM drivers can't change the resolution. If we would leave
>> in any kind of warning, all DRM users will ask back - and we don't have
>> a solution for them. It's also no regression, because it didn't worked
>> before either.
>
> The warning will only be triggered when the requested virtual
> resolution is smaller than the requested physical resolution.  As
> long as the requested virtual and physical resolution match what
> was returned by FBIOGET_VSCREENINFO before, the warning won't
> be triggered.  So in normal use cases, the user won't be impacted.
> Fuzzers will see the warning, but the kernel will no longer crash
> on invalid values.

Still, fuzzers will crash if they have panic_on_warn enabled, which
was the case for the reproducer I got.

>> But we want to detect fbdev drivers which behave badly - so we should
>> print that info with the driver name.
>>
>> Below is a new patch which addresses this. The search for drm drivers
>> looks somewhat hackish - maybe someone can propose a better approach?
>>
>> Thoughts?
>>
>> Helge
>>
>>
>> From 0f33e2a3730342ab85df372f80b4f61762fb1130 Mon Sep 17 00:00:00 2001
>> From: Helge Deller <deller@gmx.de>
>> Date: Wed, 29 Jun 2022 15:53:55 +0200
>> Subject: [PATCH] fbmem: Check virtual screen sizes in fb_set_var()
>>
>> Verify that the fbdev or drm driver correctly adjusted the virtual screen
>> sizes. On failure report (in case of fbdev drivers) the failing driver.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> Cc: stable@vger.kernel.org # v5.4+
>>
>> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
>> index 160389365a36..9b75aa4405ee 100644
>> --- a/drivers/video/fbdev/core/fbmem.c
>> +++ b/drivers/video/fbdev/core/fbmem.c
>> @@ -1016,6 +1016,21 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>>         if (ret)
>>                 return ret;
>>
>> +       /* verify that virtual resolution >= physical resolution */
>> +       if (var->xres_virtual < var->xres ||
>> +           var->yres_virtual < var->yres) {

This is the part I'd like to get feedback from DRM on:
Shall we leave that in, or drop it as Geert suggested?

>> +               /* drm drivers don't support mode changes yet. Do not report. */
>> +               if (strstr(info->fix.id, "drm"))
>> +                       return -ENOTSUPP;





>> +
>> +               pr_warn("WARNING: fbcon: Driver %s missed to adjust virtual"
>> +                       " screen size (%dx%d vs. %dx%d)\n",
>> +                       info->fix.id,
>> +                       var->xres_virtual, var->yres_virtual,
>> +                       var->xres, var->yres);
>> +               return -EINVAL;
>> +       }
>> +
>>         if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
>>                 return 0;
>
> Hence I think there is no need for ignoring drm.

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

[Helge Deller]

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.

Changes in v2:
- don't fixup wrong xy_vres values, but instead print warning.
- add Reviewed-by tags, minor variable name fixes

Helge Deller (4):
  fbcon: Disallow setting font bigger than screen size
  fbcon: Prevent that screen size is smaller than font size
  fbmem: Prevent invalid virtual screen sizes in fb_set_var()
  fbcon: Use fbcon_info_from_console() in fbcon_modechange_possible()

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

--
2.35.3