Fix memory leak in vc_resize/vc_allocate
PATCH 1/1 Fix memory leak in vc_resize/vc_allocate
From: Catalin Marinas <[email protected]>
Memory leaks can happen in the vc_resize() function in drivers/char/vt.c because of the vc->vc_screenbuf variable overriding in vc_allocate(). The kmemleak reported trace is as follows:
<__kmalloc>
<vc_resize>
<fbcon_init>
<visual_init>
<vc_allocate>
<con_open>
<tty_open>
<chrdev_open>
This patch no longer allocates a screen buffer in vc_allocate() if it was already allocated by vc_resize().
---
drivers/char/vt.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
drivers/char/vt.c +2 -1
@@ -730,7 +730,8 @@ int vc_allocate(unsigned int currcons) /visual_init (vc, currcons,1 );if (!*vc->vc_uni_pagedir_loc)con_set_default_unimap (vc); - vc->vc_screenbuf =kmalloc (vc->vc_screenbuf_size,GFP_KERNEL ); +if (!vc->vc_kmalloced) + vc->vc_screenbuf =kmalloc (vc->vc_screenbuf_size,GFP_KERNEL );if (!vc->vc_screenbuf) {kfree (vc); vc_cons[currcons].d =NULL ;
Quoted 29 lines · On Thu, 10 Aug 2006 15:22:21 +0100 Catalin Marinas <[email protected]>…
From: Catalin Marinas <[email protected]>
Memory leaks can happen in the
vc_resize()function indrivers/char/vt.cbecause of thevc->vc_screenbufvariable overriding invc_allocate(). The kmemleak reported trace is as follows:<
__kmalloc>
<vc_resize>
<fbcon_init>
<visual_init>
<vc_allocate>
<con_open>
<tty_open>
<chrdev_open>This patch no longer allocates a screen buffer in
vc_allocate()if it was already allocated byvc_resize().Signed-off-by Catalin Marinas <[email protected]>---
drivers/char/vt.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)drivers/char/vt.c +0
-0
drivers/char/vt.c int vc_allocate(unsigned int currcons) / · line 737
Quoted 9 lines
drivers/char/vt.c +2
-1@@ -730,7 +730,8 @@ int vc_allocate(unsigned int currcons) /visual_init (vc, currcons,1 );if (!*vc->vc_uni_pagedir_loc)con_set_default_unimap (vc); - vc->vc_screenbuf =kmalloc (vc->vc_screenbuf_size,GFP_KERNEL ); +if (!vc->vc_kmalloced) + vc->vc_screenbuf =kmalloc (vc->vc_screenbuf_size,GFP_KERNEL );if (!vc->vc_screenbuf) {kfree (vc); vc_cons[currcons].d =NULL ;
hm. Maybe. I'd worry that the memory at vc->vc_screenbuf isn't of the correct size and this patch will convert a leak into a buffer overrun.
Also, what's up with this, in vc_resize()?
if (vc->vc_kmalloced) kfree(vc->vc_screenbuf); vc->vc_screenbuf = newscreen; vc->vc_kmalloced = 1;
if vc->vc_kmalloced means "there is kmalloced memory at vc->vc_screenbuf" then this is wrong.
This code is all pretty twisty and I fear touching it.
Quoted 36 lines · Andrew Morton
Quoted 33 lines · On Thu, 10 Aug 2006 15:22:21 +0100 Catalin Marinas <[email protected]>…
From: Catalin Marinas <[email protected]>
Memory leaks can happen in the
vc_resize()function indrivers/char/vt.cbecause of thevc->vc_screenbufvariable overriding invc_allocate(). The kmemleak reported trace is as follows:<
__kmalloc>
<vc_resize>
<fbcon_init>
<visual_init>
<vc_allocate>
<con_open>
<tty_open>
<chrdev_open>This patch no longer allocates a screen buffer in
vc_allocate()if it was already allocated byvc_resize().Signed-off-by Catalin Marinas <[email protected]>---
drivers/char/vt.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)drivers/char/vt.c +0
-1@@ -730,7 +730,8 @@ int vc_allocate(unsigned int currcons) /visual_init (vc, currcons,1 );if (!*vc->vc_uni_pagedir_loc)con_set_default_unimap (vc); - vc->vc_screenbuf =kmalloc (vc->vc_screenbuf_size,GFP_KERNEL );
Quoted 8 lines
drivers/char/vt.c int vc_allocate(unsigned int currcons) / · line 737
Quoted 5 lines
drivers/char/vt.c +2
-0+if (!vc->vc_kmalloced) + vc->vc_screenbuf =kmalloc (vc->vc_screenbuf_size,GFP_KERNEL );if (!vc->vc_screenbuf) {kfree (vc); vc_cons[currcons].d =NULL ;hm. Maybe. I'd worry that the memory at
vc->vc_screenbufisn't of the correct size and this patch will convert a leak into a buffer overrun.
It's a safe test, visual_init() will assure that screenbuf_size will always be correct.
Quoted 9 lines
Also, what's up with this, in
vc_resize()?if (vc->vc_kmalloced) kfree(vc->vc_screenbuf); vc->vc_screenbuf = newscreen; vc->vc_kmalloced = 1;if
vc->vc_kmallocedmeans "there is kmalloced memory atvc->vc_screenbuf" then this is wrong.
vc_screenbuf is either of the bootmem type or the kmalloced type as indicated by vc->vc_kmalloced.
Quoted 2 lines
This code is all pretty twisty and I fear touching it.
Yes, but the patch as is should be okay.
Tony
Quoted 11 lines · Andrew Morton
Quoted 9 lines · On Thu, 10 Aug 2006 15:22:21 +0100 Catalin Marinas <[email protected]>…
drivers/char/vt.c +0
-1@@ -730,7 +730,8 @@ int vc_allocate(unsigned int currcons) /visual_init (vc, currcons,1 );if (!*vc->vc_uni_pagedir_loc)con_set_default_unimap (vc); - vc->vc_screenbuf =kmalloc (vc->vc_screenbuf_size,GFP_KERNEL );
Quoted 8 lines
drivers/char/vt.c int vc_allocate(unsigned int currcons) / · line 737
Quoted 5 lines
drivers/char/vt.c +2
-0+if (!vc->vc_kmalloced) + vc->vc_screenbuf =kmalloc (vc->vc_screenbuf_size,GFP_KERNEL );if (!vc->vc_screenbuf) {kfree (vc); vc_cons[currcons].d =NULL ;hm. Maybe. I'd worry that the memory at
vc->vc_screenbufisn't of the correct size and this patch will convert a leak into a buffer overrun.
My initial attempt was to free the kmalloc'ed buffer and reallocate it in vc_allocate (similar to the code in vc_resize) but I realised that vc_screenbuf kmalloced block is always of the vs_screenbuf_size length.
Quoted 9 lines
Also, what's up with this, in
vc_resize()?if (vc->vc_kmalloced) kfree(vc->vc_screenbuf); vc->vc_screenbuf = newscreen; vc->vc_kmalloced = 1;if
vc->vc_kmallocedmeans "there is kmalloced memory atvc->vc_screenbuf" then this is wrong.
I think the above should be fine because vc_resize can be called either via vc_allocate->visual_init->fbcon_init when the buffer was not yet allocated or via a tty ioctl etc. and it needs to free the old buffer.
Signature · 1 line
Catalin