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

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

[Ulf Hermann]

This exposes a bug in dwarf_formstring as detected by the dwarf-getmacros
test. We cannot unconditionally assume that a string is in either the
IDX_debug_info or the IDX_debug_types section as determined by
cu_sec_idx.

(Signed-off instead of Change-Id ...)

Signed-off-by: Ulf Hermann <ulf.hermann@qt.io>
---
  libdw/ChangeLog | 4 ++++
  libdw/libdwP.h  | 3 ++-
  2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 4375244..996cd2e 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,7 @@
+2017-05-09  Ulf Hermann  <ulf.hermann@qt.io>
+
+	* libdwP.h: Fix check for the upper border of the range in 
__libdw_in_section.
+
  2017-11-03  Mark Wielaard  <mark@klomp.org>
   	* dwarf_getlocation.c (__libdw_intern_expression): Handle
diff --git a/libdw/libdwP.h b/libdw/libdwP.h
index 78c0013..e092d8e 100644
--- a/libdw/libdwP.h
+++ b/libdw/libdwP.h
@@ -643,7 +643,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;
-- 
2.8.1.windows.1

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

[Mark Wielaard]

Hi Ulf,

(Meta, I have some trouble applying this with git am, it thinks the
patch is malformed. But I can apply by hand of course.)

On Fri, 2017-12-08 at 16:05 +0100, Ulf Hermann wrote:
> 
> +2017-05-09  Ulf Hermann  <ulf.hermann@qt.io>
> +
> +	* libdwP.h: Fix check for the upper border of the range in 
> __libdw_in_section.
> +
>   2017-11-03  Mark Wielaard  <mark@klomp.org>
>    	* dwarf_getlocation.c (__libdw_intern_expression): Handle
> diff --git a/libdw/libdwP.h b/libdw/libdwP.h
> index 78c0013..e092d8e 100644
> --- a/libdw/libdwP.h
> +++ b/libdw/libdwP.h
> @@ -643,7 +643,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;

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?

Thanks,

Mark

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

[Ulf Hermann]

On 12/14/2017 02:43 PM, Mark Wielaard wrote:
> (Meta, I have some trouble applying this with git am, it thinks the
> patch is malformed. But I can apply by hand of course.)

Oh, sorry for that. It's probably the leading spaces again. I keep messing up my mail setup on windows ...

> 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.

regards,
Ulf

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

[Mark Wielaard]

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

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,

Mark

[-- Attachment #2: 0002-Don-t-overflow-in-__libdw_in_section-and-__libdw_off.patch --]
[-- Type: text/x-patch, Size: 1929 bytes --]

From 0d100f63db640c533748a7adaa099499b2d2d4b0 Mon Sep 17 00:00:00 2001
From: Ulf Hermann <ulf.hermann@qt.io>
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 <ulf.hermann@qt.io>
Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 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  <ulf.hermann@qt.io>
+	    Mark Wielaard  <mark@klomp.org>
+
+	* libdwP.h (__libdw_in_section): Fix check for the upper border of
+	the range.
+	(__libdw_offset_in_section): Likewise.
+
 2017-12-20  Mark Wielaard  <mark@klomp.org>
 
 	* 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 1/2 v2] Don't overflow in __libdw_in_section

[Ulf Hermann]

> 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.

Looks good to me.

Ulf

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

[Mark Wielaard]

On Thu, Dec 21, 2017 at 02:47:49PM +0100, Ulf Hermann wrote:
> > 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.
> 
> Looks good to me.

Thanks for taking a look. Pushed to master.