[U-Boot] [PATCH] tools/proftool: fix use-after-free

[U-Boot] [PATCH] tools/proftool: fix use-after-free

[Vincent Stehlé]

The read_trace_config() can dereference the line pointer after freeing
it on its error path. Avoid that.

This was found by Coverity Scan.

Signed-off-by: Vincent Stehl? <vincent.stehle@freescale.com>
Cc: Simon Glass <sjg@chromium.org>
---
 tools/proftool.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/proftool.c b/tools/proftool.c
index 9ce7a77..ddf870f 100644
--- a/tools/proftool.c
+++ b/tools/proftool.c
@@ -432,9 +432,10 @@ static int read_trace_config(FILE *fin)
 
 		err = regcomp(&line->regex, tok, REG_NOSUB);
 		if (err) {
+			int r = regex_report_error(&line->regex, err,
+						   "compile", tok);
 			free(line);
-			return regex_report_error(&line->regex, err, "compile",
-						  tok);
+			return r;
 		}
 
 		/* link this new one to the end of the list */
-- 
2.5.3

[U-Boot] [PATCH] tools/proftool: fix use-after-free

[Tom Rini]

On Wed, Oct 07, 2015 at 03:48:48PM +0200, Vincent Stehl? wrote:

> The read_trace_config() can dereference the line pointer after freeing
> it on its error path. Avoid that.
> 
> This was found by Coverity Scan.
> 
> Signed-off-by: Vincent Stehl? <vincent.stehle@freescale.com>
> Cc: Simon Glass <sjg@chromium.org>

Were you in the Coverity talk too? :)  I saw this error as well today
now.  I was actually thinking along the lines of:
diff --git a/tools/proftool.c b/tools/proftool.c
index 9ce7a77..b3d3057 100644
--- a/tools/proftool.c
+++ b/tools/proftool.c
@@ -433,8 +433,9 @@ static int read_trace_config(FILE *fin)
                err = regcomp(&line->regex, tok, REG_NOSUB);
                if (err) {
                        free(line);
-                       return regex_report_error(&line->regex, err, "compile",
+                       err = regex_report_error(&line->regex, err, "compile",
                                                  tok);
+                       return err;
                }
 
                /* link this new one to the end of the list */

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151007/38022432/attachment.sig>

[U-Boot] [PATCH] tools/proftool: fix use-after-free

[Vincent Stehlé]

On 10/07/2015 04:19 PM, Tom Rini wrote:
..
> Were you in the Coverity talk too? :)

Hi Tom,

No, I was not following that talk, sorry.

..
>                         free(line);
> -                       return regex_report_error(&line->regex, err, "compile",
> +                       err = regex_report_error(&line->regex, err, "compile",
>                                                   tok);
> +                       return err;

I am not sure you solve the problem this way. Indeed the structure
pointed to by the line pointer will still have been freed before use
even this way. Who knows what the memory contains when regerror() will
access &line->regex, which is contained into the freed structure?

Best regards,

V.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151007/15a57976/attachment.sig>

[U-Boot] [PATCH] tools/proftool: fix use-after-free

[Tom Rini]

On Wed, Oct 07, 2015 at 04:35:53PM +0200, Vincent Stehl? wrote:
> On 10/07/2015 04:19 PM, Tom Rini wrote:
> ..
> > Were you in the Coverity talk too? :)
> 
> Hi Tom,
> 
> No, I was not following that talk, sorry.

Ah, coincidence then.

> ..
> >                         free(line);
> > -                       return regex_report_error(&line->regex, err, "compile",
> > +                       err = regex_report_error(&line->regex, err, "compile",
> >                                                   tok);
> > +                       return err;
> 
> I am not sure you solve the problem this way. Indeed the structure
> pointed to by the line pointer will still have been freed before use
> even this way. Who knows what the memory contains when regerror() will
> access &line->regex, which is contained into the freed structure?

Er, bah.  That's what I get for writing something in the middle of
listening to a talk too.  I meant to also move the free() to after the
regex_report_error call and just avoid adding another variable.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151007/b1004c0c/attachment.sig>

[U-Boot] tools/proftool: fix use-after-free

[Tom Rini]

On Wed, Oct 07, 2015 at 03:48:48PM +0200, Vincent Stehl? wrote:

> The read_trace_config() can dereference the line pointer after freeing
> it on its error path. Avoid that.
> 
> This was found by Coverity Scan.
> 
> Signed-off-by: Vincent Stehl? <vincent.stehle@freescale.com>
> Cc: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151116/165b8d18/attachment.sig>