Messages in this thread | | | Date | Wed, 29 Jul 2020 18:13:30 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH 1/1] vt_ioctl: prevent VT_RESIZEX font height change from causing potential out-of-bounds access |
| |
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
| |