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

[PATCH 0/5] 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.

Helge Deller (5):
  fbcon: Disallow setting font bigger than screen size
  fbcon: Fix up user-provided virtual 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 | 15 ++++++++++++++-
 include/linux/fbcon.h            |  4 ++++
 3 files changed, 51 insertions(+), 1 deletion(-)

--
2.35.3

[PATCH 1/5] 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>
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 2/5] fbcon: Fix up user-provided virtual screen size

[Helge Deller]

The virtual screen size can't be smaller than the physical screen size.
Based on the general rule that we round up user-provided input if
neccessary, adjust the virtual screen size as well if needed.

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

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index afa2863670f3..e645b96d2700 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1106,6 +1106,11 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
 			return -EFAULT;
 		console_lock();
 		lock_fb_info(info);
+		/* adjust virtual screen size if user missed it */
+		if (var.xres_virtual < var.xres)
+			var.xres_virtual = var.xres;
+		if (var.yres_virtual < var.yres)
+			var.yres_virtual = var.yres;
 		ret = fb_set_var(info, &var);
 		if (!ret)
 			fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);
--
2.35.3

[PATCH 3/5] 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 e162d5e753e5..278c065722b7 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 || 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 ||
+			   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 e645b96d2700..324f726739c4 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1111,7 +1111,9 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
 			var.xres_virtual = var.xres;
 		if (var.yres_virtual < var.yres)
 			var.yres_virtual = var.yres;
-		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 4/5] 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.

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 324f726739c4..222d94e2e0a2 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

[PATCH 5/5] 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>
---
 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 278c065722b7..ec1cfc6c2451 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 1/5] fbcon: Disallow setting font bigger than screen size

[Geert Uytterhoeven]

Hi Helge,

On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@gmx.de> 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>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

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

Note that we already have local vars w and h, albeit with the wrong
signedness.

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

There were already more opportunities for using w and h before...

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 2/5] fbcon: Fix up user-provided virtual screen size

[Geert Uytterhoeven]

Hi Helge,

On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@gmx.de> wrote:
> The virtual screen size can't be smaller than the physical screen size.
> Based on the general rule that we round up user-provided input if
> neccessary, adjust the virtual screen size as well if needed.
>
> 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
> @@ -1106,6 +1106,11 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>                         return -EFAULT;
>                 console_lock();
>                 lock_fb_info(info);
> +               /* adjust virtual screen size if user missed it */
> +               if (var.xres_virtual < var.xres)
> +                       var.xres_virtual = var.xres;
> +               if (var.yres_virtual < var.yres)
> +                       var.yres_virtual = var.yres;
>                 ret = fb_set_var(info, &var);
>                 if (!ret)
>                         fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);

Given "[PATCH 4/5] fbmem: Prevent invalid virtual screen sizes in
fb_set_var", I don't think we need this patch.  Moreover, this
patch will prevent triggering the WARN_ON()s in [PATCH 4/5] in the
most common buggy case of drivers plainly ignoring var.[xy]res_virtual.

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 3/5] fbcon: Prevent that screen size is smaller than font size

[Geert Uytterhoeven]

Hi Helge,

On Wed, Jun 29, 2022 at 10:00 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;
> +
> +       WARN_CONSOLE_UNLOCKED();
> +
> +       if (!ops || ops->currcon < 0)
> +               return -EINVAL;

So if the virtual console is _not_ used as a text console, we refuse
mode changes?

> +
> +       /* 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;
> +       }

IMHO this looks way too fragile, and we should fix the rendering code
to handle cols == 0 || rows == 0 instead...

> +
> +       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 e645b96d2700..324f726739c4 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1111,7 +1111,9 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>                         var.xres_virtual = var.xres;
>                 if (var.yres_virtual < var.yres)
>                         var.yres_virtual = var.yres;
> -               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);

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 4/5] fbmem: Prevent invalid virtual screen sizes in fb_set_var()

[Geert Uytterhoeven]

Hi Helge,

On Wed, Jun 29, 2022 at 10:00 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.
>
> 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
> @@ -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;

This should be moved below the call to info->fbops->fb_check_var(),
so the WARN_ON() catches buggy fbdev drivers, not userspace fuzzers.

> +
>         /* 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))

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 4/5] fbmem: Prevent invalid virtual screen sizes in fb_set_var()

[Helge Deller]

On 6/30/22 21:11, Geert Uytterhoeven wrote:
> Hi Helge,
>
> On Wed, Jun 29, 2022 at 10:00 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.
>>
>> 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
>> @@ -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;
>
> This should be moved below the call to info->fbops->fb_check_var(),
> so the WARN_ON() catches buggy fbdev drivers, not userspace fuzzers.

Yes, makes sense.

THX,
Helge


>> +
>>         /* 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))
>
> 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 3/5] fbcon: Prevent that screen size is smaller than font size

[Geert Uytterhoeven]

Hi Helge,

On Thu, Jun 30, 2022 at 9:09 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Wed, Jun 29, 2022 at 10:00 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;
> > +
> > +       WARN_CONSOLE_UNLOCKED();
> > +
> > +       if (!ops || ops->currcon < 0)
> > +               return -EINVAL;
>
> So if the virtual console is _not_ used as a text console, we refuse
> mode changes?
>
> > +
> > +       /* 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)

Looks like registered_fb[] is wrong since commit cad564ca557f8d3b
("fbcon: Fix boundary checks for fbcon=vc:n1-n2 parameters")?

> > +                       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;
> > +       }

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 5/5] fbcon: Use fbcon_info_from_console() in fbcon_modechange_possible()

[Geert Uytterhoeven]

Hi Helge,

On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@gmx.de> wrote:
> 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>

Thanks for your patch!

I guess you kept this separate, to ease backporting?
This does mean that upstream will never really see if using
registered_fb[] directly may cause problems...

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

Anyway, LGTM, so
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

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 1/5] fbcon: Disallow setting font bigger than screen size

[Helge Deller]

On 6/30/22 20:51, Geert Uytterhoeven wrote:
> Hi Helge,
>
> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@gmx.de> 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>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Thanks for your patch!
>
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
>> --- 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))
>
> Note that we already have local vars w and h, albeit with the wrong
> signedness.

I don't like the "h" and "w" variables. Maybe something like "fh" for "font-heigth"
would have been better explaining which kind of "h" is meant.
I assume that's why the patch below didn't used it either.

That said, I'd like to keep it as is (at least for now).

Helge


>> +               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))))
>
> There were already more opportunities for using w and h before...
>
> 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 2/5] fbcon: Fix up user-provided virtual screen size

[Helge Deller]

On 6/30/22 21:00, Geert Uytterhoeven wrote:
> Hi Helge,
>
> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@gmx.de> wrote:
>> The virtual screen size can't be smaller than the physical screen size.
>> Based on the general rule that we round up user-provided input if
>> neccessary, adjust the virtual screen size as well if needed.
>>
>> 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
>> @@ -1106,6 +1106,11 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>>                         return -EFAULT;
>>                 console_lock();
>>                 lock_fb_info(info);
>> +               /* adjust virtual screen size if user missed it */
>> +               if (var.xres_virtual < var.xres)
>> +                       var.xres_virtual = var.xres;
>> +               if (var.yres_virtual < var.yres)
>> +                       var.yres_virtual = var.yres;
>>                 ret = fb_set_var(info, &var);
>>                 if (!ret)
>>                         fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);
>
> Given "[PATCH 4/5] fbmem: Prevent invalid virtual screen sizes in
> fb_set_var", I don't think we need this patch.

