* [PATCH] Memory leak in tpm_ascii_bios_measurements_open() fix.
@ 2007-07-18 23:11 Jesper Juhl
0 siblings, 0 replies; 5+ messages in thread
From: Jesper Juhl @ 2007-07-18 23:11 UTC (permalink / raw)
To: Linux Kernel Mailing List
Cc: Seiji Munetoh, Stefan Berger, Reiner Sailer, Kylene Hall,
jesper.juhl
Ehlo,
Coverity found a memory leak in tpm_ascii_bios_measurements_open().
If "read_log(log)" fails, then we may leak 'log' and
'log->bios_event_log'.
This patch should fix it, but please double check it. I don't know
this code very well and the patch has only been compile tested.
Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---
drivers/char/tpm/tpm_bios.c | 11 ++++++++---
1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/char/tpm/tpm_bios.c b/drivers/char/tpm/tpm_bios.c
index 4eba32b..4b26ce4 100644
--- a/drivers/char/tpm/tpm_bios.c
+++ b/drivers/char/tpm/tpm_bios.c
@@ -427,7 +427,7 @@ static int tpm_ascii_bios_measurements_open(struct inode *inode,
return -ENOMEM;
if ((err = read_log(log)))
- return err;
+ goto out_free;
/* now register seq file */
err = seq_open(file, &tpm_ascii_b_measurments_seqops);
@@ -435,10 +435,15 @@ static int tpm_ascii_bios_measurements_open(struct inode *inode,
seq = file->private_data;
seq->private = log;
} else {
- kfree(log->bios_event_log);
- kfree(log);
+ goto out_free;
}
+
+out:
return err;
+out_free:
+ kfree(log->bios_event_log);
+ kfree(log);
+ goto out;
}
const struct file_operations tpm_ascii_bios_measurements_ops = {
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Memory leak in tpm_ascii_bios_measurements_open() fix.
@ 2007-07-20 3:33 Reiner Sailer
2007-07-20 4:07 ` Satyam Sharma
0 siblings, 1 reply; 5+ messages in thread
From: Reiner Sailer @ 2007-07-20 3:33 UTC (permalink / raw)
To: Jesper Juhl
Cc: jesper.juhl, kjhall, Linux Kernel Mailing List, Seiji Munetoh,
Reiner Sailer, stefanb
Jesper,
thank you very much for finding this error and for posting a patch
proposal. Since Kylene is not online, I am responding. Please see my
inlines and an alternative patch proposal below.
Jesper Juhl <jesper.juhl@gmail.com> wrote on 07/18/2007 07:11:54 PM:
> Ehlo,
>
> Coverity found a memory leak in tpm_ascii_bios_measurements_open().
>
> If "read_log(log)" fails, then we may leak 'log' and
> 'log->bios_event_log'.
>
> This patch should fix it, but please double check it. I don't know
> this code very well and the patch has only been compile tested.
>
>
> Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
> ---
>
> drivers/char/tpm/tpm_bios.c | 11 ++++++++---
> 1 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_bios.c b/drivers/char/tpm/tpm_bios.c
> index 4eba32b..4b26ce4 100644
> --- a/drivers/char/tpm/tpm_bios.c
> +++ b/drivers/char/tpm/tpm_bios.c
> @@ -427,7 +427,7 @@ static int
> tpm_ascii_bios_measurements_open(struct inode *inode,
> return -ENOMEM;
>
> if ((err = read_log(log)))
> - return err;
> + goto out_free;
log->bios_event_log should not be pointing to allocated memory here
(seems cleaner if read_log cleans its allocated memory in the error case)
---> just free log
>
> /* now register seq file */
> err = seq_open(file, &tpm_ascii_b_measurments_seqops);
> @@ -435,10 +435,15 @@ static int
> tpm_ascii_bios_measurements_open(struct inode *inode,
> seq = file->private_data;
> seq->private = log;
> } else {
> - kfree(log->bios_event_log);
> - kfree(log);
> + goto out_free;
> }
> +
> +out:
> return err;
> +out_free:
> + kfree(log->bios_event_log);
> + kfree(log);
> + goto out;
> }
>
> const struct file_operations tpm_ascii_bios_measurements_ops = {
>
>
The following patch should be sufficient to fix the problem you discovered:
---
drivers/char/tpm/tpm_bios.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Index: linux-2.6.22-rc7/drivers/char/tpm/tpm_bios.c
===================================================================
--- linux-2.6.22-rc7.orig/drivers/char/tpm/tpm_bios.c
+++ linux-2.6.22-rc7/drivers/char/tpm/tpm_bios.c
@@ -426,9 +426,10 @@ static int tpm_ascii_bios_measurements_o
if (!log)
return -ENOMEM;
- if ((err = read_log(log)))
+ if ((err = read_log(log))) {
+ kfree(log);
return err;
-
+ }
/* now register seq file */
err = seq_open(file, &tpm_ascii_b_measurments_seqops);
if (!err) {
Do you agree?
Greetings
Reiner
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Memory leak in tpm_ascii_bios_measurements_open() fix.
2007-07-20 3:33 [PATCH] Memory leak in tpm_ascii_bios_measurements_open() fix Reiner Sailer
@ 2007-07-20 4:07 ` Satyam Sharma
2007-07-20 4:20 ` Yinghai Lu
0 siblings, 1 reply; 5+ messages in thread
From: Satyam Sharma @ 2007-07-20 4:07 UTC (permalink / raw)
To: Reiner Sailer
Cc: Jesper Juhl, kjhall, Linux Kernel Mailing List, Seiji Munetoh,
Reiner Sailer, stefanb
On 7/20/07, Reiner Sailer <sailer@us.ibm.com> wrote:
>
> Jesper Juhl <jesper.juhl@gmail.com> wrote on 07/18/2007 07:11:54 PM:
> >
> > +++ b/drivers/char/tpm/tpm_bios.c
> > @@ -427,7 +427,7 @@ static int
> > tpm_ascii_bios_measurements_open(struct inode *inode,
> > return -ENOMEM;
> >
> > if ((err = read_log(log)))
> > - return err;
> > + goto out_free;
> >
> > /* now register seq file */
> > err = seq_open(file, &tpm_ascii_b_measurments_seqops);
> > @@ -435,10 +435,15 @@ static int
> > tpm_ascii_bios_measurements_open(struct inode *inode,
> > seq = file->private_data;
> > seq->private = log;
> > } else {
> > - kfree(log->bios_event_log);
> > - kfree(log);
> > + goto out_free;
> > }
> > +
> > +out:
> > return err;
> > +out_free:
> > + kfree(log->bios_event_log);
> > + kfree(log);
> > + goto out;
> > }
> >
> > const struct file_operations tpm_ascii_bios_measurements_ops = {
> >
> >
>
> Index: linux-2.6.22-rc7/drivers/char/tpm/tpm_bios.c
> ===================================================================
> --- linux-2.6.22-rc7.orig/drivers/char/tpm/tpm_bios.c
> +++ linux-2.6.22-rc7/drivers/char/tpm/tpm_bios.c
> @@ -426,9 +426,10 @@ static int tpm_ascii_bios_measurements_o
> if (!log)
> return -ENOMEM;
>
> - if ((err = read_log(log)))
> + if ((err = read_log(log))) {
> + kfree(log);
> return err;
> -
> + }
> /* now register seq file */
> err = seq_open(file, &tpm_ascii_b_measurments_seqops);
> if (!err) {
>
> Do you agree?
Jesper's patch makes the code conforming to the kernel coding style
(kfree()'s in common error paths, single "return" call, etc). Not a big
issue, just a consistency thing, so feel free to take your pick between
the two alternatives.
BTW please also fix an exactly similar memory leak 30 lines later in the
same file -- in tpm_binary_bios_measurements_open() -- while you're at it.
Thanks,
Satyam
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Memory leak in tpm_ascii_bios_measurements_open() fix.
2007-07-20 4:07 ` Satyam Sharma
@ 2007-07-20 4:20 ` Yinghai Lu
2007-07-20 4:36 ` Satyam Sharma
0 siblings, 1 reply; 5+ messages in thread
From: Yinghai Lu @ 2007-07-20 4:20 UTC (permalink / raw)
To: Satyam Sharma
Cc: Reiner Sailer, Jesper Juhl, kjhall, Linux Kernel Mailing List,
Seiji Munetoh, Reiner Sailer, stefanb
On 7/19/07, Satyam Sharma <satyam.sharma@gmail.com> wrote:
> On 7/20/07, Reiner Sailer <sailer@us.ibm.com> wrote:
> >
> > Jesper Juhl <jesper.juhl@gmail.com> wrote on 07/18/2007 07:11:54 PM:
> > >
> > > +++ b/drivers/char/tpm/tpm_bios.c
> > > @@ -427,7 +427,7 @@ static int
> > > tpm_ascii_bios_measurements_open(struct inode *inode,
> > > return -ENOMEM;
> > >
> > > if ((err = read_log(log)))
> > > - return err;
> > > + goto out_free;
> > >
> > > /* now register seq file */
> > > err = seq_open(file, &tpm_ascii_b_measurments_seqops);
> > > @@ -435,10 +435,15 @@ static int
> > > tpm_ascii_bios_measurements_open(struct inode *inode,
> > > seq = file->private_data;
> > > seq->private = log;
> > > } else {
> > > - kfree(log->bios_event_log);
> > > - kfree(log);
> > > + goto out_free;
> > > }
> > > +
> > > +out:
> > > return err;
> > > +out_free:
> > > + kfree(log->bios_event_log);
> > > + kfree(log);
> > > + goto out;
> > > }
> > >
===>
out_free:
kfree(log->bios_event_log);
kfree(log);
out:
return err;
could kill one extra goto.
YH
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Memory leak in tpm_ascii_bios_measurements_open() fix.
2007-07-20 4:20 ` Yinghai Lu
@ 2007-07-20 4:36 ` Satyam Sharma
0 siblings, 0 replies; 5+ messages in thread
From: Satyam Sharma @ 2007-07-20 4:36 UTC (permalink / raw)
To: Yinghai Lu
Cc: Reiner Sailer, Jesper Juhl, kjhall, Linux Kernel Mailing List,
Seiji Munetoh, Reiner Sailer, stefanb
On 7/20/07, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> out_free:
> kfree(log->bios_event_log);
> kfree(log);
> out:
> return err;
>
> could kill one extra goto.
Such constructs disallow the success path from falling through
to the same "return err;" (with err = 0 for success obviously)
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2007-07-20 4:36 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-07-20 3:33 [PATCH] Memory leak in tpm_ascii_bios_measurements_open() fix Reiner Sailer
2007-07-20 4:07 ` Satyam Sharma
2007-07-20 4:20 ` Yinghai Lu
2007-07-20 4:36 ` Satyam Sharma
-- strict thread matches above, loose matches on Subject: below --
2007-07-18 23:11 Jesper Juhl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox