[PATCH 1/1] vt_ioctl: prevent VT_RESIZEX font height change from causing potential out-of-bounds access

[PATCH 1/1] vt_ioctl: prevent VT_RESIZEX font height change from causing potential out-of-bounds access

[George Kennedy]

Add a VT_RESIZEX check to ensure that changing the font height will not
cause a potential out-of-bounds access. The candidate font height contained
in "v_clin", though below the max, could still result in accesses beyond
the allocated font data size.

Signed-off-by: George Kennedy <george.kennedy@oracle.com>
Reported-by: syzbot+38a3699c7eaf165b97a6@syzkaller.appspotmail.com
---
 drivers/tty/vt/vt_ioctl.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
index daf61c2..6185f1a 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -342,6 +342,9 @@ static void vt_disallocate_all(void)
 	}
 }
 
+/* from fbcon.c */
+#define FNTSIZE(fd) (((int *)(fd))[-2])
+#define FNTCHARCNT(fd) (((int *)(fd))[-3])
 
 /*
  * We handle the console-specific ioctl's here.  We allow the
@@ -895,8 +898,23 @@ int vt_ioctl(struct tty_struct *tty,
 			if (vcp) {
 				if (v.v_vlin)
 					vcp->vc_scan_lines = v.v_vlin;
-				if (v.v_clin)
+				if (v.v_clin) {
+					int width, pitch, size;
+
+					width = (vcp->vc_font.width > 8) ? 8 : vcp->vc_font.width;
+					pitch = (width + 7) >> 3;
+
+					pitch = (pitch) ? pitch : 1;
+
+					/* font size = height * pitch * charcount */
+					size = v.v_clin * pitch * FNTCHARCNT(vcp->vc_font.data);
+
+					if (size > FNTSIZE(vcp->vc_font.data)) {
+						console_unlock();
+						return -EINVAL;
+					}
 					vcp->vc_font.height = v.v_clin;
+				}
 				vcp->vc_resize_user = 1;
 				vc_resize(vcp, v.v_cols, v.v_rows);
 			}
-- 
1.8.3.1

Re: [PATCH 1/1] vt_ioctl: prevent VT_RESIZEX font height change from causing potential out-of-bounds access

[Greg KH]

On Wed, Jul 29, 2020 at 08:39:41AM -0400, George Kennedy wrote:
> Add a VT_RESIZEX check to ensure that changing the font height will not
> cause a potential out-of-bounds access. The candidate font height contained
> in "v_clin", though below the max, could still result in accesses beyond
> the allocated font data size.
> 
> Signed-off-by: George Kennedy <george.kennedy@oracle.com>
> Reported-by: syzbot+38a3699c7eaf165b97a6@syzkaller.appspotmail.com

Did syzbot also test this that it fixes the reported issue?

What commit does this fix?  Should it go back farther to stable releases
too?

thanks,

greg k-h

Re: [PATCH 1/1] vt_ioctl: prevent VT_RESIZEX font height change from causing potential out-of-bounds access

[George Kennedy]

Hi Greg,

I will re-work this patch based on comments from Dan.

Thank you,
George

On 7/29/2020 8:53 AM, Greg KH wrote:
> On Wed, Jul 29, 2020 at 08:39:41AM -0400, George Kennedy wrote:
>> Add a VT_RESIZEX check to ensure that changing the font height will not
>> cause a potential out-of-bounds access. The candidate font height contained
>> in "v_clin", though below the max, could still result in accesses beyond
>> the allocated font data size.
>>
>> Signed-off-by: George Kennedy <george.kennedy@oracle.com>
>> Reported-by: syzbot+38a3699c7eaf165b97a6@syzkaller.appspotmail.com
> Did syzbot also test this that it fixes the reported issue?
>
> What commit does this fix?  Should it go back farther to stable releases
> too?
>
> thanks,
>
> greg k-h

Re: [PATCH 1/1] vt_ioctl: prevent VT_RESIZEX font height change from causing potential out-of-bounds access

[Dan Carpenter]

On Wed, Jul 29, 2020 at 08:39:41AM -0400, George Kennedy wrote:
> Add a VT_RESIZEX check to ensure that changing the font height will not
> cause a potential out-of-bounds access. The candidate font height contained
> in "v_clin", though below the max, could still result in accesses beyond
> the allocated font data size.
> 
> Signed-off-by: George Kennedy <george.kennedy@oracle.com>
> Reported-by: syzbot+38a3699c7eaf165b97a6@syzkaller.appspotmail.com
> ---
>  drivers/tty/vt/vt_ioctl.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
> index daf61c2..6185f1a 100644
> --- a/drivers/tty/vt/vt_ioctl.c
> +++ b/drivers/tty/vt/vt_ioctl.c
> @@ -342,6 +342,9 @@ static void vt_disallocate_all(void)
>  	}
>  }
>  
> +/* from fbcon.c */
> +#define FNTSIZE(fd) (((int *)(fd))[-2])
> +#define FNTCHARCNT(fd) (((int *)(fd))[-3])

I really hate these macros.  I don't think we can actually use them here
without an out of bounds depending on the driver.

What happens is that:

con_font_set() allocates data:

	font.data = memdup_user(op->data, size);

Then it calls vc->vc_sw->con_font_set(vc, &font, op->flags);

Two of those function implementations newport_set_font() and
fbcon_set_font() make a new allocation, but with a secret extra buffer
to store extra data at the beginning.

  2645  static int fbcon_set_font(struct vc_data *vc, struct console_font *font,
  2646                            unsigned int flags)
  2647  {
  2648          struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
  2649          unsigned charcount = font->charcount;
  2650          int w = font->width;
  2651          int h = font->height;
  2652          int size;
  2653          int i, csum;
  2654          u8 *new_data, *data = font->data;
  2655          int pitch = (font->width+7) >> 3;
  2656  
  2657          /* Is there a reason why fbconsole couldn't handle any charcount >256?
  2658           * If not this check should be changed to charcount < 256 */
  2659          if (charcount != 256 && charcount != 512)
  2660                  return -EINVAL;
  2661  
  2662          /* Make sure drawing engine can handle the font */
  2663          if (!(info->pixmap.blit_x & (1 << (font->width - 1))) ||
  2664              !(info->pixmap.blit_y & (1 << (font->height - 1))))
  2665                  return -EINVAL;
  2666  
  2667          /* Make sure driver can handle the font length */
  2668          if (fbcon_invalid_charcount(info, charcount))
  2669                  return -EINVAL;
  2670  
  2671          size = h * pitch * charcount;
  2672  
  2673          new_data = kmalloc(FONT_EXTRA_WORDS * sizeof(int) + size, GFP_USER);
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Extra space to store hidden data.

  2674  
  2675          if (!new_data)
  2676                  return -ENOMEM;
  2677  
  2678          new_data += FONT_EXTRA_WORDS * sizeof(int);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Hide the new data forever.

  2679          FNTSIZE(new_data) = size;
  2680          FNTCHARCNT(new_data) = charcount;
  2681          REFCOUNT(new_data) = 0; /* usage counter */
  2682          for (i=0; i< charcount; i++) {
  2683                  memcpy(new_data + i*h*pitch, data +  i*32*pitch, h*pitch);
                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Copy the user font data to the new buffer one element at a time?


  2684          }
  2685  
  2686          /* Since linux has a nice crc32 function use it for counting font
  2687           * checksums. */
  2688          csum = crc32(0, new_data, size);
  2689  

So only the two drivers with the secret extra buffer can use FNTSIZE()
and friends.

regards,
dan carpenter