Re: [PATCH] vt: Fix a missing-check bug in drivers/tty/vt/vt.c file of Linux 5.0.14

Re: [PATCH] vt: Fix a missing-check bug in drivers/tty/vt/vt.c file of Linux 5.0.14

[Greg KH]

On Fri, May 10, 2019 at 10:24:50PM +0800, Gen Zhang wrote:
> On Fri, May 10, 2019 at 13:14:02PM +0800, Greg KH <
> gregkh@linuxfoundation.org> wrote:
> >Note, your email client ate all of the tabs and made the patch
> >impossible to apply, so please fix this up before you resend it.
> >
> >thanks,
> >
> >greg k-h
> From: Gen Zhang <blackgod016574@gmail.com>
> Date: Fri, 10 May 2019 09:31:30 +0000
> Subject: [PATCH] vt: Fix a missing-check bug in drivers/tty/vt/vt.c file of
> Linux 5.0.14
> 
> Hi,
> I found this missing-check bug in Linux-5.0.14/drivers/tty/vt/vt.c when I
> was examining the source code.
> 
> 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  be failed.
> 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 an error.
> 
> Below is the patch file, and I am ready to sumbit it to the kernel tree.
> I am looking forward to a reply on this, thank you!
> 
> Kind regards
> Gen
> 
> Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> ---
> 
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -3322,10 +3322,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)
> + 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);
>   }
> @@ -3347,6 +3351,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;
>  }
>  console_initcall(con_init);

Still impossible to apply :(

Also, what about Dave's response to you?  This really can never be hit,
like other early-init tty allocations that we do not check because of
this issue, correct?

thanks,

greg k-h

Re: [PATCH] vt: Fix a missing-check bug in drivers/tty/vt/vt.c file of Linux 5.0.14

[Greg KH]

On Sat, May 11, 2019 at 09:21:39AM +0800, Gen Zhang wrote:
> On Fri, May 10, 2019 at 11:12:50PM +0800, Greg KH <
> gregkh@linuxfoundation.org> wrote:
> >Still impossible to apply :(
> >
> >Also, what about Dave's response to you?  This really can never be hit,
> >like other early-init tty allocations that we do not check because of
> >this issue, correct?
> >
> >thanks,
> >
> >greg k-h
> 1. Cannot imply the patch
> I pulled the latest kernel from github(commit
> 1fb3b526df3bd7647e7854915ae6b22299408baf), and patched with
> **************************************
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> 
> @@ -3322,10 +3322,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)
> + 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);
>   }
> @@ -3347,6 +3351,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;
>  }
>  console_initcall(con_init);
> 
> 
> **************************************
> (It is possible that you missed the last line?)

Look at the patch above, all of the whitespace is damaged.  There is no
way you took the raw email and then were able to apply that to the
kernel tree.

You can not cut/paste patches into gmail, please read the kernel
Documentation file all about email clients and how to get them to work
properly to send patches.

> 2. David's response
> In my humble opinion, whatever the cause is, theoratically, there is a
> possibility that memory allocation (e.g. kzalloc()) can be failed.
> I don't think it is related to whether we are in the early-initial stage or
> not.

But it is directly related.

> Once the allocated pointer (e.g. vc) is deferenced, the kernel might go
> wrong.
> And in this case, variable vc_cons[currcons].d, vc and vc->vc_screenbuf is
> deferenced after allocation.
> Thus I think we should add the allocation check to prevent null pointer
> deference.

For most problems, yes, if you can successfully unwind and continue on
with a working system.  Will that happen here?

thanks,

greg k-h

Re: [PATCH] vt: Fix a missing-check bug in drivers/tty/vt/vt.c file of Linux 5.0.14

[Gen Zhang]

On Sat, May 11, 2019 at 08:07:41AM +0200, Greg KH wrote:
> Look at the patch above, all of the whitespace is damaged.  There is no
> way you took the raw email and then were able to apply that to the
> kernel tree.
> 
> You can not cut/paste patches into gmail, please read the kernel
> Documentation file all about email clients and how to get them to work
> properly to send patches.
Hi Greg,
I switched to mutt and get rid of cut/paste.
I patched it successffully with commit 1fb3b526df3bd7647e7854915ae6b22299408baf.
The patch file is:
---
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)
+			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;
 }
 console_initcall(con_init);
 
 ---
