Re: [PATCH 1/2 v2] Don't overflow in __libdw_in_section

2017-12-20 Thread Mark Wielaard
On Thu, 2017-12-14 at 14:55 +0100, Ulf Hermann wrote:
> On 12/14/2017 02:43 PM, Mark Wielaard wrote:
> > The transformation seems correct. But if we can overflow/underflow
> > here, do we have the same problem in __libdw_offset_in_section
> > where we
> >   check data->d_size - offset < size, with offset a Dwarf_Off?
> 
> Probably we have the same problem there. I didn't catch any instances
> of it, though.

It is surprising we didn't see more issues with this code. There is
also the fake loc cu that fetches data from a different section. I
updated both functions as attached.

Cheers,

MarkFrom 0d100f63db640c533748a7adaa099499b2d2d4b0 Mon Sep 17 00:00:00 2001
From: Ulf Hermann 
Date: Tue, 9 May 2017 18:28:33 +0200
Subject: [PATCH 2/2] Don't overflow in __libdw_in_section and
 __libdw_offset_in_section.

This exposes a bug in dwarf_formstring as detected by the dwarf-getmacros
test before we made sure to use the correct sec_idx for the CU.

Signed-off-by: Ulf Hermann 
Signed-off-by: Mark Wielaard 
---
 libdw/ChangeLog | 7 +++
 libdw/libdwP.h  | 6 --
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 22b7bf4..eb1cb70 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,10 @@
+2017-05-09  Ulf Hermann  
+	Mark Wielaard  
+
+	* libdwP.h (__libdw_in_section): Fix check for the upper border of
+	the range.
+	(__libdw_offset_in_section): Likewise.
+
 2017-12-20  Mark Wielaard  
 
 	* libdwP.h (struct Dwarf_CU): Add sec_idx field.
