Messages in this thread | | | Subject | Re: [PATCH v2] vt: Reject zero-sized screen buffer size. | From | Brooke Basile <> | Date | Mon, 20 Jul 2020 11:40:18 -0400 |
| |
On 7/10/20 10:34 AM, Tetsuo Handa wrote: > syzbot is reporting general protection fault in do_con_write() [1] caused > by vc->vc_screenbuf == ZERO_SIZE_PTR caused by vc->vc_screenbuf_size == 0 > caused by vc->vc_cols == vc->vc_rows == vc->vc_size_row == 0 being passed > to ioctl(FBIOPUT_VSCREENINFO) request on /dev/fb0 , for gotoxy(vc, 0, 0) > from reset_terminal() from vc_init() from vc_allocate() on such console > causes vc->vc_pos == 0x10000000e due to > ((unsigned long) ZERO_SIZE_PTR) + -1U * 0 + (-1U << 1). > > I don't think that a console with 0 column and/or 0 row makes sense, and > I think that we can reject such bogus arguments in fb_set_var() from > ioctl(FBIOPUT_VSCREENINFO). Regardless, I think that it is safer to also > check vc->vc_screenbuf_size when allocating vc->vc_screenbuf from > vc_allocate() from con_install() from tty_init_dev() from tty_open(). > > Theoretically, cols and rows can be any range as long as > 0 < cols * rows * 2 <= KMALLOC_MAX_SIZE is satisfied (e.g. > cols == 1048576 && rows == 2 is possible) because of > > vc->vc_size_row = vc->vc_cols << 1; > vc->vc_screenbuf_size = vc->vc_rows * vc->vc_size_row; > > in visual_init() and kzalloc(vc->vc_screenbuf_size) in vc_allocate(). > > But since vc_do_resize() requires cols <= 32767 and rows <= 32767, > applying 1 <= cols <= 32767 and 1 <= rows <= 32767 requirements to > vc_allocate() will be practically fine. (cols != 0 && rows != 0 is > implicitly checked via screenbuf_size != 0.) > > This patch does not touch con_init(), for returning -EINVAL there > does not help when we are not returning -ENOMEM. > > [1] https://syzkaller.appspot.com/bug?extid=017265e8553724e514e8 > > Reported-by: syzbot <syzbot+017265e8553724e514e8@syzkaller.appspotmail.com> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > --- > drivers/tty/vt/vt.c | 29 ++++++++++++++++++----------- > 1 file changed, 18 insertions(+), 11 deletions(-) > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c > index 48a8199f7845..42d8c67a481f 100644 > --- a/drivers/tty/vt/vt.c > +++ b/drivers/tty/vt/vt.c > @@ -1092,10 +1092,19 @@ static const struct tty_port_operations vc_port_ops = { > .destruct = vc_port_destruct, > }; > > +/* > + * Change # of rows and columns (0 means unchanged/the size of fg_console) > + * [this is to be used together with some user program > + * like resize that changes the hardware videomode] > + */ > +#define VC_MAXCOL (32767) > +#define VC_MAXROW (32767) > + > int vc_allocate(unsigned int currcons) /* return 0 on success */ > { > struct vt_notifier_param param; > struct vc_data *vc; > + int err; > > WARN_CONSOLE_UNLOCKED(); > > @@ -1125,6 +1134,11 @@ int vc_allocate(unsigned int currcons) /* return 0 on success */ > if (!*vc->vc_uni_pagedir_loc) > con_set_default_unimap(vc); > > + err = -EINVAL; > + if (vc->vc_cols > VC_MAXCOL || vc->vc_rows > VC_MAXROW || > + vc->vc_screenbuf_size > KMALLOC_MAX_SIZE || !vc->vc_screenbuf_size) > + goto err_free; > + err = -ENOMEM; > vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_KERNEL); > if (!vc->vc_screenbuf) > goto err_free; > @@ -1143,7 +1157,7 @@ int vc_allocate(unsigned int currcons) /* return 0 on success */ > visual_deinit(vc); > kfree(vc); > vc_cons[currcons].d = NULL; > - return -ENOMEM; > + return err; > } > > static inline int resize_screen(struct vc_data *vc, int width, int height, > @@ -1158,14 +1172,6 @@ static inline int resize_screen(struct vc_data *vc, int width, int height, > return err; > } > > -/* > - * Change # of rows and columns (0 means unchanged/the size of fg_console) > - * [this is to be used together with some user program > - * like resize that changes the hardware videomode] > - */ > -#define VC_RESIZE_MAXCOL (32767) > -#define VC_RESIZE_MAXROW (32767) > - > /** > * vc_do_resize - resizing method for the tty > * @tty: tty being resized > @@ -1201,7 +1207,7 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc, > user = vc->vc_resize_user; > vc->vc_resize_user = 0; > > - if (cols > VC_RESIZE_MAXCOL || lines > VC_RESIZE_MAXROW) > + if (cols > VC_MAXCOL || lines > VC_MAXROW) > return -EINVAL; > > new_cols = (cols ? cols : vc->vc_cols); > @@ -1212,7 +1218,7 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc, > if (new_cols == vc->vc_cols && new_rows == vc->vc_rows) > return 0; > > - if (new_screen_size > KMALLOC_MAX_SIZE) > + if (new_screen_size > KMALLOC_MAX_SIZE || !new_screen_size) > return -EINVAL; > newscreen = kzalloc(new_screen_size, GFP_USER); > if (!newscreen) > @@ -3393,6 +3399,7 @@ static int __init con_init(void) > INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK); > tty_port_init(&vc->port); > visual_init(vc, currcons, 1); > + /* Assuming vc->vc_{cols,rows,screenbuf_size} are sane here. */ > vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT); > vc_init(vc, vc->vc_rows, vc->vc_cols, > currcons || !vc->vc_sw->con_save_screen); >
Hi,
Looks like this patch also fixes this bug reported by syzbot: https://syzkaller.appspot.com/bug?id=dc5c6b1ae4952a5d72d0e82de0eeeb9e5f767efc
There's a lot of other bugs that were reported by syzbot that also touch this code, so I just wanted to give a heads up in case you weren't already aware of them. It seems like this patch could be a fix for all of them.
Here are the links to those other bugs: https://syzkaller.appspot.com/bug?id=3e2ad4922b18026c1579f50900747401842acdff https://syzkaller.appspot.com/bug?id=7329638ab83b70fc8fab07e14c4b2fcdc73af21d https://syzkaller.appspot.com/bug?id=01703eb07363bd1f9757bc4a54994455fc9db9dc https://syzkaller.appspot.com/bug?id=7a04be77a06aae337077e00f0ecdb2239dfc2fc3 https://syzkaller.appspot.com/bug?id=ff1543b5ade351b9d6c4ef51c805d89422a8271d
Best, Brooke Basile
| |