[PATCH v3] vt: Fix a missing-check bug in con_init()

[PATCH v3] vt: Fix a missing-check bug in con_init()

[Gen Zhang]

In function con_init(), the pointer variable vc_cons[currcons].d, vc and
vc->vc_screenbuf is allocated by kzalloc(). And they are used in the 
following codes. However, kzalloc() returns NULL when fails, and null 
pointer dereference may happen. And it will cause the kernel to crash. 
Therefore, we should check the return value and handle the error.

Further, since the allcoation is in a loop, we should free all the 
allocated memory in a loop.

Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
---
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index fdd12f8..d50f68f 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)
+			goto fail1;
 		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 fail2;
 		vc_init(vc, vc->vc_rows, vc->vc_cols,
 			currcons || !vc->vc_sw->con_save_screen);
 	}
@@ -3375,6 +3379,16 @@ static int __init con_init(void)
 	register_console(&vt_console_driver);
 #endif
 	return 0;
+fail1:
+	while (currcons > 0) {
+		currcons--;
+		kfree(vc_cons[currcons].d->vc_screenbuf);
+fail2:
+		kfree(vc_cons[currcons].d);
+		vc_cons[currcons].d = NULL;
+	}
+	console_unlock();
+	return -ENOMEM;
 }
 console_initcall(con_init);
 
---

Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

[Gen Zhang]

On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> vc->vc_screenbuf is allocated by kzalloc(). And they are used in the 
> following codes. However, kzalloc() returns NULL when fails, and null 
> pointer dereference may happen. And it will cause the kernel to crash. 
> Therefore, we should check the return value and handle the error.
> 
> Further, since the allcoation is in a loop, we should free all the 
> allocated memory in a loop.
> 
> Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> ---
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index fdd12f8..d50f68f 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)
> +			goto fail1;
>  		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 fail2;
>  		vc_init(vc, vc->vc_rows, vc->vc_cols,
>  			currcons || !vc->vc_sw->con_save_screen);
>  	}
> @@ -3375,6 +3379,16 @@ static int __init con_init(void)
>  	register_console(&vt_console_driver);
>  #endif
>  	return 0;
> +fail1:
> +	while (currcons > 0) {
> +		currcons--;
> +		kfree(vc_cons[currcons].d->vc_screenbuf);
> +fail2:
> +		kfree(vc_cons[currcons].d);
> +		vc_cons[currcons].d = NULL;
> +	}
> +	console_unlock();
> +	return -ENOMEM;
>  }
>  console_initcall(con_init);
>  
> ---
Can anyone look into this patch? It's already reviewed by Nicolas Pitre
<nico@fluxnic.net>.

Thanks
Gen

Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

[Greg KH]

On Sun, Jun 09, 2019 at 12:01:38AM +0800, Gen Zhang wrote:
> On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> > In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> > vc->vc_screenbuf is allocated by kzalloc(). And they are used in the 
> > following codes. However, kzalloc() returns NULL when fails, and null 
> > pointer dereference may happen. And it will cause the kernel to crash. 
> > Therefore, we should check the return value and handle the error.
> > 
> > Further, since the allcoation is in a loop, we should free all the 
> > allocated memory in a loop.
> > 
> > Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> > Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> > ---
> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index fdd12f8..d50f68f 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)
> > +			goto fail1;
> >  		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 fail2;
> >  		vc_init(vc, vc->vc_rows, vc->vc_cols,
> >  			currcons || !vc->vc_sw->con_save_screen);
> >  	}
> > @@ -3375,6 +3379,16 @@ static int __init con_init(void)
> >  	register_console(&vt_console_driver);
> >  #endif
> >  	return 0;
> > +fail1:
> > +	while (currcons > 0) {
> > +		currcons--;
> > +		kfree(vc_cons[currcons].d->vc_screenbuf);
> > +fail2:
> > +		kfree(vc_cons[currcons].d);
> > +		vc_cons[currcons].d = NULL;
> > +	}
> > +	console_unlock();
> > +	return -ENOMEM;
> >  }
> >  console_initcall(con_init);
> >  
> > ---
> Can anyone look into this patch? It's already reviewed by Nicolas Pitre
> <nico@fluxnic.net>.

It's in my queue.  But note, given the previous history of your patches,
it's really low on my piority list at the moment :(

greg k-h

Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

[Greg KH]

