* [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; 25+ 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] 25+ 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; 25+ 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] 25+ 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; 25+ 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] 25+ 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; 25+ 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] 25+ 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; 25+ 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] 25+ 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; 25+ 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] 25+ 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; 25+ 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] 25+ 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; 25+ 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] 25+ 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; 25+ 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] 25+ 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; 25+ 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] 25+ 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; 25+ 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] 25+ 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; 25+ 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] 25+ 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; 25+ 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] 25+ 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; 25+ 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] 25+ messages in thread
* [PATCH v3] vt: Fix a missing-check bug in con_init()
@ 2019-05-28 0:45 Gen Zhang
2019-06-08 16:01 ` Gen Zhang
0 siblings, 1 reply; 25+ messages in thread
From: Gen Zhang @ 2019-05-28 0:45 UTC (permalink / raw)
To: gregkh, jslaby, nico, kilobyte, textshell, mpatocka, daniel.vetter
Cc: linux-kernel
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);
---
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v3] vt: Fix a missing-check bug in con_init()
2019-05-28 0:45 [PATCH v3] vt: Fix a missing-check bug in con_init() Gen Zhang
@ 2019-06-08 16:01 ` Gen Zhang
2019-06-08 16:21 ` Greg KH
2019-06-08 16:22 ` Greg KH
0 siblings, 2 replies; 25+ messages in thread
From: Gen Zhang @ 2019-06-08 16:01 UTC (permalink / raw)
To: gregkh, jslaby, nico, kilobyte, textshell, mpatocka, daniel.vetter
Cc: linux-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3] vt: Fix a missing-check bug in con_init()
2019-06-08 16:01 ` Gen Zhang
@ 2019-06-08 16:21 ` Greg KH
2019-06-08 16:27 ` Gen Zhang
2019-06-08 16:34 ` Gen Zhang
2019-06-08 16:22 ` Greg KH
1 sibling, 2 replies; 25+ messages in thread
From: Greg KH @ 2019-06-08 16:21 UTC (permalink / raw)
To: Gen Zhang
Cc: jslaby, nico, kilobyte, textshell, mpatocka, daniel.vetter, linux-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3] vt: Fix a missing-check bug in con_init()
2019-06-08 16:21 ` Greg KH
@ 2019-06-08 16:27 ` Gen Zhang
2019-06-08 16:34 ` Gen Zhang
1 sibling, 0 replies; 25+ messages in thread
From: Gen Zhang @ 2019-06-08 16:27 UTC (permalink / raw)
To: Greg KH
Cc: jslaby, nico, kilobyte, textshell, mpatocka, daniel.vetter, linux-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3] vt: Fix a missing-check bug in con_init()
2019-06-08 16:21 ` Greg KH
2019-06-08 16:27 ` Gen Zhang
@ 2019-06-08 16:34 ` Gen Zhang
1 sibling, 0 replies; 25+ messages in thread
From: Gen Zhang @ 2019-06-08 16:34 UTC (permalink / raw)
To: Greg KH
Cc: jslaby, nico, kilobyte, textshell, mpatocka, daniel.vetter, linux-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3] vt: Fix a missing-check bug in con_init()
2019-06-08 16:01 ` Gen Zhang
2019-06-08 16:21 ` Greg KH
@ 2019-06-08 16:22 ` Greg KH
2019-06-08 16:30 ` Gen Zhang
2019-06-09 0:15 ` Nicolas Pitre
1 sibling, 2 replies; 25+ messages in thread
From: Greg KH @ 2019-06-08 16:22 UTC (permalink / raw)
To: Gen Zhang
Cc: jslaby, nico, kilobyte, textshell, mpatocka, daniel.vetter, linux-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3] vt: Fix a missing-check bug in con_init()
2019-06-08 16:22 ` Greg KH
@ 2019-06-08 16:30 ` Gen Zhang
2019-06-09 0:15 ` Nicolas Pitre
1 sibling, 0 replies; 25+ messages in thread
From: Gen Zhang @ 2019-06-08 16:30 UTC (permalink / raw)
To: Greg KH
Cc: jslaby, nico, kilobyte, textshell, mpatocka, daniel.vetter, linux-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3] vt: Fix a missing-check bug in con_init()
2019-06-08 16:22 ` Greg KH
2019-06-08 16:30 ` Gen Zhang
@ 2019-06-09 0:15 ` Nicolas Pitre
2019-06-10 6:45 ` Gen Zhang
2019-06-10 7:10 ` Jiri Slaby
1 sibling, 2 replies; 25+ messages in thread
From: Nicolas Pitre @ 2019-06-09 0:15 UTC (permalink / raw)
To: Greg KH
Cc: Gen Zhang, jslaby, kilobyte, textshell, mpatocka, daniel.vetter,
linux-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3] vt: Fix a missing-check bug in con_init()
2019-06-09 0:15 ` Nicolas Pitre
@ 2019-06-10 6:45 ` Gen Zhang
2019-06-10 14:31 ` Nicolas Pitre
2019-06-10 7:10 ` Jiri Slaby
1 sibling, 1 reply; 25+ messages in thread
From: Gen Zhang @ 2019-06-10 6:45 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Greg KH, jslaby, kilobyte, textshell, mpatocka, daniel.vetter,
linux-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3] vt: Fix a missing-check bug in con_init()
2019-06-10 6:45 ` Gen Zhang
@ 2019-06-10 14:31 ` Nicolas Pitre
0 siblings, 0 replies; 25+ messages in thread
From: Nicolas Pitre @ 2019-06-10 14:31 UTC (permalink / raw)
To: Gen Zhang
Cc: Greg KH, jslaby, kilobyte, textshell, mpatocka, daniel.vetter,
linux-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3] vt: Fix a missing-check bug in con_init()
2019-06-09 0:15 ` Nicolas Pitre
2019-06-10 6:45 ` Gen Zhang
@ 2019-06-10 7:10 ` Jiri Slaby
1 sibling, 0 replies; 25+ messages in thread
From: Jiri Slaby @ 2019-06-10 7:10 UTC (permalink / raw)
To: Nicolas Pitre, Greg KH
Cc: kilobyte, daniel.vetter, Gen Zhang, mpatocka, textshell, linux-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2019-06-10 14:31 UTC | newest]
Thread overview: 25+ 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
2019-05-28 0:45 [PATCH v3] vt: Fix a missing-check bug in con_init() Gen Zhang
2019-06-08 16:01 ` Gen Zhang
2019-06-08 16:21 ` Greg KH
2019-06-08 16:27 ` Gen Zhang
2019-06-08 16:34 ` Gen Zhang
2019-06-08 16:22 ` Greg KH
2019-06-08 16:30 ` Gen Zhang
2019-06-09 0:15 ` Nicolas Pitre
2019-06-10 6:45 ` Gen Zhang
2019-06-10 14:31 ` Nicolas Pitre
2019-06-10 7:10 ` Jiri Slaby
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).