Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 20 May 2019 22:55:40 -0400 (EDT)
From:   Nicolas Pitre <nico@fluxnic.net>
To:     Gen Zhang <blackgod016574@gmail.com>
cc:     linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] vt: Fix a missing-check bug in drivers/tty/vt/vt.c
In-Reply-To: <20190521022940.GA4858@zhanggen-UX430UQ>
Message-ID: <nycvar.YSQ.7.76.1905202242410.1558@knanqh.ubzr>
References: <20190521022940.GA4858@zhanggen-UX430UQ>
User-Agent: Alpine 2.21 (LFD 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 21 May 2019, Gen Zhang wrote:

> In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> vc->vc_screenbuf is allocated a memory space via kzalloc(). And they are
> used in the following codes.
> However, when there is a memory allocation error, kzalloc() can fail.
> Thus null pointer (vc_cons[currcons].d, vc and vc->vc_screenbuf)
> dereference may happen. And it will cause the kernel to crash. Therefore,
> we should check return value and handle the error.
> Further,the loop condition MIN_NR_CONSOLES is defined as 1 in
> include/uapi/linux/vt.h. So there is no need to unwind the loop.

But what if someone changes that define? It won't be obvious that some 
code did rely on it to be defined to 1.

> Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> 
> ---
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index fdd12f8..b756609 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -3350,10 +3350,14 @@ static int __init con_init(void)
>  
>  	for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
>  		vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
> +		if (!vc_cons[currcons].d || !vc)

Both vc_cons[currcons].d and vc are assigned the same value on the 
previous line. You don't have to test them both.

> +			goto err_vc;
>  		INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
>  		tty_port_init(&vc->port);
>  		visual_init(vc, currcons, 1);
>  		vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
> +		if (!vc->vc_screenbuf)
> +			goto err_vc_screenbuf;
>  		vc_init(vc, vc->vc_rows, vc->vc_cols,
>  			currcons || !vc->vc_sw->con_save_screen);
>  	}
> @@ -3375,6 +3379,14 @@ static int __init con_init(void)
>  	register_console(&vt_console_driver);
>  #endif
>  	return 0;
> +err_vc:
> +	console_unlock();
> +	return -ENOMEM;
> +err_vc_screenbuf:
> +	console_unlock();
> +	kfree(vc);
> +	vc_cons[currcons].d = NULL;
> +	return -ENOMEM;

As soon as you release the lock, another thread could come along and 
start using the memory pointed by vc_cons[currcons].d you're about to 
free here. This is unlikely for an initcall, but still.

You should consider this ordering instead:

err_vc_screenbuf:
	kfree(vc);
	vc_cons[currcons].d = NULL;
err_vc:
	console_unlock();
	return -ENOMEM;


>  }
>  console_initcall(con_init);
>  
>  ---
>