On Sun, Jun 09, 2019 at 12:01:38AM +0800, Gen Zhang wrote:
> On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> > In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> > vc->vc_screenbuf is allocated by kzalloc(). And they are used in the 
> > following codes. However, kzalloc() returns NULL when fails, and null 
> > pointer dereference may happen. And it will cause the kernel to crash. 
> > Therefore, we should check the return value and handle the error.
> > 
> > Further, since the allcoation is in a loop, we should free all the 
> > allocated memory in a loop.
> > 
> > Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> > Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> > ---
> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index fdd12f8..d50f68f 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)
> > +			goto fail1;
> >  		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 fail2;
> >  		vc_init(vc, vc->vc_rows, vc->vc_cols,
> >  			currcons || !vc->vc_sw->con_save_screen);
> >  	}
> > @@ -3375,6 +3379,16 @@ static int __init con_init(void)
> >  	register_console(&vt_console_driver);
> >  #endif
> >  	return 0;
> > +fail1:
> > +	while (currcons > 0) {
> > +		currcons--;
> > +		kfree(vc_cons[currcons].d->vc_screenbuf);
> > +fail2:
> > +		kfree(vc_cons[currcons].d);
> > +		vc_cons[currcons].d = NULL;
> > +	}

Wait, will that even work?  You can jump into the middle of a while
loop?

Ugh, that's beyond ugly.  And please provide "real" names for the
labels, "fail1" and "fail2" do not tell anything here.

Also, have you actually be able to trigger this?

thanks,

greg k-h

Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

[Gen Zhang]

On Sat, Jun 08, 2019 at 06:21:27PM +0200, Greg KH wrote:
> On Sun, Jun 09, 2019 at 12:01:38AM +0800, Gen Zhang wrote:
> > On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> > > In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> > > vc->vc_screenbuf is allocated by kzalloc(). And they are used in the 
> > > following codes. However, kzalloc() returns NULL when fails, and null 
> > > pointer dereference may happen. And it will cause the kernel to crash. 
> > > Therefore, we should check the return value and handle the error.
> > > 
> > > Further, since the allcoation is in a loop, we should free all the 
> > > allocated memory in a loop.
> > > 
> > > Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> > > Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> > > ---
> > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > > index fdd12f8..d50f68f 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)
> > > +			goto fail1;
> > >  		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 fail2;
> > >  		vc_init(vc, vc->vc_rows, vc->vc_cols,
> > >  			currcons || !vc->vc_sw->con_save_screen);
> > >  	}
> > > @@ -3375,6 +3379,16 @@ static int __init con_init(void)
> > >  	register_console(&vt_console_driver);
> > >  #endif
> > >  	return 0;
> > > +fail1:
> > > +	while (currcons > 0) {
> > > +		currcons--;
> > > +		kfree(vc_cons[currcons].d->vc_screenbuf);
> > > +fail2:
> > > +		kfree(vc_cons[currcons].d);
> > > +		vc_cons[currcons].d = NULL;
> > > +	}
> > > +	console_unlock();
> > > +	return -ENOMEM;
> > >  }
> > >  console_initcall(con_init);
> > >  
> > > ---
> > Can anyone look into this patch? It's already reviewed by Nicolas Pitre
> > <nico@fluxnic.net>.
> 
> It's in my queue.  But note, given the previous history of your patches,
> it's really low on my piority list at the moment :(
> 
> greg k-h
What? All the patches were revised iteratively according to the 
maintainers' or reviewers' advice. I don't think you should look down
the patches from me. It seems not fair enough. :(

Thanks
Gen

Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

[Gen Zhang]

On Sat, Jun 08, 2019 at 06:22:19PM +0200, Greg KH wrote:
> On Sun, Jun 09, 2019 at 12:01:38AM +0800, Gen Zhang wrote:
> > On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> > > In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> > > vc->vc_screenbuf is allocated by kzalloc(). And they are used in the 
> > > following codes. However, kzalloc() returns NULL when fails, and null 
> > > pointer dereference may happen. And it will cause the kernel to crash. 
> > > Therefore, we should check the return value and handle the error.
> > > 
> > > Further, since the allcoation is in a loop, we should free all the 
> > > allocated memory in a loop.
> > > 
> > > Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> > > Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> > > ---
> > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > > index fdd12f8..d50f68f 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)
> > > +			goto fail1;
> > >  		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 fail2;
> > >  		vc_init(vc, vc->vc_rows, vc->vc_cols,
> > >  			currcons || !vc->vc_sw->con_save_screen);
> > >  	}
> > > @@ -3375,6 +3379,16 @@ static int __init con_init(void)
> > >  	register_console(&vt_console_driver);
> > >  #endif
> > >  	return 0;
> > > +fail1:
> > > +	while (currcons > 0) {
> > > +		currcons--;
> > > +		kfree(vc_cons[currcons].d->vc_screenbuf);
> > > +fail2:
> > > +		kfree(vc_cons[currcons].d);
> > > +		vc_cons[currcons].d = NULL;
> > > +	}
> 
> Wait, will that even work?  You can jump into the middle of a while
> loop?
I felt like the same when I saw reviewer's advice to write in this way.
But I tested, and it did work.
> 
> Ugh, that's beyond ugly.  And please provide "real" names for the
> labels, "fail1" and "fail2" do not tell anything here.

