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 <[email protected]>
---
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);
---
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 <[email protected]>
>
> ---
> 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);
>
> ---
>
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 <[email protected]>
> >
> > ---
> > 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);
> >
> > ---
> >
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 <[email protected]>
---
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);
---
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
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
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
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 <[email protected]>
>
> ---
> 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
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
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
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
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 <[email protected]>
---
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);
---
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 <[email protected]>
Reviewed-by: Nicolas Pitre <[email protected]>
>
> ---
> 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);
>
> ---
>
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 <[email protected]>
Reviewed-by: Nicolas Pitre <[email protected]>
---
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);
---