Re: [PATCH] Memory leak in tpm_ascii_bios_measurements_open() fix.
On 7/20/07, Yinghai Lu <[EMAIL PROTECTED]> 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) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Memory leak in tpm_ascii_bios_measurements_open() fix.
On 7/19/07, Satyam Sharma <[EMAIL PROTECTED]> wrote:
On 7/20/07, Reiner Sailer <[EMAIL PROTECTED]> wrote:
>
> Jesper Juhl <[EMAIL PROTECTED]> 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, _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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Memory leak in tpm_ascii_bios_measurements_open() fix.
On 7/20/07, Reiner Sailer <[EMAIL PROTECTED]> wrote:
Jesper Juhl <[EMAIL PROTECTED]> 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, _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, _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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Memory leak in tpm_ascii_bios_measurements_open() fix.
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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]>
> ---
>
> 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, _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, _ascii_b_measurments_seqops);
if (!err) {
Do you agree?
Greetings
Reiner
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Memory leak in tpm_ascii_bios_measurements_open() fix.
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 [EMAIL PROTECTED] 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 [EMAIL PROTECTED]
---
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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Memory leak in tpm_ascii_bios_measurements_open() fix.
On 7/20/07, Reiner Sailer [EMAIL PROTECTED] wrote:
Jesper Juhl [EMAIL PROTECTED] 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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Memory leak in tpm_ascii_bios_measurements_open() fix.
On 7/19/07, Satyam Sharma [EMAIL PROTECTED] wrote:
On 7/20/07, Reiner Sailer [EMAIL PROTECTED] wrote:
Jesper Juhl [EMAIL PROTECTED] 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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Memory leak in tpm_ascii_bios_measurements_open() fix.
On 7/20/07, Yinghai Lu [EMAIL PROTECTED] 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) - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
