Patch: check for over- and under-flow in decode_locdesc

Patch: check for over- and under-flow in decode_locdesc

[Tom Tromey]

I am going to check this in sometime soon, barring complaints.

This is a new version of a patch we've been carrying in Fedora for quite
some time.  It changes decode_locdesc to check for under- and
over-flow.  This is the topic of CVE-2006-4146.

This version of the patch issues a complaint where earlier versions
called internal_error.  I also cleaned up the comment formatting.

Writing a test case for this is a pain, but I tried it by hand (by
modifying state using a second gdb) to make sure it does the right
thing.

Built and regtested on x86-64 (compile farm).

Tom

2010-11-18  Will Drewry <wad@google.com>
	    Tavis Ormandy <taviso@google.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>

	* dwarf2read.c (decode_locdesc): Enforce location description stack
	boundaries.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 33ebea8..30e1baa 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -13279,8 +13279,7 @@ read_signatured_type (struct objfile *objfile,
    callers will only want a very basic result and this can become a
    complaint.
 
-   Note that stack[0] is unused except as a default error return.
-   Note that stack overflow is not yet handled.  */
+   Note that stack[0] is unused except as a default error return.  */
 
 static CORE_ADDR
 decode_locdesc (struct dwarf_block *blk, struct dwarf2_cu *cu)
@@ -13297,6 +13296,7 @@ decode_locdesc (struct dwarf_block *blk, struct dwarf2_cu *cu)
   i = 0;
   stacki = 0;
   stack[stacki] = 0;
+  stack[++stacki] = 0;
 
   while (i < size)
     {
@@ -13478,6 +13478,22 @@ decode_locdesc (struct dwarf_block *blk, struct dwarf2_cu *cu)
 		     dwarf_stack_op_name (op, 1));
 	  return (stack[stacki]);
 	}
+
+      /* Enforce maximum stack depth of SIZE-1 to avoid writing
+         outside of the allocated space.  Also enforce minimum>0.  */
+      if (stacki >= sizeof (stack) / sizeof (*stack) - 1)
+	{
+	  complaint (&symfile_complaints,
+		     _("location description stack overflow"));
+	  return 0;
+	}
+
+      if (stacki <= 0)
+	{
+	  complaint (&symfile_complaints,
+		     _("location description stack underflow"));
+	  return 0;
+	}
     }
   return (stack[stacki]);
 }

Re: Patch: check for over- and under-flow in decode_locdesc

[Jan Kratochvil]

On Thu, 18 Nov 2010 22:24:27 +0100, Tom Tromey wrote:
> +      if (stacki >= sizeof (stack) / sizeof (*stack) - 1)

I would prefer ARRAY_SIZE here (but sizeof()/sizeof() is also common in GDB).

I will check in the testcase later and if OK.


Thanks,
Jan


gdb/testsuite/
2010-11-19  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.dwarf2/dw2-stack-boundary.exp: New file.
	* gdb.dwarf2/dw2-stack-boundary.S: New file.