Sure, I can revise that if needed.

Thanks
Gen
> 
> Also, have you actually be able to trigger this?
> 
> thanks,
> 
> greg k-h

Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

[Gen Zhang]

On Sat, Jun 08, 2019 at 06:21:27PM +0200, Greg KH wrote:
> On Sun, Jun 09, 2019 at 12:01:38AM +0800, Gen Zhang wrote:
> > On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> > > In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> > > vc->vc_screenbuf is allocated by kzalloc(). And they are used in the 
> > > following codes. However, kzalloc() returns NULL when fails, and null 
> > > pointer dereference may happen. And it will cause the kernel to crash. 
> > > Therefore, we should check the return value and handle the error.
> > > 
> > > Further, since the allcoation is in a loop, we should free all the 
> > > allocated memory in a loop.
> > > 
> > > Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> > > Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> > > ---
> > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > > index fdd12f8..d50f68f 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)
> > > +			goto fail1;
> > >  		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 fail2;
> > >  		vc_init(vc, vc->vc_rows, vc->vc_cols,
> > >  			currcons || !vc->vc_sw->con_save_screen);
> > >  	}
> > > @@ -3375,6 +3379,16 @@ static int __init con_init(void)
> > >  	register_console(&vt_console_driver);
> > >  #endif
> > >  	return 0;
> > > +fail1:
> > > +	while (currcons > 0) {
> > > +		currcons--;
> > > +		kfree(vc_cons[currcons].d->vc_screenbuf);
> > > +fail2:
> > > +		kfree(vc_cons[currcons].d);
> > > +		vc_cons[currcons].d = NULL;
> > > +	}
> > > +	console_unlock();
> > > +	return -ENOMEM;
> > >  }
> > >  console_initcall(con_init);
> > >  
> > > ---
> > Can anyone look into this patch? It's already reviewed by Nicolas Pitre
> > <nico@fluxnic.net>.
> 
> It's in my queue.  But note, given the previous history of your patches,
> it's really low on my piority list at the moment :(
> 
> greg k-h
Anyway, I should be honored to be remembered by Greg K-H. :-)

Thanks
Gen

Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

[Nicolas Pitre]

On Sat, 8 Jun 2019, Greg KH wrote:

> On Sun, Jun 09, 2019 at 12:01:38AM +0800, Gen Zhang wrote:
> > On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> > > In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> > > vc->vc_screenbuf is allocated by kzalloc(). And they are used in the 
> > > following codes. However, kzalloc() returns NULL when fails, and null 
> > > pointer dereference may happen. And it will cause the kernel to crash. 
> > > Therefore, we should check the return value and handle the error.
> > > 
> > > Further, since the allcoation is in a loop, we should free all the 
> > > allocated memory in a loop.
> > > 
> > > Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> > > Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> > > ---
> > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > > index fdd12f8..d50f68f 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)
> > > +			goto fail1;
> > >  		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 fail2;
> > >  		vc_init(vc, vc->vc_rows, vc->vc_cols,
> > >  			currcons || !vc->vc_sw->con_save_screen);
> > >  	}
> > > @@ -3375,6 +3379,16 @@ static int __init con_init(void)
> > >  	register_console(&vt_console_driver);
> > >  #endif
> > >  	return 0;
> > > +fail1:
> > > +	while (currcons > 0) {
> > > +		currcons--;
> > > +		kfree(vc_cons[currcons].d->vc_screenbuf);
> > > +fail2:
> > > +		kfree(vc_cons[currcons].d);
> > > +		vc_cons[currcons].d = NULL;
> > > +	}
> 
> Wait, will that even work?  You can jump into the middle of a while
> loop?

Absolutely.

> Ugh, that's beyond ugly.

That was me who suggested to do it like that. To me, this is nicer than 
the proposed alternatives. For an error path that is rather unlikely to 
happen, I think this is a very concise and eleguant way to do it.

