diff mbox series

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

Message ID 20220625122502.68095-2-deller@gmx.de (mailing list archive)
State Superseded
Headers show
Series fbcon: Fixes for screen resolution changes | expand

Commit Message

Helge Deller June 25, 2022, 12:24 p.m. UTC 
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(+)

--
2.35.3

Comments

Daniel Vetter June 25, 2022, 12:45 p.m. UTC | #1 
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
>
Helge Deller June 25, 2022, 2:53 p.m. UTC | #2 
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
Daniel Vetter June 25, 2022, 10:27 p.m. UTC | #3 
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 June 25, 2022, 10:32 p.m. UTC | #4 
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
diff mbox series

Patch

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