I hope that the format is not broken any more.
As for whether the patch should be applied, it is totally your call.
Anyway, thanks for your patient reply!
Gen

Re: [PATCH] vt: Fix a missing-check bug in drivers/tty/vt/vt.c file of Linux 5.0.14

[Greg KH]

On Sun, May 12, 2019 at 11:27:19AM +0800, Gen Zhang wrote:
> On Sat, May 11, 2019 at 08:07:41AM +0200, Greg KH wrote:
> > Look at the patch above, all of the whitespace is damaged.  There is no
> > way you took the raw email and then were able to apply that to the
> > kernel tree.
> > 
> > You can not cut/paste patches into gmail, please read the kernel
> > Documentation file all about email clients and how to get them to work
> > properly to send patches.
> Hi Greg,
> I switched to mutt and get rid of cut/paste.
> I patched it successffully with commit 1fb3b526df3bd7647e7854915ae6b22299408baf.
> The patch file is:
> ---
> 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)
> +			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;
>  }
>  console_initcall(con_init);
>  
>  ---
> I hope that the format is not broken any more.

Yes, that worked!  Now, can you resend it in a proper format that I can
apply it in?  (with changelog text, signed-off-by, etc.) as described in
Documentation/SubmittingPatches, I will be glad to review it after the
5.2-rc1 release happens.

thanks,

greg k-h

Re: [PATCH] vt: Fix a missing-check bug in drivers/tty/vt/vt.c file of Linux 5.0.14

[Gen Zhang]

On Sun, May 12, 2019 at 08:20:09AM +0200, Greg KH wrote:
> Yes, that worked!  Now, can you resend it in a proper format that I can
> apply it in?  (with changelog text, signed-off-by, etc.) as described in
> Documentation/SubmittingPatches, I will be glad to review it after the
> 5.2-rc1 release happens.
> 
> thanks,
> 
> greg k-h
From: Gen Zhang <blackgod016574@gmail.com>
Date: Sun, 11 May 2019 15:31:30 +0000
Subject: [PATCH] vt: Fix a missing-check bug in drivers/tty/vt/vt.c file of Linux 5.0.14

Hi,
I found this missing-check bug in drivers/tty/vt/vt.c when I was examining the source code. 

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  be failed. 
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 an error.

And this patch works in 5.1.1.

Thank you!

Kind regards
Gen

Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
---
--- drivers/tty/vt/vt.c
+++ drivers/tty/vt/vt.c
@@ -3349,10 +3349,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)
+			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);
 	}
@@ -3374,6 +3378,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;
 }
 console_initcall(con_init);
 
---

Re: [PATCH] vt: Fix a missing-check bug in drivers/tty/vt/vt.c file of Linux 5.0.14

[Greg KH]

On Sun, May 12, 2019 at 04:49:39PM +0800, Gen Zhang wrote:
> On Sun, May 12, 2019 at 08:20:09AM +0200, Greg KH wrote:
> > Yes, that worked!  Now, can you resend it in a proper format that I can
> > apply it in?  (with changelog text, signed-off-by, etc.) as described in
> > Documentation/SubmittingPatches, I will be glad to review it after the
> > 5.2-rc1 release happens.
> > 
> > thanks,
> > 
> > greg k-h
> From: Gen Zhang <blackgod016574@gmail.com>
> Date: Sun, 11 May 2019 15:31:30 +0000
> Subject: [PATCH] vt: Fix a missing-check bug in drivers/tty/vt/vt.c file of Linux 5.0.14

Better, but no need for this to be in the body, just send it like any
other patch on the mailing list.

> 
> Hi,
> I found this missing-check bug in drivers/tty/vt/vt.c when I was examining the source code. 

That doesn't need to be in the changelog text.

> 
> 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. 

Properly wrap your lines at 72 columns please.

> 
> However, when there is a memory allocation error, kzalloc can  be failed. 
> 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 an error.
> 
> And this patch works in 5.1.1.

No need to say that.

> 
> Thank you!
> 
> Kind regards
> Gen

Or that :)


> 
> Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> ---
> --- drivers/tty/vt/vt.c
> +++ drivers/tty/vt/vt.c
> @@ -3349,10 +3349,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)
> +			goto err_vc;

What about the other memory that was allocated?  You never free that.

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

Same here, you are now leaking memory.

Did you test this patch out with a kmalloc function that can fail?  If
not, please try to do so.

thanks,

greg k-h

