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 <
> [email protected]> 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
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
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
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 <[email protected]>
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 <[email protected]>
---
--- 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);
---
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 <[email protected]>
> 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 <[email protected]>
> ---
> --- 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
On Mon, May 13, 2019 at 09:36:19AM +0200, Greg KH wrote:
> > Signed-off-by: Gen Zhang <[email protected]>
> > ---
> > --- 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
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 <[email protected]>
> > > ---
> > > --- 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
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
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