We do.

> Moreover, this
> patch will prevent triggering the WARN_ON()s in [PATCH 4/5]

Right.

> in the most common buggy case of drivers plainly ignoring var.[xy]res_virtual.

In summary:
This patch #2 is fixing up user-space invalid input and is not
allowed to trigger any WARN_ON().

We could drop patch #4, but then we wouldn't catch bad drivers.

Helge

Re: [PATCH 2/5] fbcon: Fix up user-provided virtual screen size

[Geert Uytterhoeven]

Hi Helge,

On Thu, Jun 30, 2022 at 9:31 PM Helge Deller <deller@gmx.de> wrote:
> On 6/30/22 21:00, Geert Uytterhoeven wrote:
> > On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@gmx.de> wrote:
> >> The virtual screen size can't be smaller than the physical screen size.
> >> Based on the general rule that we round up user-provided input if
> >> neccessary, adjust the virtual screen size as well if needed.
> >>
> >> 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
> >> @@ -1106,6 +1106,11 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
> >>                         return -EFAULT;
> >>                 console_lock();
> >>                 lock_fb_info(info);
> >> +               /* adjust virtual screen size if user missed it */
> >> +               if (var.xres_virtual < var.xres)
> >> +                       var.xres_virtual = var.xres;
> >> +               if (var.yres_virtual < var.yres)
> >> +                       var.yres_virtual = var.yres;
> >>                 ret = fb_set_var(info, &var);
> >>                 if (!ret)
> >>                         fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);
> >
> > Given "[PATCH 4/5] fbmem: Prevent invalid virtual screen sizes in
> > fb_set_var", I don't think we need this patch.
>
> We do.

Why? It will be caught by [PATCH 4/5].

> > Moreover, this
> > patch will prevent triggering the WARN_ON()s in [PATCH 4/5]
>
> Right.
>
> > in the most common buggy case of drivers plainly ignoring var.[xy]res_virtual.
>
> In summary:
> This patch #2 is fixing up user-space invalid input and is not
> allowed to trigger any WARN_ON().

It's the responsibility of the driver to at least look at its parameters.
What other invalid values might it let pass, that we cannot catch
at the generic level?

> We could drop patch #4, but then we wouldn't catch bad drivers.

I do want to keep patch #4.

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 4/5] fbmem: Prevent invalid virtual screen sizes in fb_set_var()

[Geert Uytterhoeven]

Hi Helge,

On Thu, Jun 30, 2022 at 9:17 PM Helge Deller <deller@gmx.de> wrote:
> On 6/30/22 21:11, Geert Uytterhoeven wrote:
> > On Wed, Jun 29, 2022 at 10:00 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.
> >>
> >> 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
> >> @@ -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;
> >
> > This should be moved below the call to info->fbops->fb_check_var(),
> > so the WARN_ON() catches buggy fbdev drivers, not userspace fuzzers.
>
> Yes, makes sense.

And print the name of the frame buffer device driver, so people know
who to blame.

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 3/5] fbcon: Prevent that screen size is smaller than font size

[Helge Deller]

On 6/30/22 21:09, Geert Uytterhoeven wrote:
> Hi Helge,
>
> On Wed, Jun 29, 2022 at 10:00 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;
>> +
>> +       WARN_CONSOLE_UNLOCKED();
>> +
>> +       if (!ops || ops->currcon < 0)
>> +               return -EINVAL;
>
> So if the virtual console is _not_ used as a text console, we refuse
> mode changes?

Right, this needs to be dropped.

Helge


>> +
>> +       /* 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;
>> +       }
>
> IMHO this looks way too fragile, and we should fix the rendering code
> to handle cols == 0 || rows == 0 instead...

Long-term we might need both.
The code above will at least return EINVAL if users try it, while
the cols/rows=0 code just prevents rendering if it doesn't fit.

Helge

>
>> +
>> +       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 e645b96d2700..324f726739c4 100644
>> --- a/drivers/video/fbdev/core/fbmem.c
>> +++ b/drivers/video/fbdev/core/fbmem.c
>> @@ -1111,7 +1111,9 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>>                         var.xres_virtual = var.xres;
>>                 if (var.yres_virtual < var.yres)
>>                         var.yres_virtual = var.yres;
>> -               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);
>
> 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 2/5] fbcon: Fix up user-provided virtual screen size

[Helge Deller]

On 6/30/22 21:36, Geert Uytterhoeven wrote:
> Hi Helge,
>
> On Thu, Jun 30, 2022 at 9:31 PM Helge Deller <deller@gmx.de> wrote:
>> On 6/30/22 21:00, Geert Uytterhoeven wrote:
>>> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@gmx.de> wrote:
>>>> The virtual screen size can't be smaller than the physical screen size.
>>>> Based on the general rule that we round up user-provided input if
>>>> neccessary, adjust the virtual screen size as well if needed.
>>>>
>>>> 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
>>>> @@ -1106,6 +1106,11 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>>>>                         return -EFAULT;
>>>>                 console_lock();
>>>>                 lock_fb_info(info);
>>>> +               /* adjust virtual screen size if user missed it */
>>>> +               if (var.xres_virtual < var.xres)
>>>> +                       var.xres_virtual = var.xres;
>>>> +               if (var.yres_virtual < var.yres)
>>>> +                       var.yres_virtual = var.yres;
>>>>                 ret = fb_set_var(info, &var);
>>>>                 if (!ret)
>>>>                         fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);
>>>
>>> Given "[PATCH 4/5] fbmem: Prevent invalid virtual screen sizes in
>>> fb_set_var", I don't think we need this patch.
>>
>> We do.
>
> Why? It will be caught by [PATCH 4/5].

