linux-kernel - Re: [PATCH] tty:vt: Add check the return value of kzalloc to avoid oops

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LRH.2.02.1909191114340.14850@file01.intranet.prod.int.rdu2.redhat.com>
Date:   Thu, 19 Sep 2019 11:16:17 -0400 (EDT)
From:   Mikulas Patocka <mpatocka@...hat.com>
To:     Greg KH <gregkh@...uxfoundation.org>
cc:     Xiaoming Ni <nixiaoming@...wei.com>, penberg@...helsinki.fi,
        jslaby@...e.com, nico@...xnic.net, textshell@...uujin.de,
        sam@...nborg.org, daniel.vetter@...ll.ch, ghalat@...hat.com,
        linux-kernel@...r.kernel.org, yangyingliang@...wei.com,
        yuehaibing@...wei.com, zengweilin@...wei.com
Subject: Re: [PATCH] tty:vt: Add check the return value of kzalloc to avoid
 oops



On Thu, 19 Sep 2019, Greg KH wrote:

> On Thu, Sep 19, 2019 at 05:18:15PM +0800, Xiaoming Ni wrote:
> > Using kzalloc() to allocate memory in function con_init(), but not
> > checking the return value, there is a risk of null pointer references
> > oops.
> > 
> > Signed-off-by: Xiaoming Ni <nixiaoming@...wei.com>
> 
> We keep having this be "reported" :(
> 
> > ---
> >  drivers/tty/vt/vt.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index 34aa39d..db83e52 100644
> > --- a/drivers/tty/vt/vt.c
> > +++ b/drivers/tty/vt/vt.c
> > @@ -3357,15 +3357,33 @@ 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 (unlikely(!vc)) {
> > +			pr_warn("%s:failed to allocate memory for the %u vc\n",
> > +					__func__, currcons);
> > +			break;
> > +		}
> 
> At init, this really can not happen.  Have you see it ever happen?
> 
> >  		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 (unlikely(!vc->vc_screenbuf)) {
> 
> Never use likely/unlikely unless you can actually measure the speed
> difference.  For something like this, the compiler will always get it
> right without you having to do anything.
> 
> And again, how can this fail?  Have you seen it fail?
> 
> > +			pr_warn("%s:failed to allocate memory for the %u vc_screenbuf\n",
> > +					__func__, currcons);
> > +			visual_deinit(vc);
> > +			tty_port_destroy(&vc->port);
> > +			kfree(vc);
> > +			vc_cons[currcons].d = NULL;
> > +			break;
> > +		}
> >  		vc_init(vc, vc->vc_rows, vc->vc_cols,
> >  			currcons || !vc->vc_sw->con_save_screen);
> >  	}
> >  	currcons = fg_console = 0;
> >  	master_display_fg = vc = vc_cons[currcons].d;
> > +	if (unlikely(!vc)) {
> 
> Again, never use likely/unlikely unless you can measure it.
> 
> thanks,
> 
> greg k-h

Why does it use GFP_NOWAIT and not GFP_KERNEL? Is there some problem with 
GFP_KERNEL during initialization?

Mikulas