LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
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
Date: Mon, 20 May 2019 22:55:40 -0400 (EDT)	[thread overview]
Message-ID: <nycvar.YSQ.7.76.1905202242410.1558@knanqh.ubzr> (raw)
In-Reply-To: <20190521022940.GA4858@zhanggen-UX430UQ>

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);
>  
>  ---
> 

  reply	other threads:[~2019-05-21  3:05 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-21  2:29 Gen Zhang
2019-05-21  2:55 ` Nicolas Pitre [this message]
2019-05-21  3:09   ` Gen Zhang
2019-05-21  3:26     ` Nicolas Pitre
2019-05-21  4:00       ` Gen Zhang
2019-05-21  4:30         ` Nicolas Pitre
2019-05-21  7:39           ` Gen Zhang
2019-05-22  2:43             ` Nicolas Pitre
2019-05-22  8:10               ` Gen Zhang
2019-05-22 12:19               ` [PATCH v3] " Gen Zhang
2019-05-22 14:18                 ` Nicolas Pitre
2019-05-24  2:27                   ` [PATCH v3] vt: Fix a missing-check bug in con_init() Gen Zhang
2019-05-21  3:21   ` [PATCH v3] vt: Fix a missing-check bug in drivers/tty/vt/vt.c Gen Zhang
2019-05-21  6:46     ` Oleksandr Natalenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=nycvar.YSQ.7.76.1905202242410.1558@knanqh.ubzr \
    --to=nico@fluxnic.net \
    --cc=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' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).