[PATCH] fbcon: Check font dimension limits

blit_x and blit_y are uint32_t, so fbcon currently cannot support fonts
larger than 32x32.

The 32x32 case also needs shifting an unsigned int, to properly set bit
31, otherwise we get "UBSAN: shift-out-of-bounds in fbcon_set_font",
as reported on

http://lore.kernel.org/all/IA1PR07MB98308653E259A6F2CE94A4AFABCE9@IA1PR07MB9830.namprd07.prod.outlook.com
Kernel Branch: 6.2.0-rc5-next-20230124
Kernel config: https://drive.google.com/file/d/1F-LszDAizEEH0ZX0HcSR06v5q8FPl2Uv/view?usp=sharing
Reproducer: https://drive.google.com/file/d/1mP1jcLBY7vWCNM60OMf-ogw-urQRjNrm/view?usp=sharing

Reported-by: Sanan Hasanov <redacted>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

Index: linux-6.0/drivers/video/fbdev/core/fbcon.c
===================================================================

--- linux-6.0.orig/drivers/video/fbdev/core/fbcon.c
+++ linux-6.0/drivers/video/fbdev/core/fbcon.c

@@ -2489,9 +2489,12 @@ static int fbcon_set_font(struct vc_data
 	    h > FBCON_SWAP(info->var.rotate, info->var.yres, info->var.xres))
 		return -EINVAL;
 
+	if (font->width > 32 || font->height > 32)
+		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))))
+	if (!(info->pixmap.blit_x & (1U << (font->width - 1))) ||
+	    !(info->pixmap.blit_y & (1U << (font->height - 1))))
 		return -EINVAL;
 
 	/* Make sure driver can handle the font length */

(FYI, the UB was already a problem before my big-font patch submission,
the checks I add here are already making sense in previous versions
anyway)

Samuel Thibault, le jeu. 26 janv. 2023 01:49:12 +0100, a ecrit:

On Thu, Jan 26, 2023 at 01:49:12AM +0100, Samuel Thibault wrote:

blit_x and blit_y are uint32_t, so fbcon currently cannot support fonts
larger than 32x32.

"u32" you mean, right?

The 32x32 case also needs shifting an unsigned int, to properly set bit
31, otherwise we get "UBSAN: shift-out-of-bounds in fbcon_set_font",
as reported on

http://lore.kernel.org/all/IA1PR07MB98308653E259A6F2CE94A4AFABCE9@IA1PR07MB9830.namprd07.prod.outlook.com

Odd blank line?

Kernel Branch: 6.2.0-rc5-next-20230124
Kernel config: https://drive.google.com/file/d/1F-LszDAizEEH0ZX0HcSR06v5q8FPl2Uv/view?usp=sharing
Reproducer: https://drive.google.com/file/d/1mP1jcLBY7vWCNM60OMf-ogw-urQRjNrm/view?usp=sharing

What are all of these lines for?

Reported-by: Sanan Hasanov <redacted>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

What commit id does this fix?  Should it go to stable kernels?

Are you sure this is still needed with the above check added?  If so,
why?  What is the difference in the compiled code?

thanks,

greg k-h

On 26. 01. 23, 1:49, Samuel Thibault wrote:

So use BIT() properly then? That should be used in all these shifts 
anyway. Exactly to avoid UB.

thanks,
-- 
js
suse labs

On 26. 01. 23, 8:43, Greg KH wrote:

quoted

--- linux-6.0.orig/drivers/video/fbdev/core/fbcon.c
+++ linux-6.0/drivers/video/fbdev/core/fbcon.c

@@ -2489,9 +2489,12 @@ static int fbcon_set_font(struct vc_data
  	    h > FBCON_SWAP(info->var.rotate, info->var.yres, info->var.xres))
  		return -EINVAL;
  
+	if (font->width > 32 || font->height > 32)
+		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))))
+	if (!(info->pixmap.blit_x & (1U << (font->width - 1))) ||
+	    !(info->pixmap.blit_y & (1U << (font->height - 1))))