Re: [PATCH] vt: Fix a missing-check bug in drivers/tty/vt/vt.c file of Linux 5.0.14

[Gen Zhang]

On Mon, May 13, 2019 at 09:36:19AM +0200, Greg KH wrote:
> > Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> > ---
> > --- drivers/tty/vt/vt.c
> > +++ drivers/tty/vt/vt.c
> > @@ -3349,10 +3349,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)
> > +			goto err_vc;
> 
> What about the other memory that was allocated?  You never free that.
> 
> >  		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;
> 
> Same here, you are now leaking memory.
> 
> Did you test this patch out with a kmalloc function that can fail?  If
> not, please try to do so.
> 
> thanks,
> 
> greg k-h
Hi, Greg
1. I re-examined the source code.
For vc_cons[currcons].d and vc allocation fail, we may need to free
vc->vc_screenbuf from the previous loop. So kfree(vc->vc_screenbuf) 
need to be added to err_vc;
As for vc->vc_screenbuf allocation fail, I don't think there is other
memory need to be freed. Because in function con_init, there's no other 
allocation operations except this two kzalloc functions. And in
err_vc_screenbuf, vc_cons[currcons].d and vc is freed in the patch.

2. I tried to test this patch with a compiled kernel in QEMU but 
failed. Testing this is out of my skills. So is there any other ways
to test this patch?
Thanks
Gen

Re: [PATCH] vt: Fix a missing-check bug in drivers/tty/vt/vt.c file of Linux 5.0.14

[Greg KH]

On Mon, May 13, 2019 at 05:37:41PM +0800, Gen Zhang wrote:
> On Mon, May 13, 2019 at 09:36:19AM +0200, Greg KH wrote:
> > > Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> > > ---
> > > --- drivers/tty/vt/vt.c
> > > +++ drivers/tty/vt/vt.c
> > > @@ -3349,10 +3349,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)
> > > +			goto err_vc;
> > 
> > What about the other memory that was allocated?  You never free that.
> > 
> > >  		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;
> > 
> > Same here, you are now leaking memory.
> > 
> > Did you test this patch out with a kmalloc function that can fail?  If
> > not, please try to do so.
> > 
> > thanks,
> > 
> > greg k-h
> Hi, Greg
> 1. I re-examined the source code.
> For vc_cons[currcons].d and vc allocation fail, we may need to free
> vc->vc_screenbuf from the previous loop. So kfree(vc->vc_screenbuf) 
> need to be added to err_vc;
> As for vc->vc_screenbuf allocation fail, I don't think there is other
> memory need to be freed. Because in function con_init, there's no other 
> allocation operations except this two kzalloc functions. And in
> err_vc_screenbuf, vc_cons[currcons].d and vc is freed in the patch.

You have to unwind the loop and free and uninitialize all of the other
things you just created as well.

> 2. I tried to test this patch with a compiled kernel in QEMU but 
> failed. Testing this is out of my skills. So is there any other ways
> to test this patch?

qemu should work just fine, I don't know what else to suggest.  Run it
on "real hardware" with a kmalloc function modified to fail this
allocation?

good luck!

greg k-h

Re: [PATCH] vt: Fix a missing-check bug in drivers/tty/vt/vt.c file of Linux 5.0.14

[Gen Zhang]

On Mon, May 13, 2019 at 11:58:09AM +0200, Greg KH wrote:
 
> You have to unwind the loop and free and uninitialize all of the other
> things you just created as well.
Hi Greg,
I don't think we need to unwind the loop. The loop condition 
MIN_NR_CONSOLES is defined as 1 in include/uapi/linux/vt.h. In this
situation, should we free other memory except vc_cons[currcons].d, vc
and vc->vc_screenbuf?
Thanks
Gen

Re: [PATCH] vt: Fix a missing-check bug in drivers/tty/vt/vt.c file of Linux 5.0.14

[Gen Zhang]

On Mon, May 13, 2019 at 11:58:09AM +0200, Greg KH wrote:
> qemu should work just fine, I don't know what else to suggest.  Run it
> on "real hardware" with a kmalloc function modified to fail this
> allocation?
> 
> good luck!
> 
> greg k-h
I don't think we need to unwind the loop. The loop condition 
MIN_NR_CONSOLES is defined as 1 in include/uapi/linux/vt.h. In this
situation, should we free other memory except vc_cons[currcons].d, vc
and vc->vc_screenbuf?
Thanks
Gen