This is the mail archive of the
elfutils-devel@sourceware.org
mailing list for the elfutils project.
Re: [PATCH] libdw: Fix __libdw_form_val_len endp pointer.
- From: Mark Wielaard <mjw at redhat dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Fri, 12 Dec 2014 16:54:21 +0100
- Subject: Re: [PATCH] libdw: Fix __libdw_form_val_len endp pointer.
On Thu, 2014-12-11 at 15:34 -0800, Josh Stone wrote:
> On 12/11/2014 03:05 PM, Mark Wielaard wrote:
> > While rebasing my work on top of Josh attrabbrev reading speedups
> > I noticed a typo/thinko in my form_val_len bound checking patch.
> >
> > We should check against the die->cu->endp, not the abbrev endp.
>
> In that case, I don't think you need endp as a parameter at all. Just
> use cu->endp as needed, and prevent this mistake from ever happening.
Yes we can. But it is slightly tricky since we have a couple of places
where we have "fake" attributes that get their form data not from
the .debug_info CU data. So we need to make sure those attributes have a
fake cu with the data pointers properly initialized.
We have one "static" empty location expression that we have to give a
static "one byte" fake CU. The new debug macros also have form data,
which comes from a fake debug_macro CU. The somewhat tricky one is
dwarf_getlocation_attr, which gives a fake attribute that represents a
location given an Dwarf_Op. In the case of DW_OP_implicit_value,
DW_OP_GNU_entry_value and DW_OP_GNU_const_type this returns an attribute
that represents a (expression) block. The block data can come from
either the .debug_info cu data or from the location list data
in .debug_loc. For the last case we need a fake loc CU for each Dwarf
that covers the .debug_loc data.
The attached patch implements this. It also adds bounds checking for
dwarf_form_block. Which did actually try to do some checking to see
whether the returned block was completely in the CU. Luckily this check
was wrong and for the above tricky cases not notice the block data was
beyond the .debug_info data. Correcting the check without the proper
fake CUs in place would make some testcases fail.
> Maybe you could also check valp >= cu->startp while you're at it, but
> that's probably an ok invariant to just assume.
I think that is redundant checking in a place where we want to do as
little work as possible. Also that is checked earlier in the code.
> Also along these lines, the dbg parameter could just use cu->dbg as
> needed. Saving parameters probably won't do much for performance in
> this case, nor is changing to a pointer read likely to hurt. It does
> help code clarity though, IMO.
Yes, much nicer to read.
From c93108be2a0257976a16a56a6acac22248efcbfc Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Fri, 12 Dec 2014 16:43:04 +0100
Subject: [PATCH] libdw: Make sure all attributes come with a (fake) CU for
bound checks.
All attributes now have a reference to a (fake) CU that has startp and
endp set to the data section where the form data comes from. Use that
for bounds checking in __libdw_form_val_len and dwarf_formblock to make
sure data read doesn't overflow any data section.
Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
libdw/ChangeLog | 21 +++++++++++++++++++++
libdw/dwarf_begin_elf.c | 22 ++++++++++++++++++++++
libdw/dwarf_child.c | 4 +---
libdw/dwarf_end.c | 3 +++
libdw/dwarf_formblock.c | 21 ++++++++++++++-------
libdw/dwarf_getattrs.c | 4 +---
libdw/dwarf_getlocation_attr.c | 29 ++++++++++++++++++++++++-----
libdw/dwarf_getlocation_implicit_pointer.c | 8 +++++---
libdw/dwarf_getmacros.c | 5 +++--
libdw/libdwP.h | 23 +++++++++++++----------
libdw/libdw_form.c | 12 ++++++------
11 files changed, 113 insertions(+), 39 deletions(-)
diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index db4995f..2fe5454 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,24 @@
+2014-12-12 Mark Wielaard <mjw@redhat.com>
+
+ * libdwP.h (struct Dwarf): Add fake_loc_cu.
+ (__libdw_form_val_compute_len): Drop dbg and endp arguments.
+ (__libdw_form_val_len): Likewise.
+ * libdw_form.c (__libdw_form_val_compute_len): Likewise.
+ * dwarf_begin_elf.c (valid_p): Setup fake_loc_cu.
+ * dwarf_end.c (dwarf_end): Free fake_loc_cu.
+ * dwarf_child.c (__libdw_find_attr): Call __libdw_form_val_len with
+ just cu.
+ * dwarf_getattrs.c (dwarf_getattrs): Likewise.
+ * dwarf_formblock.c (dwarf_formblock): Add bounds checking.
+ * dwarf_getlocation_attr.c (attr_form_cu): New function.
+ (dwarf_getlocation_attr): Use attr_form_cu to set result->cu.
+ * dwarf_getlocation_implicit_pointer.c (empty_cu): New static var.
+ (__libdw_empty_loc_attr): Drop cu argument, use empty_cu.
+ (dwarf_getlocation_implicit_pointer): Call __libdw_empty_loc_attr with
+ one argument.
+ * dwarf_getmacros.c (read_macros): Also setup startp and endp for
+ fake_cu. Call __libdw_form_val_len with just fake_cu.
+
2014-12-11 Mark Wielaard <mjw@redhat.com>
* libdw_findcu.c (__libdw_intern_next_unit): Sanity check offset.
diff --git a/libdw/dwarf_begin_elf.c b/libdw/dwarf_begin_elf.c
index 4c6346a..4c49ce2 100644
--- a/libdw/dwarf_begin_elf.c
+++ b/libdw/dwarf_begin_elf.c
@@ -235,6 +235,28 @@ valid_p (Dwarf *result)
result = NULL;
}
+ if (result != NULL && result->sectiondata[IDX_debug_loc] != NULL)
+ {
+ result->fake_loc_cu = (Dwarf_CU *) calloc (1, sizeof (Dwarf_CU));
+ if (unlikely (result->fake_loc_cu == NULL))
+ {
+ __libdw_free_zdata (result);
+ Dwarf_Sig8_Hash_free (&result->sig8_hash);
+ __libdw_seterrno (DWARF_E_NOMEM);
+ free (result);
+ result = NULL;
+ }
+ else
+ {
+ result->fake_loc_cu->dbg = result;
+ result->fake_loc_cu->startp
+ = result->sectiondata[IDX_debug_loc]->d_buf;
+ result->fake_loc_cu->endp
+ = (result->sectiondata[IDX_debug_loc]->d_buf
+ + result->sectiondata[IDX_debug_loc]->d_size);
+ }
+ }
+
return result;
}
diff --git a/libdw/dwarf_child.c b/libdw/dwarf_child.c
index 2a5d379..96d8e23 100644
--- a/libdw/dwarf_child.c
+++ b/libdw/dwarf_child.c
@@ -94,9 +94,7 @@ __libdw_find_attr (Dwarf_Die *die, unsigned int search_name,
/* Skip over the rest of this attribute (if there is any). */
if (attr_form != 0)
{
- size_t len = __libdw_form_val_len (dbg, die->cu, attr_form, readp,
- endp);
-
+ size_t len = __libdw_form_val_len (die->cu, attr_form, readp);
if (unlikely (len == (size_t) -1l))
{
readp = NULL;
diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c
index 647a1b8..922dc8f 100644
--- a/libdw/dwarf_end.c
+++ b/libdw/dwarf_end.c
@@ -117,6 +117,9 @@ dwarf_end (dwarf)
if (dwarf->free_elf)
elf_end (dwarf->elf);
+ /* Free the fake location list CU. */
+ free (dwarf->fake_loc_cu);
+
/* Free the context descriptor. */
free (dwarf);
}
diff --git a/libdw/dwarf_formblock.c b/libdw/dwarf_formblock.c
index 799d877..980bc10 100644
--- a/libdw/dwarf_formblock.c
+++ b/libdw/dwarf_formblock.c
@@ -1,5 +1,5 @@
/* Return block represented by attribute.
- Copyright (C) 2004-2010 Red Hat, Inc.
+ Copyright (C) 2004-2010, 2014 Red Hat, Inc.
This file is part of elfutils.
Written by Ulrich Drepper <drepper@redhat.com>, 2004.
@@ -43,28 +43,37 @@ dwarf_formblock (attr, return_block)
if (attr == NULL)
return -1;
- const unsigned char *datap;
+ const unsigned char *datap = attr->valp;
+ const unsigned char *endp = attr->cu->endp;
switch (attr->form)
{
case DW_FORM_block1:
+ if (unlikely (endp - datap < 1))
+ goto invalid;
return_block->length = *(uint8_t *) attr->valp;
return_block->data = attr->valp + 1;
break;
case DW_FORM_block2:
+ if (unlikely (endp - datap < 2))
+ goto invalid;
return_block->length = read_2ubyte_unaligned (attr->cu->dbg, attr->valp);
return_block->data = attr->valp + 2;
break;
case DW_FORM_block4:
+ if (unlikely (endp - datap < 4))
+ goto invalid;
return_block->length = read_4ubyte_unaligned (attr->cu->dbg, attr->valp);
return_block->data = attr->valp + 4;
break;
case DW_FORM_block:
case DW_FORM_exprloc:
- datap = attr->valp;
+ if (unlikely (endp - datap < 1))
+ goto invalid;
+ // XXX bounds check
get_uleb128 (return_block->length, datap);
return_block->data = (unsigned char *) datap;
break;
@@ -74,12 +83,10 @@ dwarf_formblock (attr, return_block)
return -1;
}
- if (unlikely (cu_data (attr->cu)->d_size
- - (return_block->data
- - (unsigned char *) cu_data (attr->cu)->d_buf)
- < return_block->length))
+ if (unlikely (return_block->length > (size_t) (endp - return_block->data)))
{
/* Block does not fit. */
+ invalid:
__libdw_seterrno (DWARF_E_INVALID_DWARF);
return -1;
}
diff --git a/libdw/dwarf_getattrs.c b/libdw/dwarf_getattrs.c
index 9ea70fc..0c54e5d 100644
--- a/libdw/dwarf_getattrs.c
+++ b/libdw/dwarf_getattrs.c
@@ -106,9 +106,7 @@ dwarf_getattrs (Dwarf_Die *die, int (*callback) (Dwarf_Attribute *, void *),
/* Skip over the rest of this attribute (if there is any). */
if (attr.form != 0)
{
- size_t len = __libdw_form_val_len (dbg, die->cu, attr.form,
- die_addr, endp);
-
+ size_t len = __libdw_form_val_len (die->cu, attr.form, die_addr);
if (unlikely (len == (size_t) -1l))
/* Something wrong with the file. */
return -1l;
diff --git a/libdw/dwarf_getlocation_attr.c b/libdw/dwarf_getlocation_attr.c
index cb29045..3229baf 100644
--- a/libdw/dwarf_getlocation_attr.c
+++ b/libdw/dwarf_getlocation_attr.c
@@ -1,5 +1,5 @@
/* Return DWARF attribute associated with a location expression op.
- Copyright (C) 2013 Red Hat, Inc.
+ Copyright (C) 2013, 2014 Red Hat, Inc.
This file is part of elfutils.
This file is free software; you can redistribute it and/or modify
@@ -33,6 +33,24 @@
#include <dwarf.h>
#include <libdwP.h>
+static Dwarf_CU *
+attr_form_cu (Dwarf_Attribute *attr)
+{
+ /* If the attribute has block/expr form the data comes from the
+ .debug_info from the same cu as the attr. Otherwise it comes from
+ the .debug_loc data section. */
+ switch (attr->form)
+ {
+ case DW_FORM_block1:
+ case DW_FORM_block2:
+ case DW_FORM_block4:
+ case DW_FORM_block:
+ case DW_FORM_exprloc:
+ return attr->cu;
+ default:
+ return attr->cu->dbg->fake_loc_cu;
+ }
+}
int
dwarf_getlocation_attr (attr, op, result)
@@ -43,26 +61,27 @@ dwarf_getlocation_attr (attr, op, result)
if (attr == NULL)
return -1;
- result->cu = attr->cu;
-
switch (op->atom)
{
case DW_OP_implicit_value:
result->code = DW_AT_const_value;
result->form = DW_FORM_block;
result->valp = (unsigned char *) (uintptr_t) op->number2;
+ result->cu = attr_form_cu (attr);
break;
case DW_OP_GNU_entry_value:
result->code = DW_AT_location;
result->form = DW_FORM_exprloc;
result->valp = (unsigned char *) (uintptr_t) op->number2;
+ result->cu = attr_form_cu (attr);
break;
case DW_OP_GNU_const_type:
result->code = DW_AT_const_value;
result->form = DW_FORM_block1;
result->valp = (unsigned char *) (uintptr_t) op->number2;
+ result->cu = attr_form_cu (attr);
break;
case DW_OP_call2:
@@ -74,7 +93,7 @@ dwarf_getlocation_attr (attr, op, result)
return -1;
if (INTUSE(dwarf_attr) (&die, DW_AT_location, result) == NULL)
{
- __libdw_empty_loc_attr (result, attr->cu);
+ __libdw_empty_loc_attr (result);
return 0;
}
}
@@ -88,7 +107,7 @@ dwarf_getlocation_attr (attr, op, result)
if (INTUSE(dwarf_attr) (&die, DW_AT_location, result) == NULL
&& INTUSE(dwarf_attr) (&die, DW_AT_const_value, result) == NULL)
{
- __libdw_empty_loc_attr (result, attr->cu);
+ __libdw_empty_loc_attr (result);
return 0;
}
}
diff --git a/libdw/dwarf_getlocation_implicit_pointer.c b/libdw/dwarf_getlocation_implicit_pointer.c
index f93d17e..f1c16be 100644
--- a/libdw/dwarf_getlocation_implicit_pointer.c
+++ b/libdw/dwarf_getlocation_implicit_pointer.c
@@ -35,15 +35,17 @@
static unsigned char empty_exprloc = 0;
+static Dwarf_CU empty_cu = { .startp = &empty_exprloc,
+ .endp = &empty_exprloc + 1 };
void
internal_function
-__libdw_empty_loc_attr (Dwarf_Attribute *attr, struct Dwarf_CU *cu )
+__libdw_empty_loc_attr (Dwarf_Attribute *attr)
{
attr->code = DW_AT_location;
attr->form = DW_FORM_exprloc;
attr->valp = &empty_exprloc;
- attr->cu = cu;
+ attr->cu = &empty_cu;
}
int
@@ -69,7 +71,7 @@ dwarf_getlocation_implicit_pointer (attr, op, result)
if (INTUSE(dwarf_attr) (&die, DW_AT_location, result) == NULL
&& INTUSE(dwarf_attr) (&die, DW_AT_const_value, result) == NULL)
{
- __libdw_empty_loc_attr (result, attr->cu);
+ __libdw_empty_loc_attr (result);
return 0;
}
diff --git a/libdw/dwarf_getmacros.c b/libdw/dwarf_getmacros.c
index 737dc5d..848128e 100644
--- a/libdw/dwarf_getmacros.c
+++ b/libdw/dwarf_getmacros.c
@@ -354,6 +354,8 @@ read_macros (Dwarf *dbg, int sec_index,
.dbg = dbg,
.version = 4,
.offset_size = table->is_64bit ? 8 : 4,
+ .startp = (void *) startp + offset,
+ .endp = (void *) endp,
};
Dwarf_Attribute attributes[proto->nforms];
@@ -367,8 +369,7 @@ read_macros (Dwarf *dbg, int sec_index,
attributes[i].valp = (void *) readp;
attributes[i].cu = &fake_cu;
- size_t len = __libdw_form_val_len (dbg, &fake_cu,
- proto->forms[i], readp, endp);
+ size_t len = __libdw_form_val_len (&fake_cu, proto->forms[i], readp);
if (len == (size_t) -1)
return -1;
diff --git a/libdw/libdwP.h b/libdw/libdwP.h
index edceb59..fb922f5 100644
--- a/libdw/libdwP.h
+++ b/libdw/libdwP.h
@@ -188,6 +188,10 @@ struct Dwarf
/* Cached info from the CFI section. */
struct Dwarf_CFI_s *cfi;
+ /* Fake loc CU. Used when synthesizing attributes for Dwarf_Ops that
+ came from a location list entry in dwarf_getlocation_attr. */
+ struct Dwarf_CU *fake_loc_cu;
+
/* Internal memory handling. This is basically a simplified
reimplementation of obstacks. Unfortunately the standard obstack
implementation is not usable in libraries. */
@@ -483,18 +487,16 @@ __libdw_dieabbrev (Dwarf_Die *die, const unsigned char **readp)
}
/* Helper functions for form handling. */
-extern size_t __libdw_form_val_compute_len (Dwarf *dbg, struct Dwarf_CU *cu,
+extern size_t __libdw_form_val_compute_len (struct Dwarf_CU *cu,
unsigned int form,
- const unsigned char *valp,
- const unsigned char *endp)
- __nonnull_attribute__ (1, 2, 4, 5) internal_function;
+ const unsigned char *valp)
+ __nonnull_attribute__ (1, 3) internal_function;
/* Find the length of a form attribute. */
static inline size_t
-__nonnull_attribute__ (1, 2, 4, 5)
-__libdw_form_val_len (Dwarf *dbg, struct Dwarf_CU *cu,
- unsigned int form, const unsigned char *valp,
- const unsigned char *endp)
+__nonnull_attribute__ (1, 3)
+__libdw_form_val_len (struct Dwarf_CU *cu, unsigned int form,
+ const unsigned char *valp)
{
/* Small lookup table of forms with fixed lengths. Absent indexes are
initialized 0, so any truly desired 0 is set to 0x80 and masked. */
@@ -513,6 +515,7 @@ __libdw_form_val_len (Dwarf *dbg, struct Dwarf_CU *cu,
uint8_t len = form_lengths[form];
if (len != 0)
{
+ const const unsigned char *endp = cu->endp;
len &= 0x7f; /* Mask to allow 0x80 -> 0. */
if (unlikely (len > (size_t) (endp - valp)))
{
@@ -524,7 +527,7 @@ __libdw_form_val_len (Dwarf *dbg, struct Dwarf_CU *cu,
}
/* Other forms require some computation. */
- return __libdw_form_val_compute_len (dbg, cu, form, valp, endp);
+ return __libdw_form_val_compute_len (cu, form, valp);
}
/* Helper function for DW_FORM_ref* handling. */
@@ -744,7 +747,7 @@ unsigned char * __libdw_formptr (Dwarf_Attribute *attr, int sec_index,
internal_function;
/* Fills in the given attribute to point at an empty location expression. */
-void __libdw_empty_loc_attr (Dwarf_Attribute *attr, struct Dwarf_CU *cu)
+void __libdw_empty_loc_attr (Dwarf_Attribute *attr)
internal_function;
/* Load .debug_line unit at DEBUG_LINE_OFFSET. COMP_DIR is a value of
diff --git a/libdw/libdw_form.c b/libdw/libdw_form.c
index 65ac7de..379ede5 100644
--- a/libdw/libdw_form.c
+++ b/libdw/libdw_form.c
@@ -39,11 +39,11 @@
size_t
internal_function
-__libdw_form_val_compute_len (Dwarf *dbg, struct Dwarf_CU *cu,
- unsigned int form, const unsigned char *valp,
- const unsigned char *endp)
+__libdw_form_val_compute_len (struct Dwarf_CU *cu, unsigned int form,
+ const unsigned char *valp)
{
const unsigned char *startp = valp;
+ const unsigned char *endp = cu->endp;
Dwarf_Word u128;
size_t result;
@@ -75,13 +75,13 @@ __libdw_form_val_compute_len (Dwarf *dbg, struct Dwarf_CU *cu,
case DW_FORM_block2:
if (unlikely ((size_t) (endp - startp) < 2))
goto invalid;
- result = read_2ubyte_unaligned (dbg, valp) + 2;
+ result = read_2ubyte_unaligned (cu->dbg, valp) + 2;
break;
case DW_FORM_block4:
if (unlikely ((size_t) (endp - startp) < 4))
goto invalid;
- result = read_4ubyte_unaligned (dbg, valp) + 4;
+ result = read_4ubyte_unaligned (cu->dbg, valp) + 4;
break;
case DW_FORM_block:
@@ -112,7 +112,7 @@ __libdw_form_val_compute_len (Dwarf *dbg, struct Dwarf_CU *cu,
case DW_FORM_indirect:
get_uleb128 (u128, valp);
// XXX Is this really correct?
- result = __libdw_form_val_len (dbg, cu, u128, valp, endp);
+ result = __libdw_form_val_len (cu, u128, valp);
if (result != (size_t) -1)
result += valp - startp;
else
--
1.8.3.1