Re: [PATCH] libdw: Don't overflow stack with user defined macro attributes array.

Re: [PATCH] libdw: Don't overflow stack with user defined macro attributes array.

[Petr Machata]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

Mark Wielaard <mjw@redhat.com> writes:

> -      Dwarf_Attribute attributes[proto->nforms];
> +      Dwarf_Attribute *attributes;
> +      Dwarf_Attribute *attributesp = NULL;
> +      Dwarf_Attribute nattributes[8];
> +      if (unlikely (proto->nforms > 8))
> +	{
> +	  attributesp = malloc (sizeof (Dwarf_Attribute) * proto->nforms);
> +	  if (attributesp == NULL)
> +	    {
> +	      __libdw_seterrno (DWARF_E_NOMEM);
> +	      return -1;
> +	    }
> +	  attributes = attributesp;
> +	}
> +      else
> +	attributes = &nattributes[0];
> +
>        for (Dwarf_Word i = 0; i < proto->nforms; ++i)
>  	{

There's a return in this loop that needs free (attributesp) as well.

Thanks,
Petr

Re: [PATCH] libdw: Don't overflow stack with user defined macro attributes array.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 177 bytes --]

On Tue, 2015-04-21 at 20:40 +0200, Petr Machata wrote:
> Mark Wielaard <mjw@redhat.com> writes:
> 
> > Fixed patch attached.
> 
> Looks good.

Thanks. Pushed to master.

Re: [PATCH] libdw: Don't overflow stack with user defined macro attributes array.

[Petr Machata]

[-- Attachment #1: Type: text/plain, Size: 99 bytes --]

Mark Wielaard <mjw@redhat.com> writes:

> Fixed patch attached.

Looks good.

Thanks,
Petr

Re: [PATCH] libdw: Don't overflow stack with user defined macro attributes array.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 317 bytes --]

On Tue, Apr 21, 2015 at 04:22:53PM +0200, Petr Machata wrote:
> Mark Wielaard <mjw@redhat.com> writes:
> >        for (Dwarf_Word i = 0; i < proto->nforms; ++i)
> >  	{
> 
> There's a return in this loop that needs free (attributesp) as well.

Oops. Missed that one.
Fixed patch attached.

Thanks,

Mark

[-- Attachment #2: 0001-libdw-Don-t-overflow-stack-with-user-defined-macro-a.patch --]
[-- Type: text/plain, Size: 2736 bytes --]

>From c91d13dd269b08432e8c1cd4385952071097d827 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Tue, 21 Apr 2015 15:46:01 +0200
Subject: [PATCH] libdw: Don't overflow stack with user defined macro
 attributes array.

In theory user defined debug macros can have an arbitrary number of
arguments. Don't allocate them all on stack. If there are more than
8 (arbitrary number, but no sane macro should have more arguments),
then dynamically allocate and free the attributes.

Found by gcc -fsanitize=undefined. Which pointed out the nforms could
be zero, creating an empty vla (which could cause undefined behavior).

Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 libdw/ChangeLog         |  5 +++++
 libdw/dwarf_getmacros.c | 30 ++++++++++++++++++++++++++----
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 3abb382..87c7d82 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,8 @@
+2015-04-21  Mark Wielaard  <mjw@redhat.com>
+
+	* dwarf_getmacros.c (read_macros): Allocate attributes dynamically
+	when there are more than 8.
+
 2015-04-01  Petr Machata  <pmachata@redhat.com>
 
 	* libdwP.h (DWARF_E_NOT_CUDIE): New enumerator.
diff --git a/libdw/dwarf_getmacros.c b/libdw/dwarf_getmacros.c
index f9f2996..740368e 100644
--- a/libdw/dwarf_getmacros.c
+++ b/libdw/dwarf_getmacros.c
@@ -361,7 +361,22 @@ read_macros (Dwarf *dbg, int sec_index,
 	.endp = (void *) endp,
       };
 
-      Dwarf_Attribute attributes[proto->nforms];
+      Dwarf_Attribute *attributes;
+      Dwarf_Attribute *attributesp = NULL;
+      Dwarf_Attribute nattributes[8];
+      if (unlikely (proto->nforms > 8))
+	{
+	  attributesp = malloc (sizeof (Dwarf_Attribute) * proto->nforms);
+	  if (attributesp == NULL)
+	    {
+	      __libdw_seterrno (DWARF_E_NOMEM);
+	      return -1;
+	    }
+	  attributes = attributesp;
+	}
+      else
+	attributes = &nattributes[0];
+
       for (Dwarf_Word i = 0; i < proto->nforms; ++i)
 	{
 	  /* We pretend this is a DW_AT_GNU_macros attribute so that
@@ -373,8 +388,11 @@ read_macros (Dwarf *dbg, int sec_index,
 	  attributes[i].cu = &fake_cu;
 
 	  size_t len = __libdw_form_val_len (&fake_cu, proto->forms[i], readp);
-	  if (len == (size_t) -1)
-	    return -1;
+	  if (unlikely (len == (size_t) -1))
+	    {
+	      free (attributesp);
+	      return -1;
+	    }
 
 	  readp += len;
 	}
@@ -385,7 +403,11 @@ read_macros (Dwarf *dbg, int sec_index,
 	.attributes = attributes,
       };
 
-      if (callback (&macro, arg) != DWARF_CB_OK)
+      int res = callback (&macro, arg);
+      if (unlikely (attributesp != NULL))
+	free (attributesp);
+
+      if (res != DWARF_CB_OK)
 	return readp - startp;
     }
 
-- 
2.1.0

[PATCH] libdw: Don't overflow stack with user defined macro attributes array.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 2223 bytes --]

In theory user defined debug macros can have an arbitrary number of
arguments. Don't allocate them all on stack. If there are more than
8 (arbitrary number, but no sane macro should have more arguments),
then dynamically allocate and free the attributes.

Found by gcc -fsanitize=undefined. Which pointed out the nforms could
be zero, creating an empty vla (which could cause undefined behavior).

Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 libdw/ChangeLog         |  5 +++++
 libdw/dwarf_getmacros.c | 23 +++++++++++++++++++++--
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 3abb382..87c7d82 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,8 @@
+2015-04-21  Mark Wielaard  <mjw@redhat.com>
+
+	* dwarf_getmacros.c (read_macros): Allocate attributes dynamically
+	when there are more than 8.
+
 2015-04-01  Petr Machata  <pmachata@redhat.com>
 
 	* libdwP.h (DWARF_E_NOT_CUDIE): New enumerator.
diff --git a/libdw/dwarf_getmacros.c b/libdw/dwarf_getmacros.c
index f9f2996..c5d47d6 100644
--- a/libdw/dwarf_getmacros.c
+++ b/libdw/dwarf_getmacros.c
@@ -361,7 +361,22 @@ read_macros (Dwarf *dbg, int sec_index,
 	.endp = (void *) endp,
       };
 
-      Dwarf_Attribute attributes[proto->nforms];
+      Dwarf_Attribute *attributes;
+      Dwarf_Attribute *attributesp = NULL;
+      Dwarf_Attribute nattributes[8];
+      if (unlikely (proto->nforms > 8))
+	{
+	  attributesp = malloc (sizeof (Dwarf_Attribute) * proto->nforms);
+	  if (attributesp == NULL)
+	    {
+	      __libdw_seterrno (DWARF_E_NOMEM);
+	      return -1;
+	    }
+	  attributes = attributesp;
+	}
+      else
+	attributes = &nattributes[0];
+
       for (Dwarf_Word i = 0; i < proto->nforms; ++i)
 	{
 	  /* We pretend this is a DW_AT_GNU_macros attribute so that
@@ -385,7 +400,11 @@ read_macros (Dwarf *dbg, int sec_index,
 	.attributes = attributes,
       };
 
-      if (callback (&macro, arg) != DWARF_CB_OK)
+      int res = callback (&macro, arg);
+      if (unlikely (attributesp != NULL))
+	free (attributesp);
+
+      if (res != DWARF_CB_OK)
 	return readp - startp;
     }
 
-- 
2.1.0