diff --git a/libdw/libdwP.h b/libdw/libdwP.h
index f524347..82b47d0 100644
--- a/libdw/libdwP.h
+++ b/libdw/libdwP.h
@@ -628,7 +628,8 @@ __libdw_offset_in_section (Dwarf *dbg, int sec_index,
   if (data == NULL)
 return -1;
   if (unlikely (offset > data->d_size)
-  || unlikely (data->d_size - offset < size))
+  || unlikely (data->d_size < size)
+  || unlikely (offset > data->d_size - size))
 {
   __libdw_seterrno (DWARF_E_INVALID_OFFSET);
   return -1;
@@ -645,7 +646,8 @@ __libdw_in_section (Dwarf *dbg, int sec_index,
   if (data == NULL)
 return false;
   if (unlikely (addr < data->d_buf)
-  || unlikely (data->d_size - (addr - data->d_buf) < size))
+  || unlikely (data->d_size < size)
+  || unlikely ((size_t)(addr - data->d_buf) > data->d_size - size))
 {
   __libdw_seterrno (DWARF_E_INVALID_OFFSET);
   return false;
-- 
1.8.3.1



Re: [PATCH 2/2 v2] Generalize cu_sec_idx

2017-12-20 Thread Mark Wielaard
On Thu, 2017-12-14 at 14:53 +0100, Ulf Hermann wrote:
> On 12/14/2017 02:51 PM, Mark Wielaard wrote:
> > This is clever and indeed cu_sec_idx () is not generic enough.
> > But this is also somewhat inefficient. I am working on DWARF5 support
> > and there a CU can come from even more different sections (or file). So
> > I am changing Dwarf_CU to have an explicit section to which is it is
> > associated. This can then also be used by the "fake" CUs like created
> > in dwarf_getmacros.
> 
> Mind that the two most common cases are 0 and 1. In fact nothing else
> was supported before this change. So, most of the time this will not
> do a lot of iteration.

yes, but the original code really was not correct. The attached patch
fixes it by adding an explicit sec_idx field to the Dwarf_CU struct
that is set whenever a struct Dwarf_CU is created, so that we never
have to guess.

This patch combined with the overflow fix makes all testcases PASS.

Cheers,

MarkFrom 51a7292b7ec7ddebcd2abddc7efff9d604494d44 Mon Sep 17 00:00:00 2001
From: Mark Wielaard 
Date: Wed, 20 Dec 2017 16:50:57 +0100
Subject: [PATCH 1/2] libdw: Add explicit section index to struct Dwarf_CU.

The DIE (attribute) data might come from either the main .debug_info
section or for DWARFv4 from a separate .debug_types section. Or in
case of the fake_loc_cu from the .debug_loc section and in the case
of macros from the .debug_macinfo or .debug_macro section.

We didn't handle the last two "fake" CU cases correctly when sanity
checking offsets in __libdw_read_address and __libdw_read_offset.

Add an explicit sec_idx field to struct Dwarf_CU that is always set
to the actual section that the data came from.

Signed-off-by: Mark Wielaard 
---
 libdw/ChangeLog | 11 +++
 libdw/dwarf_begin_elf.c |  1 +
 libdw/dwarf_getmacros.c |  1 +
 libdw/libdwP.h  |  4 +++-
 libdw/libdw_findcu.c|  5 +++--
 5 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 350230e..22b7bf4 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,14 @@
+2017-12-20  Mark Wielaard  
+
+	* libdwP.h (struct Dwarf_CU): Add sec_idx field.
+	(cu_sec_idx): Return cu->sec_idx.
+	* libdw_findcu.c (__libdw_intern_next_unit): Set cu sec_idx to
+	IDX_debug_info or IDX_debug_types.
+	* dwarf_begin_elf.c (valid_p): Set fake_loc_cu->sec_idx to
+	IDX_debug_loc.
+	* dwarf_getmacros.c (read_macros): Set fake_cu->sec_idx to
+	IDX_debug_macro or IDX_debug_macinfo.
+
 2017-12-12  Mark Wielaard  
 
 	* dwarf_aggregate_size.c (dwarf_aggregate_size): Don't peel the
diff --git a/libdw/dwarf_begin_elf.c b/libdw/dwarf_begin_elf.c
index afa15ce..7c3fe10 100644
--- a/libdw/dwarf_begin_elf.c
+++ b/libdw/dwarf_begin_elf.c
@@ -201,6 +201,7 @@ valid_p (Dwarf *result)
 	}
   else
 	{
+	  result->fake_loc_cu->sec_idx = IDX_debug_loc;
 	  result->fake_loc_cu->dbg = result;
 	  result->fake_loc_cu->startp
 	= result->sectiondata[IDX_debug_loc]->d_buf;
diff --git a/libdw/dwarf_getmacros.c b/libdw/dwarf_getmacros.c
index db6582b..c456051 100644
--- a/libdw/dwarf_getmacros.c
+++ b/libdw/dwarf_getmacros.c
@@ -360,6 +360,7 @@ read_macros (Dwarf *dbg, int sec_index,
 	 Version 4 for the old GNU extension, version 5 for DWARF5.  */
   Dwarf_CU fake_cu = {
 	.dbg = dbg,
+	.sec_idx = sec_index,
 	.version = table->version,
 	.offset_size = table->is_64bit ? 8 : 4,
 	.startp = (void *) startp + offset,
diff --git a/libdw/libdwP.h b/libdw/libdwP.h
index 78c0013..f524347 100644
--- a/libdw/libdwP.h
+++ b/libdw/libdwP.h
@@ -293,6 +293,8 @@ struct Dwarf_CU
   uint8_t offset_size;
   uint16_t version;
 
+  size_t sec_idx; /* Normally .debug_info, could be .debug_type or "fake". */
+
   /* Zero if this is a normal CU.  Nonzero if it is a type unit.  */
   size_t type_offset;
   uint64_t type_sig8;
@@ -714,7 +716,7 @@ __libdw_read_offset (Dwarf *dbg, Dwarf *dbg_ret,
 static inline size_t
 cu_sec_idx (struct Dwarf_CU *cu)
 {
-  return cu->type_offset == 0 ? IDX_debug_info : IDX_debug_types;
+  return cu->sec_idx;
 }
 
 static inline bool
diff --git a/libdw/libdw_findcu.c b/libdw/libdw_findcu.c
index 082307b..4e025e2 100644
--- a/libdw/libdw_findcu.c
+++ b/libdw/libdw_findcu.c
@@ -93,8 +93,8 @@ __libdw_intern_next_unit (Dwarf *dbg, bool debug_types)
 }
 
   /* Invalid or truncated debug section data?  */
-  Elf_Data *data = dbg->sectiondata[debug_types
-? IDX_debug_types : IDX_debug_info];
+  size_t sec_idx = debug_types ? IDX_debug_types : IDX_debug_info;
+  Elf_Data *data = dbg->sectiondata[sec_idx];
   if (unlikely (*offsetp > data->d_size))
 *offsetp = data->d_size;
 
@@ -102,6 +102,7 @@ __libdw_intern_next_unit (Dwarf *dbg, bool debug_types)
   struct Dwarf_CU *newp = libdw_typed_alloc (dbg, struct Dwarf_CU);
 
   newp->dbg = dbg;
+  newp->sec_idx = sec_idx;
   newp->start = oldoff;
   newp->end = *offsetp;
   newp->address_size = address_size;
-- 
1.8.3.1