On Fri, Jul 10, 2020 at 02:53:29PM +0900, 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 ZERO_SIZE_PTR when allocating vc->vc_screenbuf from vc_allocate() > from con_install() from tty_init_dev() from tty_open(). > > [1] https://syzkaller.appspot.com/bug?extid=017265e8553724e514e8 > > Reported-by: syzbot <syzbot+017265e8553724e51...@syzkaller.appspotmail.com> > Signed-off-by: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp> > --- > drivers/tty/vt/vt.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c > index 48a8199f7845..8497e9206607 100644 > --- a/drivers/tty/vt/vt.c > +++ b/drivers/tty/vt/vt.c > @@ -1126,7 +1126,7 @@ int vc_allocate(unsigned int currcons) /* return 0 on > success */ > con_set_default_unimap(vc); > > vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_KERNEL); > - if (!vc->vc_screenbuf) > + if (ZERO_OR_NULL_PTR(vc->vc_screenbuf))
No, let's check this before we do kzalloc() please, that's just an odd way of doing an allocation we shouldn't have had to do. > goto err_free; > > /* If no drivers have overridden us and the user didn't pass a > @@ -1212,7 +1212,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 +3393,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_screenbuf_size is sane here, for this is > __init code. */ Shouldn't we also check this here, or before we get here, too? Just checking the values and rejecting that as a valid screen size should be sufficient. thanks, greg k-h