Right, it will be caught by patch #4.
But if you drop this part, then everytime a user runs
	fbset -xres 800 -yres 600 -xvres 200
users will get the KERNEL BUG WARNING (from patch #4) including
a kernel backtrace in their syslogs.
This is not what you want.

If you drop the WARN() from patch #4 you'll end up with my
last patch series.



>>> Moreover, this
>>> patch will prevent triggering the WARN_ON()s in [PATCH 4/5]
>>
>> Right.
>>
>>> in the most common buggy case of drivers plainly ignoring var.[xy]res_virtual.
>>
>> In summary:
>> This patch #2 is fixing up user-space invalid input and is not
>> allowed to trigger any WARN_ON().
>
> It's the responsibility of the driver to at least look at its parameters.
> What other invalid values might it let pass, that we cannot catch
> at the generic level?
>
>> We could drop patch #4, but then we wouldn't catch bad drivers.
>
> I do want to keep patch #4.
>
> 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 2/5] fbcon: Fix up user-provided virtual screen size

[Geert Uytterhoeven]

Hi Helge,

On Thu, Jun 30, 2022 at 9:46 PM Helge Deller <deller@gmx.de> wrote:
> On 6/30/22 21:36, Geert Uytterhoeven wrote:
> > On Thu, Jun 30, 2022 at 9:31 PM Helge Deller <deller@gmx.de> wrote:
> >> On 6/30/22 21:00, Geert Uytterhoeven wrote:
> >>> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@gmx.de> wrote:
> >>>> The virtual screen size can't be smaller than the physical screen size.
> >>>> Based on the general rule that we round up user-provided input if
> >>>> neccessary, adjust the virtual screen size as well if needed.
> >>>>
> >>>> 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
> >>>> @@ -1106,6 +1106,11 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
> >>>>                         return -EFAULT;
> >>>>                 console_lock();
> >>>>                 lock_fb_info(info);
> >>>> +               /* adjust virtual screen size if user missed it */
> >>>> +               if (var.xres_virtual < var.xres)
> >>>> +                       var.xres_virtual = var.xres;
> >>>> +               if (var.yres_virtual < var.yres)
> >>>> +                       var.yres_virtual = var.yres;
> >>>>                 ret = fb_set_var(info, &var);
> >>>>                 if (!ret)
> >>>>                         fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);
> >>>
> >>> Given "[PATCH 4/5] fbmem: Prevent invalid virtual screen sizes in
> >>> fb_set_var", I don't think we need this patch.
> >>
> >> We do.
> >
> > Why? It will be caught by [PATCH 4/5].
>
> Right, it will be caught by patch #4.
> But if you drop this part, then everytime a user runs
>         fbset -xres 800 -yres 600 -xvres 200
> users will get the KERNEL BUG WARNING (from patch #4) including
> a kernel backtrace in their syslogs.

No, they will only see that warning if they are using a broken fbdev
driver that implements .fb_check_var(), but fails to validate or
update the passed geometry.

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 2/5] fbcon: Fix up user-provided virtual screen size

[Helge Deller]