--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-stack-boundary.S
@@ -0,0 +1,68 @@
+/* Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+	.section	.debug_info
+debug_start:
+	.long	debug_end - 1f	/* Length of Compilation Unit Info */
+1:
+	.2byte	0x3	/* DWARF version number */
+	.long	.Ldebug_abbrev0	/* Offset Into Abbrev. Section */
+	.byte	0x4	/* Pointer Size (in bytes) */
+	.uleb128 0x1	/* (DIE (0xb) DW_TAG_compile_unit) */
+	.ascii "GNU C 4.4.3\0"	/* DW_AT_producer */
+	.byte	0x1	/* DW_AT_language */
+	.ascii "1.c\0"	/* DW_AT_name */
+
+	.uleb128 0x4	/* (DW_TAG_variable) */
+	.ascii "underflow\0"	/* DW_AT_name */
+	.2byte	2f - 1f	/* DW_AT_location: DW_FORM_block2 */
+1:
+	.byte	0x22	/* DW_OP_plus */
+2:
+
+	.uleb128 0x4	/* (DW_TAG_variable) */
+	.ascii "overflow\0"	/* DW_AT_name */
+	.2byte	2f - 1f	/* DW_AT_location: DW_FORM_block2 */
+1:
+	.fill	100, 1, 0x32	/* 100x DW_OP_lit2 */
+	.byte	0x9f	/* DW_OP_stack_value */
+2:
+
+	.byte	0x0	/* end of children of CU */
+debug_end:
+
+	.section	.debug_abbrev
+.Ldebug_abbrev0:
+	.uleb128 0x1	/* (abbrev code) */
+	.uleb128 0x11	/* (TAG: DW_TAG_compile_unit) */
+	.byte	0x1	/* DW_children_yes */
+	.uleb128 0x25	/* (DW_AT_producer) */
+	.uleb128 0x8	/* (DW_FORM_string) */
+	.uleb128 0x13	/* (DW_AT_language) */
+	.uleb128 0xb	/* (DW_FORM_data1) */
+	.uleb128 0x3	/* (DW_AT_name) */
+	.uleb128 0x8	/* (DW_FORM_string) */
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x4	/* (abbrev code) */
+	.uleb128 0x34	/* (TAG: DW_TAG_variable) */
+	.byte	0x0	/* DW_children_yes */
+	.uleb128 0x3	/* (DW_AT_name) */
+	.uleb128 0x8	/* (DW_FORM_string) */
+	.uleb128 0x02	/* (DW_AT_location) */
+	.uleb128 0x3	/* (DW_FORM_block2) */
+	.byte	0x0
+	.byte	0x0
+	.byte	0x0
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-stack-boundary.exp
@@ -0,0 +1,52 @@
+# Copyright 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+# For now pick a sampling of likely targets.
+if {![istarget *-*-linux*]
+    && ![istarget *-*-gnu*]
+    && ![istarget *-*-elf*]
+    && ![istarget *-*-openbsd*]
+    && ![istarget arm-*-eabi*]
+    && ![istarget powerpc-*-eabi*]} {
+    return 0  
+}
+
+set testfile "dw2-stack-boundary"
+set srcfile ${testfile}.S
+set executable ${testfile}.x
+set binfile ${objdir}/${subdir}/${executable}
+
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" object {}] != "" } {
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+# From gdb_file_cmd:
+if [is_remote host] {
+    set arg [remote_download host $binfile]
+    if { $arg == "" } {
+	perror "download failed"
+	return -1
+    }
+}
+gdb_test_no_output "set complaints 100"
+gdb_test "file $binfile" {Reading symbols from .*\.\.\.location description stack underflow\.\.\.location description stack overflow\.\.\.done\.} "check partial symtab errors"
+
+gdb_test "p underflow" {Asked for position 0 of stack, stack only has 0 elements on it\.}
+gdb_test "p overflow" " = 2"

Re: Patch: check for over- and under-flow in decode_locdesc

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> On Thu, 18 Nov 2010 22:24:27 +0100, Tom Tromey wrote:
>> +      if (stacki >= sizeof (stack) / sizeof (*stack) - 1)

Jan> I would prefer ARRAY_SIZE here (but sizeof()/sizeof() is also
Jan> common in GDB).

I'll commit it shortly with this change.

Jan> I will check in the testcase later and if OK.

I am curious to know how you made this.
Did you just write it by hand?

Tom

Re: Patch: check for over- and under-flow in decode_locdesc

[Tom Tromey]

Jan> I will check in the testcase later and if OK.

I forgot to mention -- I read this and it looks good to me, thanks.

Tom

Re: Patch: check for over- and under-flow in decode_locdesc

[Jan Kratochvil]

On Fri, 19 Nov 2010 17:32:38 +0100, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> Jan> I will check in the testcase later and if OK.
> 
> I am curious to know how you made this.
> Did you just write it by hand?

Yes, copy+modify dw2-bad-parameter-type.{exp,S}.

Sometimes thinking after writing so many gdb.dwarf2/ testcases it could be more
time effective to write some script generator for it instead.

OTOH the gdb.dwarf2/ testcases are for various special cases of DWARF and even
about invalid DWARF. So which generated parts can be automated? Maybe not many.


Regards,
Jan

Re: Patch: check for over- and under-flow in decode_locdesc

[Jan Kratochvil]

On Fri, 19 Nov 2010 17:34:33 +0100, Tom Tromey wrote:
> Jan> I will check in the testcase later and if OK.
> 
> I forgot to mention -- I read this and it looks good to me, thanks.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2010-11/msg00099.html


Thanks,
Jan