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:

...

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

"u32" you mean, right?

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@IA1PR07MB9...

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.

...

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

What are all of these lines for?

They provide the references that were set in the original report

http://lore.kernel.org/all/IA1PR07MB98308653E259A6F2CE94A4AFABCE9@IA1PR07MB9...

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.

...

Reported-by: Sanan Hasanov sanan.hasanov@Knights.ucf.edu 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.

...

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

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

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

...

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