Are you sure this is still needed with the above check added?  If so,
why?  What is the difference in the compiled code?

For font->{width,height} == 32, definitely. IMO, 1 << 31 is undefined as 
1 << 31 cannot be represented by an (signed) int.

-- 
js
suse labs

Jiri Slaby, le jeu. 26 janv. 2023 10:02:55 +0100, a ecrit:

On 26. 01. 23, 1:49, Samuel Thibault wrote:

quoted

Index: linux-6.0/drivers/video/fbdev/core/fbcon.c
===================================================================

--- linux-6.0.orig/drivers/video/fbdev/core/fbcon.c
+++ linux-6.0/drivers/video/fbdev/core/fbcon.c

@@ -2489,9 +2489,12 @@ static int fbcon_set_font(struct vc_data
  	    h > FBCON_SWAP(info->var.rotate, info->var.yres, info->var.xres))
  		return -EINVAL;
+	if (font->width > 32 || font->height > 32)
+		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))))
+	if (!(info->pixmap.blit_x & (1U << (font->width - 1))) ||
+	    !(info->pixmap.blit_y & (1U << (font->height - 1))))

So use BIT() properly then? That should be used in all these shifts anyway.
Exactly to avoid UB.

Right, I'll use that in next version.

Samuel

Greg KH, le jeu. 26 janv. 2023 08:43:02 +0100, a ecrit:

On Thu, Jan 26, 2023 at 01:49:12AM +0100, Samuel Thibault wrote:

quoted

blit_x and blit_y are uint32_t, so fbcon currently cannot support fonts
larger than 32x32.

"u32" you mean, right?

Right :)

quoted

The 32x32 case also needs shifting an unsigned int, to properly set bit
31, otherwise we get "UBSAN: shift-out-of-bounds in fbcon_set_font",
as reported on

http://lore.kernel.org/all/IA1PR07MB98308653E259A6F2CE94A4AFABCE9@IA1PR07MB9830.namprd07.prod.outlook.com

Odd blank line?

Not sure what you mean? I found it easier to read to put a blank line
between the text and the references.

quoted

Kernel Branch: 6.2.0-rc5-next-20230124
Kernel config: https://drive.google.com/file/d/1F-LszDAizEEH0ZX0HcSR06v5q8FPl2Uv/view?usp=sharing
Reproducer: https://drive.google.com/file/d/1mP1jcLBY7vWCNM60OMf-ogw-urQRjNrm/view?usp=sharing

What are all of these lines for?

They provide the references that were set in the original report

http://lore.kernel.org/all/IA1PR07MB98308653E259A6F2CE94A4AFABCE9@IA1PR07MB9830.namprd07.prod.outlook.com

Arguably the "branch" and "config" are not that useful since the bug has
been there for more than a dozen years, but notably the "Reproducer" is
useful to provide a userland program that triggers the UB.

quoted

Reported-by: Sanan Hasanov <redacted>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

What commit id does this fix?

I though it was there forever, but it seems the check was added in 2007,
so I'll add it in next version.

Should it go to stable kernels?

Yes. I added stable in Cc of the mail but missed adding it in the text,
I will add it.

quoted

Index: linux-6.0/drivers/video/fbdev/core/fbcon.c
===================================================================

--- linux-6.0.orig/drivers/video/fbdev/core/fbcon.c
+++ linux-6.0/drivers/video/fbdev/core/fbcon.c

@@ -2489,9 +2489,12 @@ static int fbcon_set_font(struct vc_data
 	    h > FBCON_SWAP(info->var.rotate, info->var.yres, info->var.xres))
 		return -EINVAL;
 
+	if (font->width > 32 || font->height > 32)
+		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))))
+	if (!(info->pixmap.blit_x & (1U << (font->width - 1))) ||
+	    !(info->pixmap.blit_y & (1U << (font->height - 1))))

Are you sure this is still needed with the above check added?  If so,
why?  What is the difference in the compiled code?

As mentioned by Jiri, yes in the 32 case it's needed otherwise it's UB.

Samuel