* [PATCH v2] vt: Fix a missing-check bug in drivers/tty/vt/vt.c
@ 2019-05-21 2:29 Gen Zhang
2019-05-21 2:55 ` Nicolas Pitre
0 siblings, 1 reply; 14+ messages in thread
From: Gen Zhang @ 2019-05-21 2:29 UTC (permalink / raw)
To: nico; +Cc: linux-kernel
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.
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)
+ 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);
---
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] vt: Fix a missing-check bug in drivers/tty/vt/vt.c
2019-05-21 2:29 [PATCH v2] vt: Fix a missing-check bug in drivers/tty/vt/vt.c Gen Zhang
@ 2019-05-21 2:55 ` Nicolas Pitre
2019-05-21 3:09 ` Gen Zhang
2019-05-21 3:21 ` [PATCH v3] vt: Fix a missing-check bug in drivers/tty/vt/vt.c Gen Zhang
0 siblings, 2 replies; 14+ messages in thread
From: Nicolas Pitre @ 2019-05-21 2:55 UTC (permalink / raw)
To: Gen Zhang; +Cc: linux-kernel
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);
>
> ---
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] vt: Fix a missing-check bug in drivers/tty/vt/vt.c
2019-05-21 2:55 ` Nicolas Pitre
@ 2019-05-21 3:09 ` Gen Zhang
2019-05-21 3:26 ` Nicolas Pitre
2019-05-21 3:21 ` [PATCH v3] vt: Fix a missing-check bug in drivers/tty/vt/vt.c Gen Zhang
1 sibling, 1 reply; 14+ messages in thread
From: Gen Zhang @ 2019-05-21 3:09 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: linux-kernel
On Mon, May 20, 2019 at 10:55:40PM -0400, Nicolas Pitre wrote:
> 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.
I re-examine the source code. MIN_NR_CONSOLES is only defined once and
no other changes to it.
>
> > 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.
Thanks for this comment!
>
> > + 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;
>
>
Thanks for your patient reply, Nicolas!
I will work on this patch and resubmit it.
Thanks
Gen
> > }
> > console_initcall(con_init);
> >
> > ---
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] vt: Fix a missing-check bug in drivers/tty/vt/vt.c
2019-05-21 2:55 ` Nicolas Pitre
2019-05-21 3:09 ` Gen Zhang
@ 2019-05-21 3:21 ` Gen Zhang
2019-05-21 6:46 ` Oleksandr Natalenko
1 sibling, 1 reply; 14+ messages in thread
From: Gen Zhang @ 2019-05-21 3:21 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: linux-kernel
On Mon, May 20, 2019 at 10:55:40PM -0400, Nicolas Pitre wrote:
> 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;
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 and it is not changed. So there is no need to
unwind the loop.
Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
---
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index fdd12f8..ea47eb3 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 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,13 @@ static int __init con_init(void)
register_console(&vt_console_driver);
#endif
return 0;
+err_vc_screenbuf:
+ kfree(vc);
+ vc_cons[currcons].d = NULL;
+err_vc:
+ console_unlock();
+ return -ENOMEM;
+
}
console_initcall(con_init);
---
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] vt: Fix a missing-check bug in drivers/tty/vt/vt.c
2019-05-21 3:09 ` Gen Zhang
@ 2019-05-21 3:26 ` Nicolas Pitre
2019-05-21 4:00 ` Gen Zhang
0 siblings, 1 reply; 14+ messages in thread
From: Nicolas Pitre @ 2019-05-21 3:26 UTC (permalink / raw)
To: Gen Zhang; +Cc: linux-kernel
On Tue, 21 May 2019, Gen Zhang wrote:
> On Mon, May 20, 2019 at 10:55:40PM -0400, Nicolas Pitre wrote:
> > 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.
> I re-examine the source code. MIN_NR_CONSOLES is only defined once and
> no other changes to it.
Yes, that is true today. But if someone changes that in the future, how
will that person know that you relied on it to be 1 for not needing to
unwind the loop?
Nicolas
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] vt: Fix a missing-check bug in drivers/tty/vt/vt.c
2019-05-21 3:26 ` Nicolas Pitre
@ 2019-05-21 4:00 ` Gen Zhang
2019-05-21 4:30 ` Nicolas Pitre
0 siblings, 1 reply; 14+ messages in thread
From: Gen Zhang @ 2019-05-21 4:00 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: linux-kernel
On Mon, May 20, 2019 at 11:26:20PM -0400, Nicolas Pitre wrote:
> On Tue, 21 May 2019, Gen Zhang wrote:
>
> > On Mon, May 20, 2019 at 10:55:40PM -0400, Nicolas Pitre wrote:
> > > 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.
> > I re-examine the source code. MIN_NR_CONSOLES is only defined once and
> > no other changes to it.
>
> Yes, that is true today. But if someone changes that in the future, how
> will that person know that you relied on it to be 1 for not needing to
> unwind the loop?
>
>
> Nicolas
Hi Nicolas,
Thanks for your explaination! And I got your point. And is this way
proper?
err_vc_screenbuf:
kfree(vc);
for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++)
vc_cons[currcons].d = NULL;
return -ENOMEM;
err_vc:
console_unlock();
return -ENOMEM;
Thanks
Gen
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] vt: Fix a missing-check bug in drivers/tty/vt/vt.c
2019-05-21 4:00 ` Gen Zhang
@ 2019-05-21 4:30 ` Nicolas Pitre
2019-05-21 7:39 ` Gen Zhang
0 siblings, 1 reply; 14+ messages in thread
From: Nicolas Pitre @ 2019-05-21 4:30 UTC (permalink / raw)
To: Gen Zhang; +Cc: linux-kernel
On Tue, 21 May 2019, Gen Zhang wrote:
> On Mon, May 20, 2019 at 11:26:20PM -0400, Nicolas Pitre wrote:
> > On Tue, 21 May 2019, Gen Zhang wrote:
> >
> > > On Mon, May 20, 2019 at 10:55:40PM -0400, Nicolas Pitre wrote:
> > > > 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.
> > > I re-examine the source code. MIN_NR_CONSOLES is only defined once and
> > > no other changes to it.
> >
> > Yes, that is true today. But if someone changes that in the future, how
> > will that person know that you relied on it to be 1 for not needing to
> > unwind the loop?
> >
> >
> > Nicolas
> Hi Nicolas,
> Thanks for your explaination! And I got your point. And is this way
> proper?
Not quite.
> err_vc_screenbuf:
> kfree(vc);
> for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++)
> vc_cons[currcons].d = NULL;
> return -ENOMEM;
> err_vc:
> console_unlock();
> return -ENOMEM;
Now imagine that MIN_NR_CONSOLES is defined to 10 instead of 1.
What happens with allocated memory if the err_vc condition is met on the
5th loop?
If err_vc_screenbuf condition is encountered on the 5th loop (curcons =
4), what is the value of vc_cons[4].d? Isn't it the same as vc that you
just freed?
Nicolas
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] vt: Fix a missing-check bug in drivers/tty/vt/vt.c
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
0 siblings, 0 replies; 14+ messages in thread
From: Oleksandr Natalenko @ 2019-05-21 6:46 UTC (permalink / raw)
To: Gen Zhang; +Cc: Nicolas Pitre, linux-kernel, Grzegorz Halat
Cc'ing Grzegorz.
On Tue, May 21, 2019 at 11:21:18AM +0800, Gen Zhang wrote:
> On Mon, May 20, 2019 at 10:55:40PM -0400, Nicolas Pitre wrote:
> > 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;
> 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 and it is not changed. So there is no need to
> unwind the loop.
>
> Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
>
> ---
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index fdd12f8..ea47eb3 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 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,13 @@ static int __init con_init(void)
> register_console(&vt_console_driver);
> #endif
> return 0;
> +err_vc_screenbuf:
> + kfree(vc);
> + vc_cons[currcons].d = NULL;
> +err_vc:
> + console_unlock();
> + return -ENOMEM;
> +
> }
> console_initcall(con_init);
> ---
--
Best regards,
Oleksandr Natalenko (post-factum)
Senior Software Maintenance Engineer
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] vt: Fix a missing-check bug in drivers/tty/vt/vt.c
2019-05-21 4:30 ` Nicolas Pitre
@ 2019-05-21 7:39 ` Gen Zhang
2019-05-22 2:43 ` Nicolas Pitre
0 siblings, 1 reply; 14+ messages in thread
From: Gen Zhang @ 2019-05-21 7:39 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: linux-kernel
On Tue, May 21, 2019 at 12:30:38AM -0400, Nicolas Pitre wrote:
> Now imagine that MIN_NR_CONSOLES is defined to 10 instead of 1.
>
> What happens with allocated memory if the err_vc condition is met on the
> 5th loop?
Yes, vc->vc_screenbuf from the last loop is still not freed, right? I
don't have idea to solve this one. Could please give some advice? Since
we have to consider the err_vc condition.
> If err_vc_screenbuf condition is encountered on the 5th loop (curcons =
> 4), what is the value of vc_cons[4].d? Isn't it the same as vc that you
> just freed?
>
>
> Nicolas
Thanks for your explaination! You mean a double free situation may
happen, right? But in vc_allocate() there is also such a kfree(vc) and
vc_cons[currcons].d = NULL operation. This situation is really confusing
me.
Thanks
Gen
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] vt: Fix a missing-check bug in drivers/tty/vt/vt.c
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
0 siblings, 2 replies; 14+ messages in thread
From: Nicolas Pitre @ 2019-05-22 2:43 UTC (permalink / raw)
To: Gen Zhang; +Cc: linux-kernel
On Tue, 21 May 2019, Gen Zhang wrote:
> On Tue, May 21, 2019 at 12:30:38AM -0400, Nicolas Pitre wrote:
> > Now imagine that MIN_NR_CONSOLES is defined to 10 instead of 1.
> >
> > What happens with allocated memory if the err_vc condition is met on the
> > 5th loop?
> Yes, vc->vc_screenbuf from the last loop is still not freed, right? I
> don't have idea to solve this one. Could please give some advice? Since
> we have to consider the err_vc condition.
>
> > If err_vc_screenbuf condition is encountered on the 5th loop (curcons =
> > 4), what is the value of vc_cons[4].d? Isn't it the same as vc that you
> > just freed?
> >
> >
> > Nicolas
> Thanks for your explaination! You mean a double free situation may
> happen, right? But in vc_allocate() there is also such a kfree(vc) and
> vc_cons[currcons].d = NULL operation. This situation is really confusing
> me.
What you could do is something that looks like:
for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
vc_cons[currcons].d = vc = kzalloc(...);
if (!vc)
goto fail1;
...
vc->vc_screenbuf = kzalloc(...);
if (!vc->vc_screenbuf)
goto fail2;
...
return 0;
/* free already allocated memory on error */
fail1:
while (curcons > 0) {
curcons--;
kfree(vc_cons[currcons].d->vc_screenbuf);
fail2:
kfree(vc_cons[currcons].d);
vc_cons[currcons].d = NULL;
}
console_unlock();
return -ENOMEM;
Nicolas
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] vt: Fix a missing-check bug in drivers/tty/vt/vt.c
2019-05-22 2:43 ` Nicolas Pitre
@ 2019-05-22 8:10 ` Gen Zhang
2019-05-22 12:19 ` [PATCH v3] " Gen Zhang
1 sibling, 0 replies; 14+ messages in thread
From: Gen Zhang @ 2019-05-22 8:10 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: linux-kernel
On Tue, May 21, 2019 at 10:43:11PM -0400, Nicolas Pitre wrote:
> On Tue, 21 May 2019, Gen Zhang wrote:
>
> > On Tue, May 21, 2019 at 12:30:38AM -0400, Nicolas Pitre wrote:
> > > Now imagine that MIN_NR_CONSOLES is defined to 10 instead of 1.
> > >
> > > What happens with allocated memory if the err_vc condition is met on the
> > > 5th loop?
> > Yes, vc->vc_screenbuf from the last loop is still not freed, right? I
> > don't have idea to solve this one. Could please give some advice? Since
> > we have to consider the err_vc condition.
> >
> > > If err_vc_screenbuf condition is encountered on the 5th loop (curcons =
> > > 4), what is the value of vc_cons[4].d? Isn't it the same as vc that you
> > > just freed?
> > >
> > >
> > > Nicolas
> > Thanks for your explaination! You mean a double free situation may
> > happen, right? But in vc_allocate() there is also such a kfree(vc) and
> > vc_cons[currcons].d = NULL operation. This situation is really confusing
> > me.
>
> What you could do is something that looks like:
>
> for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
> vc_cons[currcons].d = vc = kzalloc(...);
> if (!vc)
> goto fail1;
> ...
> vc->vc_screenbuf = kzalloc(...);
> if (!vc->vc_screenbuf)
> goto fail2;
> ...
>
> return 0;
>
> /* free already allocated memory on error */
> fail1:
> while (curcons > 0) {
> curcons--;
> kfree(vc_cons[currcons].d->vc_screenbuf);
> fail2:
> kfree(vc_cons[currcons].d);
> vc_cons[currcons].d = NULL;
> }
> console_unlock();
> return -ENOMEM;
>
>
> Nicolas
Thanks for your patient explaination, Nicolas!
I will work on it and resubmit it.
Thanks
Gen
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3] vt: Fix a missing-check bug in drivers/tty/vt/vt.c
2019-05-22 2:43 ` Nicolas Pitre
2019-05-22 8:10 ` Gen Zhang
@ 2019-05-22 12:19 ` Gen Zhang
2019-05-22 14:18 ` Nicolas Pitre
1 sibling, 1 reply; 14+ messages in thread
From: Gen Zhang @ 2019-05-22 12:19 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: linux-kernel
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>
---
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);
---
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3] vt: Fix a missing-check bug in drivers/tty/vt/vt.c
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
0 siblings, 1 reply; 14+ messages in thread
From: Nicolas Pitre @ 2019-05-22 14:18 UTC (permalink / raw)
To: Gen Zhang; +Cc: linux-kernel
On Wed, 22 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, 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);
>
> ---
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3] vt: Fix a missing-check bug in con_init()
2019-05-22 14:18 ` Nicolas Pitre
@ 2019-05-24 2:27 ` Gen Zhang
0 siblings, 0 replies; 14+ messages in thread
From: Gen Zhang @ 2019-05-24 2:27 UTC (permalink / raw)
To: jslaby; +Cc: mpatocka, linux-kernel
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);
---
^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-05-24 2:28 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-21 2:29 [PATCH v2] vt: Fix a missing-check bug in drivers/tty/vt/vt.c Gen Zhang
2019-05-21 2:55 ` Nicolas Pitre
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
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).