Re: [PATCH] fbcon: Check font dimension limits
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
Re: [PATCH] fbcon: Check font dimension limits
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/ia1pr07mb98308653e259a6f2ce94a4afab...@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. > > 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/ia1pr07mb98308653e259a6f2ce94a4afab...@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. > > Reported-by: Sanan Hasanov > > Signed-off-by: Samuel Thibault > > 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
[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/ia1pr07mb98308653e259a6f2ce94a4afab...@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 Signed-off-by: Samuel Thibault 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 */
Re: [PATCH] fbcon: Check font dimension limits
(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: > 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/ia1pr07mb98308653e259a6f2ce94a4afab...@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 > Signed-off-by: Samuel Thibault > > 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 */ > >
Re: [PATCH] fbcon: Check font dimension limits
On 26. 01. 23, 8:43, Greg KH wrote: --- 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
Re: [PATCH] fbcon: Check font dimension limits
On 26. 01. 23, 1:49, Samuel Thibault wrote: 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/ia1pr07mb98308653e259a6f2ce94a4afab...@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 Signed-off-by: Samuel Thibault 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. thanks, -- js suse labs
Re: [PATCH] fbcon: Check font dimension limits
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/ia1pr07mb98308653e259a6f2ce94a4afab...@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 > Signed-off-by: Samuel Thibault What commit id does this fix? Should it go to stable kernels? > > 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? thanks, greg k-h