On 6/30/22 22:00, Geert Uytterhoeven wrote:
> Hi Helge,
>
> On Thu, Jun 30, 2022 at 9:46 PM Helge Deller <deller@gmx.de> wrote:
>> On 6/30/22 21:36, Geert Uytterhoeven wrote:
>>> On Thu, Jun 30, 2022 at 9:31 PM Helge Deller <deller@gmx.de> wrote:
>>>> On 6/30/22 21:00, Geert Uytterhoeven wrote:
>>>>> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@gmx.de> wrote:
>>>>>> The virtual screen size can't be smaller than the physical screen size.
>>>>>> Based on the general rule that we round up user-provided input if
>>>>>> neccessary, adjust the virtual screen size as well if needed.
>>>>>>
>>>>>> 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
>>>>>> @@ -1106,6 +1106,11 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>>>>>>                         return -EFAULT;
>>>>>>                 console_lock();
>>>>>>                 lock_fb_info(info);
>>>>>> +               /* adjust virtual screen size if user missed it */
>>>>>> +               if (var.xres_virtual < var.xres)
>>>>>> +                       var.xres_virtual = var.xres;
>>>>>> +               if (var.yres_virtual < var.yres)
>>>>>> +                       var.yres_virtual = var.yres;
>>>>>>                 ret = fb_set_var(info, &var);
>>>>>>                 if (!ret)
>>>>>>                         fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);
>>>>>
>>>>> Given "[PATCH 4/5] fbmem: Prevent invalid virtual screen sizes in
>>>>> fb_set_var", I don't think we need this patch.
>>>>
>>>> We do.
>>>
>>> Why? It will be caught by [PATCH 4/5].
>>
>> Right, it will be caught by patch #4.
>> But if you drop this part, then everytime a user runs
>>         fbset -xres 800 -yres 600 -xvres 200
>> users will get the KERNEL BUG WARNING (from patch #4) including
>> a kernel backtrace in their syslogs.
>
> No, they will only see that warning if they are using a broken fbdev
> driver that implements .fb_check_var(), but fails to validate or
> update the passed geometry.

IMHO this argument is mood.
That way you put pressure on and need such simple code in
each single driver to fix it up, instead of cleaning it up at a central
place.

Helge

Re: [PATCH 2/5] fbcon: Fix up user-provided virtual screen size

[Geert Uytterhoeven]

Hi Helge,

On Thu, Jun 30, 2022 at 10:10 PM Helge Deller <deller@gmx.de> wrote:
> On 6/30/22 22:00, Geert Uytterhoeven wrote:
> > On Thu, Jun 30, 2022 at 9:46 PM Helge Deller <deller@gmx.de> wrote:
> >> On 6/30/22 21:36, Geert Uytterhoeven wrote:
> >>> On Thu, Jun 30, 2022 at 9:31 PM Helge Deller <deller@gmx.de> wrote:
> >>>> On 6/30/22 21:00, Geert Uytterhoeven wrote:
> >>>>> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@gmx.de> wrote:
> >>>>>> The virtual screen size can't be smaller than the physical screen size.
> >>>>>> Based on the general rule that we round up user-provided input if
> >>>>>> neccessary, adjust the virtual screen size as well if needed.
> >>>>>>
> >>>>>> 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
> >>>>>> @@ -1106,6 +1106,11 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
> >>>>>>                         return -EFAULT;
> >>>>>>                 console_lock();
> >>>>>>                 lock_fb_info(info);
> >>>>>> +               /* adjust virtual screen size if user missed it */
> >>>>>> +               if (var.xres_virtual < var.xres)
> >>>>>> +                       var.xres_virtual = var.xres;
> >>>>>> +               if (var.yres_virtual < var.yres)
> >>>>>> +                       var.yres_virtual = var.yres;
> >>>>>>                 ret = fb_set_var(info, &var);
> >>>>>>                 if (!ret)
> >>>>>>                         fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);
> >>>>>
> >>>>> Given "[PATCH 4/5] fbmem: Prevent invalid virtual screen sizes in
> >>>>> fb_set_var", I don't think we need this patch.
> >>>>
> >>>> We do.
> >>>
> >>> Why? It will be caught by [PATCH 4/5].
> >>
> >> Right, it will be caught by patch #4.
> >> But if you drop this part, then everytime a user runs
> >>         fbset -xres 800 -yres 600 -xvres 200
> >> users will get the KERNEL BUG WARNING (from patch #4) including
> >> a kernel backtrace in their syslogs.
> >
> > No, they will only see that warning if they are using a broken fbdev
> > driver that implements .fb_check_var(), but fails to validate or
> > update the passed geometry.
>
> IMHO this argument is mood.
> That way you put pressure on and need such simple code in
> each single driver to fix it up, instead of cleaning it up at a central
> place.

Most hardware has restrictions on resolution (e.g. xres must be a
multiple of N), so the driver has to round up the resolution to make
it fit.  And after that the driver has to validate and update the
virtual resolution again anyway...

If a driver does not support changing the video mode, it can leave
out the .fb_check_var() and .fb_set_par() callbacks, so the fbdev
core will ignore the userspace-supplied parameters, and reinstate
the single supported mode. See e.g. "[PATCH] drm/fb-helper:
Remove helpers to change frame buffer config"
(https://lore.kernel.org/all/20220629105658.1373770-1-geert@linux-m68k.org).

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 2/5] fbcon: Fix up user-provided virtual screen size

[Helge Deller]

Hi Geert,

On 7/1/22 09:28, Geert Uytterhoeven wrote:
> On Thu, Jun 30, 2022 at 10:10 PM Helge Deller <deller@gmx.de> wrote:
>> On 6/30/22 22:00, Geert Uytterhoeven wrote:
>>> On Thu, Jun 30, 2022 at 9:46 PM Helge Deller <deller@gmx.de> wrote:
>>>> On 6/30/22 21:36, Geert Uytterhoeven wrote:
>>>>> On Thu, Jun 30, 2022 at 9:31 PM Helge Deller <deller@gmx.de> wrote:
>>>>>> On 6/30/22 21:00, Geert Uytterhoeven wrote:
>>>>>>> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@gmx.de> wrote:
>>>>>>>> The virtual screen size can't be smaller than the physical screen size.
>>>>>>>> Based on the general rule that we round up user-provided input if
>>>>>>>> neccessary, adjust the virtual screen size as well if needed.
>>>>>>>>
>>>>>>>> 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
>>>>>>>> @@ -1106,6 +1106,11 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>>>>>>>>                         return -EFAULT;
>>>>>>>>                 console_lock();
>>>>>>>>                 lock_fb_info(info);
>>>>>>>> +               /* adjust virtual screen size if user missed it */
>>>>>>>> +               if (var.xres_virtual < var.xres)
>>>>>>>> +                       var.xres_virtual = var.xres;
>>>>>>>> +               if (var.yres_virtual < var.yres)
>>>>>>>> +                       var.yres_virtual = var.yres;
>>>>>>>>                 ret = fb_set_var(info, &var);
>>>>>>>>                 if (!ret)
>>>>>>>>                         fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);
>>>>>>>
>>>>>>> Given "[PATCH 4/5] fbmem: Prevent invalid virtual screen sizes in
>>>>>>> fb_set_var", I don't think we need this patch.
>>>>>>
>>>>>> We do.
>>>>>
>>>>> Why? It will be caught by [PATCH 4/5].
>>>>
>>>> Right, it will be caught by patch #4.
>>>> But if you drop this part, then everytime a user runs
>>>>         fbset -xres 800 -yres 600 -xvres 200
>>>> users will get the KERNEL BUG WARNING (from patch #4) including
>>>> a kernel backtrace in their syslogs.
>>>
>>> No, they will only see that warning if they are using a broken fbdev
>>> driver that implements .fb_check_var(), but fails to validate or
>>> update the passed geometry.
>>
>> IMHO this argument is mood.
>> That way you put pressure on and need such simple code in
>> each single driver to fix it up, instead of cleaning it up at a central
>> place.
>
> Most hardware has restrictions on resolution (e.g. xres must be a
> multiple of N), so the driver has to round up the resolution to make
> it fit.  And after that the driver has to validate and update the
> virtual resolution again anyway...
>
> If a driver does not support changing the video mode, it can leave
> out the .fb_check_var() and .fb_set_par() callbacks, so the fbdev
> core will ignore the userspace-supplied parameters, and reinstate
> the single supported mode. See e.g. "[PATCH] drm/fb-helper:
> Remove helpers to change frame buffer config"
> (https://lore.kernel.org/all/20220629105658.1373770-1-geert@linux-m68k.org).

I implemented all of your suggested changes - from this mail and the others.
I've committed a new testing tree to the fbcon-fix-testing branch at:
https://github.com/hdeller/linux/tree/fbcon-fix-testing
The diff is here:
https://github.com/torvalds/linux/compare/master...hdeller:linux:fbcon-fix-testing

Although your idea is good since we now would find issues in the drivers,
I don't think we want to commit it, since the testcase from
the bug report then immediately crashes the kernel - see below.

I think we need to fix up earlier.
Your other patch to disable DRM's set_fb_var() might fix this specific issue,
but in general we may face other problems in other drivers too.
Thoughts?

Helge



root@debian:~# ./a.out
[   44.118212][ T3081] ------------[ cut here ]------------
[   44.118298][ T3081] WARNING: CPU: 2 PID: 3081 at drivers/video/fbdev/core/fbmem.c:1020 fb_set_var.cold+0x10d/0x1bc
[   44.118376][ T3081] Modules linked in:
[   44.118401][ T3081] CPU: 2 PID: 3081 Comm: a.out Not tainted 5.19.0-rc4-00004-g11dd75029515 #17
[   44.118432][ T3081] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1.fc35 04/01/2014
[   44.118453][ T3081] RIP: 0010:fb_set_var.cold+0x10d/0x1bc
[   44.118709][ T3081] Call Trace:
[   44.118719][ T3081]  <TASK>
[   44.118731][ T3081]  ? fb_blank+0x190/0x190
[   44.118784][ T3081]  ? rcu_read_lock_sched_held+0x3a/0x70
[   44.118816][ T3081]  ? trace_contention_end+0xea/0x150
[   44.118845][ T3081]  ? __mutex_lock+0x259/0x1450
[   44.118875][ T3081]  ? do_fb_ioctl+0x2fd/0x6f0
[   44.118906][ T3081]  ? mutex_lock_io_nested+0x1260/0x1260
[   44.118936][ T3081]  ? lock_downgrade+0x6e0/0x6e0
[   44.118966][ T3081]  ? rwlock_bug.part.0+0x90/0x90
[   44.118998][ T3081]  ? _raw_spin_lock_irqsave+0x4e/0x50
[   44.119031][ T3081]  ? is_console_locked+0x5/0x10
[   44.119060][ T3081]  ? fbcon_info_from_console+0x61/0x190
[   44.119087][ T3081]  ? fbcon_modechange_possible+0x359/0x4c0
[   44.119116][ T3081]  do_fb_ioctl+0x63b/0x6f0
[   44.119146][ T3081]  ? putname+0xfe/0x140
[   44.119174][ T3081]  ? fb_set_suspend+0x1a0/0x1a0
[   44.119204][ T3081]  ? path_openat+0x1016/0x2810
[   44.119234][ T3081]  ? mark_lock.part.0+0xfc/0x1a00
[   44.119266][ T3081]  ? lock_chain_count+0x20/0x20
[   44.119297][ T3081]  ? lock_chain_count+0x20/0x20
[   44.119326][ T3081]  ? lock_downgrade+0x6e0/0x6e0
[   44.119358][ T3081]  ? _raw_spin_unlock_irqrestore+0x50/0x70
[   44.119396][ T3081]  fb_ioctl+0xe7/0x150
[   44.119424][ T3081]  ? do_fb_ioctl+0x6f0/0x6f0
[   44.119454][ T3081]  __x64_sys_ioctl+0x94c/0x18a0
[   44.119487][ T3081]  ? vfs_fileattr_set+0xb70/0xb70
[   44.119520][ T3081]  ? find_held_lock+0x2d/0x110
[   44.119550][ T3081]  ? __context_tracking_exit+0xb8/0xe0
[   44.119582][ T3081]  ? lock_downgrade+0x6e0/0x6e0
[   44.119613][ T3081]  ? lock_downgrade+0x6e0/0x6e0
[   44.119645][ T3081]  ? syscall_enter_from_user_mode+0x21/0x70
[   44.119678][ T3081]  ? syscall_enter_from_user_mode+0x21/0x70
[   44.119711][ T3081]  do_syscall_64+0x35/0x80
[   44.119751][ T3081]  entry_SYSCALL_64_after_hwframe+0x46/0xb0
[   44.119786][ T3081] RIP: 0033:0x7ffb0251a9b9
[   44.119816][ T3081] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a7 54 0c 00 f7 d8 64 89 01 48
[   44.119842][ T3081] RSP: 002b:00007ffcce124e78 EFLAGS: 00000287 ORIG_RAX: 0000000000000010
[   44.119871][ T3081] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007ffb0251a9b9
[   44.119890][ T3081] RDX: 00000000200001c0 RSI: 0000000000004601 RDI: 0000000000000004
[   44.119909][ T3081] RBP: 00007ffcce124e90 R08: 00007ffcce124e90 R09: 00007ffcce124e90
[   44.119928][ T3081] R10: 00007ffcce124e90 R11: 0000000000000287 R12: 000055b9dba381d0
[   44.119947][ T3081] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[   44.119968][ T3081]  </TASK>
[   44.119980][ T3081] Kernel panic - not syncing: panic_on_warn set ...
[   44.119991][ T3081] CPU: 2 PID: 3081 Comm: a.out Not tainted 5.19.0-rc4-00004-g11dd75029515 #17
[   44.120017][ T3081] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1.fc35 04/01/2014
[   44.120031][ T3081] Call Trace:
[   44.120037][ T3081]  <TASK>
[   44.120046][ T3081]  dump_stack_lvl+0xcd/0x134
[   44.120075][ T3081]  panic+0x2d3/0x632
[   44.120099][ T3081]  ? panic_print_sys_info.part.0+0x10b/0x10b
[   44.120128][ T3081]  ? __warn.cold+0x1d1/0x2c5
[   44.120153][ T3081]  ? fb_set_var.cold+0x10d/0x1bc
[   44.120176][ T3081]  __warn.cold+0x1e2/0x2c5
[   44.120200][ T3081]  ? fb_set_var.cold+0x10d/0x1bc
[   44.120224][ T3081]  report_bug+0x1c0/0x210
[   44.120253][ T3081]  handle_bug+0x3c/0x60
[   44.120277][ T3081]  exc_invalid_op+0x14/0x40
[   44.120303][ T3081]  asm_exc_invalid_op+0x1b/0x20
[   44.120326][ T3081] RIP: 0010:fb_set_var.cold+0x10d/0x1bc
[   44.120352][ T3081] Code: b6 14 02 48 89 f0 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85 92 00 00 00 89 4d 0c e9 41 d8 5a fc 44 89 44 24 08 e8 3f b2 3a fb <0f> 0b 8b 74 24 08 49 8d 94 24 d0 01 00 00 48 c7 c7 60 b9 d9 86 e8
[   44.120375][ T3081] RSP: 0018:ffffc900019d7808 EFLAGS: 00010293
[   44.120395][ T3081] RAX: 0000000000000000 RBX: 1ffff9200033af07 RCX: 0000000000000000
[   44.120412][ T3081] RDX: ffff888043a39c00 RSI: ffffffff863b5621 RDI: 0000000000000004
[   44.120429][ T3081] RBP: ffffc900019d7be0 R08: 0000000000000000 R09: 0000000000000000
[   44.120445][ T3081] R10: 0000000000000340 R11: 0000000000000000 R12: ffff888041abc000
[   44.120462][ T3081] R13: ffffc900019d7c34 R14: 0000000000000000 R15: 0000000000000080
[   44.120480][ T3081]  ? fb_set_var.cold+0x10d/0x1bc
[   44.120505][ T3081]  ? fb_set_var.cold+0x10d/0x1bc
[   44.120529][ T3081]  ? fb_blank+0x190/0x190
[   44.120558][ T3081]  ? rcu_read_lock_sched_held+0x3a/0x70
[   44.120586][ T3081]  ? trace_contention_end+0xea/0x150
[   44.120612][ T3081]  ? __mutex_lock+0x259/0x1450
[   44.120639][ T3081]  ? do_fb_ioctl+0x2fd/0x6f0
[   44.120667][ T3081]  ? mutex_lock_io_nested+0x1260/0x1260
[   44.120695][ T3081]  ? lock_downgrade+0x6e0/0x6e0
[   44.120723][ T3081]  ? rwlock_bug.part.0+0x90/0x90
[   44.120777][ T3081]  ? _raw_spin_lock_irqsave+0x4e/0x50
[   44.120809][ T3081]  ? is_console_locked+0x5/0x10
[   44.120835][ T3081]  ? fbcon_info_from_console+0x61/0x190
[   44.120860][ T3081]  ? fbcon_modechange_possible+0x359/0x4c0
[   44.120887][ T3081]  do_fb_ioctl+0x63b/0x6f0
[   44.120914][ T3081]  ? putname+0xfe/0x140
[   44.120940][ T3081]  ? fb_set_suspend+0x1a0/0x1a0
[   44.120968][ T3081]  ? path_openat+0x1016/0x2810
[   44.120995][ T3081]  ? mark_lock.part.0+0xfc/0x1a00
[   44.121025][ T3081]  ? lock_chain_count+0x20/0x20
[   44.121054][ T3081]  ? lock_chain_count+0x20/0x20
[   44.121081][ T3081]  ? lock_downgrade+0x6e0/0x6e0
[   44.121110][ T3081]  ? _raw_spin_unlock_irqrestore+0x50/0x70
[   44.121146][ T3081]  fb_ioctl+0xe7/0x150
[   44.121173][ T3081]  ? do_fb_ioctl+0x6f0/0x6f0
[   44.121200][ T3081]  __x64_sys_ioctl+0x94c/0x18a0
[   44.121231][ T3081]  ? vfs_fileattr_set+0xb70/0xb70
[   44.121261][ T3081]  ? find_held_lock+0x2d/0x110
[   44.121289][ T3081]  ? __context_tracking_exit+0xb8/0xe0
[   44.121318][ T3081]  ? lock_downgrade+0x6e0/0x6e0
[   44.121347][ T3081]  ? lock_downgrade+0x6e0/0x6e0
[   44.121377][ T3081]  ? syscall_enter_from_user_mode+0x21/0x70
[   44.121407][ T3081]  ? syscall_enter_from_user_mode+0x21/0x70
[   44.121439][ T3081]  do_syscall_64+0x35/0x80
[   44.121464][ T3081]  entry_SYSCALL_64_after_hwframe+0x46/0xb0
[   44.121497][ T3081] RIP: 0033:0x7ffb0251a9b9
[   44.121515][ T3081] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a7 54 0c 00 f7 d8 64 89 01 48
[   44.121539][ T3081] RSP: 002b:00007ffcce124e78 EFLAGS: 00000287 ORIG_RAX: 0000000000000010
[   44.121564][ T3081] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007ffb0251a9b9
[   44.121581][ T3081] RDX: 00000000200001c0 RSI: 0000000000004601 RDI: 0000000000000004
[   44.121597][ T3081] RBP: 00007ffcce124e90 R08: 00007ffcce124e90 R09: 00007ffcce124e90
[   44.121614][ T3081] R10: 00007ffcce124e90 R11: 0000000000000287 R12: 000055b9dba381d0
[   44.121631][ T3081] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[   44.121650][ T3081]  </TASK>
[   44.122149][ T3081] Kernel Offset: disabled
[   44.291225][ T3081] Rebooting in 86400 seconds..

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

[Geert Uytterhoeven]

Hi Helge,

On Thu, Jun 30, 2022 at 9:38 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, Jun 30, 2022 at 9:17 PM Helge Deller <deller@gmx.de> wrote:
> > On 6/30/22 21:11, Geert Uytterhoeven wrote:
> > > On Wed, Jun 29, 2022 at 10:00 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.
> > >>
> > >> 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
> > >> @@ -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;
> > >
> > > This should be moved below the call to info->fbops->fb_check_var(),
> > > so the WARN_ON() catches buggy fbdev drivers, not userspace fuzzers.
> >
> > Yes, makes sense.
>
> And print the name of the frame buffer device driver, so people know
> who to blame.

Or better, do not continue, but return with a failure:

    if (WARN(var->xres_virtual < var->xres || var->yres_virtual < var->yres,
        "%ps for %s is broken\n", info->fbops->fb_check_var, info->fix.id)
            return -EINVAL;

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 2/5] fbcon: Fix up user-provided virtual screen size

[Geert Uytterhoeven]

Hi Helge,

On Fri, Jul 1, 2022 at 11:30 AM Helge Deller <deller@gmx.de> wrote:
> On 7/1/22 09:28, Geert Uytterhoeven wrote:
> > On Thu, Jun 30, 2022 at 10:10 PM Helge Deller <deller@gmx.de> wrote:
> >> On 6/30/22 22:00, Geert Uytterhoeven wrote:
> >>> On Thu, Jun 30, 2022 at 9:46 PM Helge Deller <deller@gmx.de> wrote:
> >>>> On 6/30/22 21:36, Geert Uytterhoeven wrote:
> >>>>> On Thu, Jun 30, 2022 at 9:31 PM Helge Deller <deller@gmx.de> wrote:
> >>>>>> On 6/30/22 21:00, Geert Uytterhoeven wrote:
> >>>>>>> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@gmx.de> wrote:
> >>>>>>>> The virtual screen size can't be smaller than the physical screen size.
> >>>>>>>> Based on the general rule that we round up user-provided input if
> >>>>>>>> neccessary, adjust the virtual screen size as well if needed.
> >>>>>>>>
> >>>>>>>> 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
> >>>>>>>> @@ -1106,6 +1106,11 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
> >>>>>>>>                         return -EFAULT;
> >>>>>>>>                 console_lock();
> >>>>>>>>                 lock_fb_info(info);
> >>>>>>>> +               /* adjust virtual screen size if user missed it */
> >>>>>>>> +               if (var.xres_virtual < var.xres)
> >>>>>>>> +                       var.xres_virtual = var.xres;
> >>>>>>>> +               if (var.yres_virtual < var.yres)
> >>>>>>>> +                       var.yres_virtual = var.yres;
> >>>>>>>>                 ret = fb_set_var(info, &var);
> >>>>>>>>                 if (!ret)
> >>>>>>>>                         fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);
> >>>>>>>
> >>>>>>> Given "[PATCH 4/5] fbmem: Prevent invalid virtual screen sizes in
> >>>>>>> fb_set_var", I don't think we need this patch.
> >>>>>>
> >>>>>> We do.
> >>>>>
> >>>>> Why? It will be caught by [PATCH 4/5].
> >>>>
> >>>> Right, it will be caught by patch #4.
> >>>> But if you drop this part, then everytime a user runs
> >>>>         fbset -xres 800 -yres 600 -xvres 200
> >>>> users will get the KERNEL BUG WARNING (from patch #4) including
> >>>> a kernel backtrace in their syslogs.
> >>>
> >>> No, they will only see that warning if they are using a broken fbdev
> >>> driver that implements .fb_check_var(), but fails to validate or
> >>> update the passed geometry.
> >>
> >> IMHO this argument is mood.
> >> That way you put pressure on and need such simple code in
> >> each single driver to fix it up, instead of cleaning it up at a central
> >> place.
> >
> > Most hardware has restrictions on resolution (e.g. xres must be a
> > multiple of N), so the driver has to round up the resolution to make
> > it fit.  And after that the driver has to validate and update the
> > virtual resolution again anyway...
> >
> > If a driver does not support changing the video mode, it can leave
> > out the .fb_check_var() and .fb_set_par() callbacks, so the fbdev
> > core will ignore the userspace-supplied parameters, and reinstate
> > the single supported mode. See e.g. "[PATCH] drm/fb-helper:
> > Remove helpers to change frame buffer config"
> > (https://lore.kernel.org/all/20220629105658.1373770-1-geert@linux-m68k.org).
>
> I implemented all of your suggested changes - from this mail and the others.
> I've committed a new testing tree to the fbcon-fix-testing branch at:
> https://github.com/hdeller/linux/tree/fbcon-fix-testing
> The diff is here:
> https://github.com/torvalds/linux/compare/master...hdeller:linux:fbcon-fix-testing
>
> Although your idea is good since we now would find issues in the drivers,
> I don't think we want to commit it, since the testcase from
> the bug report then immediately crashes the kernel - see below.

That is expected behavior with panic_on_warn?
The right fix is to fix the broken .fb_check_var() implementation.

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 2/5] fbcon: Fix up user-provided virtual screen size

[Helge Deller]

On 7/1/22 16:52, Geert Uytterhoeven wrote:
> Hi Helge,
>
> On Fri, Jul 1, 2022 at 11:30 AM Helge Deller <deller@gmx.de> wrote:
>> On 7/1/22 09:28, Geert Uytterhoeven wrote:
>>> On Thu, Jun 30, 2022 at 10:10 PM Helge Deller <deller@gmx.de> wrote:
>>>> On 6/30/22 22:00, Geert Uytterhoeven wrote:
>>>>> On Thu, Jun 30, 2022 at 9:46 PM Helge Deller <deller@gmx.de> wrote:
>>>>>> On 6/30/22 21:36, Geert Uytterhoeven wrote:
>>>>>>> On Thu, Jun 30, 2022 at 9:31 PM Helge Deller <deller@gmx.de> wrote:
>>>>>>>> On 6/30/22 21:00, Geert Uytterhoeven wrote:
>>>>>>>>> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@gmx.de> wrote:
>>>>>>>>>> The virtual screen size can't be smaller than the physical screen size.
>>>>>>>>>> Based on the general rule that we round up user-provided input if
>>>>>>>>>> neccessary, adjust the virtual screen size as well if needed.
>>>>>>>>>>
>>>>>>>>>> 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
>>>>>>>>>> @@ -1106,6 +1106,11 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>>>>>>>>>>                         return -EFAULT;
>>>>>>>>>>                 console_lock();
>>>>>>>>>>                 lock_fb_info(info);
>>>>>>>>>> +               /* adjust virtual screen size if user missed it */
>>>>>>>>>> +               if (var.xres_virtual < var.xres)
>>>>>>>>>> +                       var.xres_virtual = var.xres;
>>>>>>>>>> +               if (var.yres_virtual < var.yres)
>>>>>>>>>> +                       var.yres_virtual = var.yres;
>>>>>>>>>>                 ret = fb_set_var(info, &var);
>>>>>>>>>>                 if (!ret)
>>>>>>>>>>                         fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);
>>>>>>>>>
>>>>>>>>> Given "[PATCH 4/5] fbmem: Prevent invalid virtual screen sizes in
>>>>>>>>> fb_set_var", I don't think we need this patch.
>>>>>>>>
>>>>>>>> We do.
>>>>>>>
>>>>>>> Why? It will be caught by [PATCH 4/5].
>>>>>>
>>>>>> Right, it will be caught by patch #4.
>>>>>> But if you drop this part, then everytime a user runs
>>>>>>         fbset -xres 800 -yres 600 -xvres 200
>>>>>> users will get the KERNEL BUG WARNING (from patch #4) including
>>>>>> a kernel backtrace in their syslogs.
>>>>>
>>>>> No, they will only see that warning if they are using a broken fbdev
>>>>> driver that implements .fb_check_var(), but fails to validate or
>>>>> update the passed geometry.
>>>>
>>>> IMHO this argument is mood.
>>>> That way you put pressure on and need such simple code in
>>>> each single driver to fix it up, instead of cleaning it up at a central
>>>> place.
>>>
>>> Most hardware has restrictions on resolution (e.g. xres must be a
>>> multiple of N), so the driver has to round up the resolution to make
>>> it fit.  And after that the driver has to validate and update the
>>> virtual resolution again anyway...
>>>
>>> If a driver does not support changing the video mode, it can leave
>>> out the .fb_check_var() and .fb_set_par() callbacks, so the fbdev
>>> core will ignore the userspace-supplied parameters, and reinstate
>>> the single supported mode. See e.g. "[PATCH] drm/fb-helper:
>>> Remove helpers to change frame buffer config"
>>> (https://lore.kernel.org/all/20220629105658.1373770-1-geert@linux-m68k.org).
>>
>> I implemented all of your suggested changes - from this mail and the others.
>> I've committed a new testing tree to the fbcon-fix-testing branch at:
>> https://github.com/hdeller/linux/tree/fbcon-fix-testing
>> The diff is here:
>> https://github.com/torvalds/linux/compare/master...hdeller:linux:fbcon-fix-testing
>>
>> Although your idea is good since we now would find issues in the drivers,
>> I don't think we want to commit it, since the testcase from
>> the bug report then immediately crashes the kernel - see below.
>
> That is expected behavior with panic_on_warn?

Oh well. You're right!
The kernel config had panic_on_warn enabled, which I didn't noticed.
I disabled that option and now it works:

[  147.430332][ T3171] WARNING: CPU: 0 PID: 3171 at drivers/video/fbdev/core/fbmem.c:1025 fb_set_var.cold+0x83/0x1bc
....
[  147.431126][ T3171] ---[ end trace 0000000000000000 ]---
[  147.431132][ T3171] fbcon: Fix up invalid yres 0 for bochs-drmdrmfb

> The right fix is to fix the broken .fb_check_var() implementation.

Yep.
I'll send this patch series now.

Helge

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

[Michel Dänzer]

On 2022-07-01 16:49, Geert Uytterhoeven wrote:
> On Thu, Jun 30, 2022 at 9:38 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Thu, Jun 30, 2022 at 9:17 PM Helge Deller <deller@gmx.de> wrote:
>>> On 6/30/22 21:11, Geert Uytterhoeven wrote:
>>>> On Wed, Jun 29, 2022 at 10:00 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.
>>>>>
>>>>> 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
>>>>> @@ -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;
>>>>
>>>> This should be moved below the call to info->fbops->fb_check_var(),
>>>> so the WARN_ON() catches buggy fbdev drivers, not userspace fuzzers.
>>>
>>> Yes, makes sense.
>>
>> And print the name of the frame buffer device driver, so people know
>> who to blame.
> 
> Or better, do not continue, but return with a failure:
> 
>     if (WARN(var->xres_virtual < var->xres || var->yres_virtual < var->yres,
>         "%ps for %s is broken\n", info->fbops->fb_check_var, info->fix.id)
>             return -EINVAL;

I'd also recommend WARN(_ON)_ONCE, or users with a broken driver might get spammed.


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer

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

[Helge Deller]

On 7/2/22 14:05, Michel Dänzer wrote:
> On 2022-07-01 16:49, Geert Uytterhoeven wrote:
>> On Thu, Jun 30, 2022 at 9:38 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Thu, Jun 30, 2022 at 9:17 PM Helge Deller <deller@gmx.de> wrote:
>>>> On 6/30/22 21:11, Geert Uytterhoeven wrote:
>>>>> On Wed, Jun 29, 2022 at 10:00 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.
>>>>>>
>>>>>> 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
>>>>>> @@ -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;
>>>>>
>>>>> This should be moved below the call to info->fbops->fb_check_var(),
>>>>> so the WARN_ON() catches buggy fbdev drivers, not userspace fuzzers.
>>>>
>>>> Yes, makes sense.
>>>
>>> And print the name of the frame buffer device driver, so people know
>>> who to blame.
>>
>> Or better, do not continue, but return with a failure:
>>
>>     if (WARN(var->xres_virtual < var->xres || var->yres_virtual < var->yres,
>>         "%ps for %s is broken\n", info->fbops->fb_check_var, info->fix.id)
>>             return -EINVAL;
>
> I'd also recommend WARN(_ON)_ONCE, or users with a broken driver might get spammed.

Yes, that's probably better. Will do.

Thanks!
Helge