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

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

[Daniel Vetter]

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

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

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

[Daniel Vetter]

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

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

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

[Daniel Vetter]

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

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

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

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

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

[Daniel Vetter]

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

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

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

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

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

[Helge Deller]

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

Thanks!

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

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

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

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

Helge

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

[Helge Deller]

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

Thanks!

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

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

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

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

Helge

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

[Daniel Vetter]

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

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

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

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

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

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

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

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

Anyway just an idea.
-Daniel

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

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

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

[Daniel Vetter]

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

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

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

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

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

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

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

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

Anyway just an idea.
-Daniel

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

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

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

[Helge Deller]

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

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

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

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