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@xxxxxxxxxx>
> Reported-by: syzbot+38a3699c7eaf165b97a6@xxxxxxxxxxxxxxxxxxxxxxxxx
> ---
>  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