Re: [PATCH] vt: Reject zero-sized screen buffer size.

[Greg Kroah-Hartman]

On Fri, Jul 10, 2020 at 08:31:42PM +0900, Tetsuo Handa wrote:
> On 2020/07/10 19:55, Greg Kroah-Hartman wrote:
> >> 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.
> 
> OK. I can change to
> 
> +	if (vc->vc_screenbuf_size > KMALLOC_MAX_SIZE || !vc->vc_screenbuf_size)
> +		goto err_free;
>  	vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_KERNEL);
>  	if (!vc->vc_screenbuf)
>  		goto err_free;
> 
> like vc_do_resize() does. But I'm currently waiting for syzbot to test this patch, for
> I don't have an environment for reproducing this problem.

That looks much more sane, thanks.


> 
> > 
> >>  		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?
> 
> This is an __init function. Can we somehow pass column=0 or row=0 ?

You could, it's much less likely, but why not catch this if you can?

> > Just checking the values and rejecting that as a valid screen size
> > should be sufficient.
> 
> Hmm, where are we checking that column * row does not exceed UINT_MAX, given that
> "struct vc_data"->vc_{cols,rows,screenbuf_size} are "unsigned int" and we do
> 
>   vc->vc_size_row = vc->vc_cols << 1;
>   vc->vc_screenbuf_size = vc->vc_rows * vc->vc_size_row;
> 
> in visual_init() ? Don't we need to reject earlier?

Probably, it's some twisty code :(