> And please provide "real" names for the
> labels, "fail1" and "fail2" do not tell anything here.

That I agree with.


Nicolas

Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

[Gen Zhang]

On Sat, Jun 08, 2019 at 08:15:46PM -0400, Nicolas Pitre wrote:
> On Sat, 8 Jun 2019, Greg KH wrote:
> 
> > On Sun, Jun 09, 2019 at 12:01:38AM +0800, Gen Zhang wrote:
> > > On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> > > > In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> > > > vc->vc_screenbuf is allocated by kzalloc(). And they are used in the 
> > > > following codes. However, kzalloc() returns NULL when fails, and null 
> > > > pointer dereference may happen. And it will cause the kernel to crash. 
> > > > Therefore, we should check the return value and handle the error.
> > > > 
> > > > Further, since the allcoation is in a loop, we should free all the 
> > > > allocated memory in a loop.
> > > > 
> > > > Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> > > > Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> > > > ---
> > > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > > > index fdd12f8..d50f68f 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)
> > > > +			goto fail1;
> > > >  		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 fail2;
> > > >  		vc_init(vc, vc->vc_rows, vc->vc_cols,
> > > >  			currcons || !vc->vc_sw->con_save_screen);
> > > >  	}
> > > > @@ -3375,6 +3379,16 @@ static int __init con_init(void)
> > > >  	register_console(&vt_console_driver);
> > > >  #endif
> > > >  	return 0;
> > > > +fail1:
> > > > +	while (currcons > 0) {
> > > > +		currcons--;
> > > > +		kfree(vc_cons[currcons].d->vc_screenbuf);
> > > > +fail2:
> > > > +		kfree(vc_cons[currcons].d);
> > > > +		vc_cons[currcons].d = NULL;
> > > > +	}
> > 
> > Wait, will that even work?  You can jump into the middle of a while
> > loop?
> 
> Absolutely.
> 
> > Ugh, that's beyond ugly.
> 
> That was me who suggested to do it like that. To me, this is nicer than 
> the proposed alternatives. For an error path that is rather unlikely to 
> happen, I think this is a very concise and eleguant way to do it.
> 
> > And please provide "real" names for the
> > labels, "fail1" and "fail2" do not tell anything here.
> 
> That I agree with.
> 
> 
> Nicolas
Thanks for your comments. Then am I supposed to revise the patch and
send a v4 version?

Thanks
Gen

Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

[Jiri Slaby]

On 09. 06. 19, 2:15, Nicolas Pitre wrote:
>>>> +fail1:
>>>> +	while (currcons > 0) {
>>>> +		currcons--;
>>>> +		kfree(vc_cons[currcons].d->vc_screenbuf);
>>>> +fail2:
>>>> +		kfree(vc_cons[currcons].d);
>>>> +		vc_cons[currcons].d = NULL;
>>>> +	}
>>
>> Wait, will that even work?  You can jump into the middle of a while
>> loop?
> 
> Absolutely.

In C99, exceptions are only blocks with variable-sized declarations
(like "int a[b]").

thanks,
-- 
js
suse labs

Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

[Nicolas Pitre]

On Sun, 9 Jun 2019, Gen Zhang wrote:

> On Sat, Jun 08, 2019 at 08:15:46PM -0400, Nicolas Pitre wrote:
> > On Sat, 8 Jun 2019, Greg KH wrote:
> > 
> > > And please provide "real" names for the
> > > labels, "fail1" and "fail2" do not tell anything here.
> > 
> > That I agree with.
> > 
> > 
> > Nicolas
> Thanks for your comments. Then am I supposed to revise the patch and
> send a v4 version?

Yes.


Nicolas

[PATCH v3] vt: Fix a missing-check bug in con_init()

[Gen Zhang]

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, since the allcoation is in a loop, we should free all the 
allocated memory in a loop.

Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
---
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index fdd12f8..d50f68f 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)
+			goto fail1;
 		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 fail2;
 		vc_init(vc, vc->vc_rows, vc->vc_cols,
 			currcons || !vc->vc_sw->con_save_screen);
 	}
@@ -3375,6 +3379,16 @@ static int __init con_init(void)
 	register_console(&vt_console_driver);
 #endif
 	return 0;
+fail1:
+	while (currcons > 0) {
+		currcons--;
+		kfree(vc_cons[currcons].d->vc_screenbuf);
+fail2:
+		kfree(vc_cons[currcons].d);
+		vc_cons[currcons].d = NULL;
+	}
+	console_unlock();
+	return -ENOMEM;
 }
 console_initcall(con_init);
 
---