Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2] vt: Reject zero-sized screen buffer size.
To:     Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jslaby@suse.com>
Cc:     Dmitry Vyukov <dvyukov@google.com>, linux-kernel@vger.kernel.org,
        syzbot <syzbot+017265e8553724e514e8@syzkaller.appspotmail.com>
References: <20200710113658.GA1238355@kroah.com>
 <20200710143455.3438-1-penguin-kernel@I-love.SAKURA.ne.jp>
From:   Brooke Basile <brookebasile@gmail.com>
Message-ID: <741c66b4-3855-9c5e-ca67-9e44eb6fa083@gmail.com>
Date:   Mon, 20 Jul 2020 11:40:18 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20200710143455.3438-1-penguin-kernel@I-love.SAKURA.ne.jp>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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