The way it is currently done it is possible for read_symbols() to find the
same symbol as parent for ".cold" functions. This leads to a bunch of
complications such as func length being set to 0 and a segfault in
add_switch_table(). Fix by copying the search string instead of modifying
it in place.
Signed-off-by: Artem Savkov <[email protected]>
---
tools/objtool/elf.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 6dbb9fae0f9d..781c8afb29b9 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -298,6 +298,7 @@ static int read_symbols(struct elf *elf)
/* Create parent/child links for any cold subfunctions */
list_for_each_entry(sec, &elf->sections, list) {
list_for_each_entry(sym, &sec->symbol_list, list) {
+ char *pname;
if (sym->type != STT_FUNC)
continue;
sym->pfunc = sym->cfunc = sym;
@@ -305,9 +306,9 @@ static int read_symbols(struct elf *elf)
if (!coldstr)
continue;
- coldstr[0] = '\0';
- pfunc = find_symbol_by_name(elf, sym->name);
- coldstr[0] = '.';
+ pname = strndup(sym->name, coldstr - sym->name);
+ pfunc = find_symbol_by_name(elf, pname);
+ free(pname);
if (!pfunc) {
WARN("%s(): can't find parent function",
--
2.17.2
On Wed, Nov 07, 2018 at 03:05:59PM +0100, Artem Savkov wrote:
> The way it is currently done it is possible for read_symbols() to find the
> same symbol as parent for ".cold" functions.
I seem to remember having this discussion for kpatch-build, but I forget
the details. Can you explain how this can happen (and also add that
detail to the commit message)? I haven't seen any bug reports, is it
purely theoretical?
> This leads to a bunch of
> complications such as func length being set to 0 and a segfault in
> add_switch_table(). Fix by copying the search string instead of modifying
> it in place.
>
> Signed-off-by: Artem Savkov <[email protected]>
> ---
> tools/objtool/elf.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> index 6dbb9fae0f9d..781c8afb29b9 100644
> --- a/tools/objtool/elf.c
> +++ b/tools/objtool/elf.c
> @@ -298,6 +298,7 @@ static int read_symbols(struct elf *elf)
> /* Create parent/child links for any cold subfunctions */
> list_for_each_entry(sec, &elf->sections, list) {
> list_for_each_entry(sym, &sec->symbol_list, list) {
> + char *pname;
> if (sym->type != STT_FUNC)
> continue;
> sym->pfunc = sym->cfunc = sym;
> @@ -305,9 +306,9 @@ static int read_symbols(struct elf *elf)
> if (!coldstr)
> continue;
>
> - coldstr[0] = '\0';
> - pfunc = find_symbol_by_name(elf, sym->name);
> - coldstr[0] = '.';
> + pname = strndup(sym->name, coldstr - sym->name);
> + pfunc = find_symbol_by_name(elf, pname);
> + free(pname);
>
> if (!pfunc) {
> WARN("%s(): can't find parent function",
> --
> 2.17.2
>
--
Josh
On Wed, Nov 07, 2018 at 11:08:56AM -0600, Josh Poimboeuf wrote:
> On Wed, Nov 07, 2018 at 03:05:59PM +0100, Artem Savkov wrote:
> > The way it is currently done it is possible for read_symbols() to find the
> > same symbol as parent for ".cold" functions.
>
> I seem to remember having this discussion for kpatch-build, but I forget
> the details. Can you explain how this can happen (and also add that
> detail to the commit message)?
find_symbol_by_name() traverses the same lists as read_symbols and when
we change sym->name in place without copying it it changes in the list
as well. So if child function is before parent in sec->symbol_list the
same function will be returned as "parent". It is hard for me to put it
into words worthy to be included into commit message.
> I haven't seen any bug reports, is it purely theoretical?
No, 4.20-rc1 (actually anything after 4a60aa05a063 "objtool: Support
per-function rodata sections", before that add_switch_table() doesn't
seem to be called for those .cold. funcs) fails to build for mewith
KCFLAGS="-ffunction-sections -fdata-sections" because objtool is
segfaulting.
> > This leads to a bunch of
> > complications such as func length being set to 0 and a segfault in
> > add_switch_table(). Fix by copying the search string instead of modifying
> > it in place.
> >
> > Signed-off-by: Artem Savkov <[email protected]>
> > ---
> > tools/objtool/elf.c | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> > index 6dbb9fae0f9d..781c8afb29b9 100644
> > --- a/tools/objtool/elf.c
> > +++ b/tools/objtool/elf.c
> > @@ -298,6 +298,7 @@ static int read_symbols(struct elf *elf)
> > /* Create parent/child links for any cold subfunctions */
> > list_for_each_entry(sec, &elf->sections, list) {
> > list_for_each_entry(sym, &sec->symbol_list, list) {
> > + char *pname;
> > if (sym->type != STT_FUNC)
> > continue;
> > sym->pfunc = sym->cfunc = sym;
> > @@ -305,9 +306,9 @@ static int read_symbols(struct elf *elf)
> > if (!coldstr)
> > continue;
> >
> > - coldstr[0] = '\0';
> > - pfunc = find_symbol_by_name(elf, sym->name);
> > - coldstr[0] = '.';
> > + pname = strndup(sym->name, coldstr - sym->name);
> > + pfunc = find_symbol_by_name(elf, pname);
> > + free(pname);
> >
> > if (!pfunc) {
> > WARN("%s(): can't find parent function",
> > --
> > 2.17.2
> >
>
> --
> Josh
--
Regards,
Artem
On Wed, Nov 07, 2018 at 07:42:46PM +0100, Artem Savkov wrote:
> On Wed, Nov 07, 2018 at 11:08:56AM -0600, Josh Poimboeuf wrote:
> > On Wed, Nov 07, 2018 at 03:05:59PM +0100, Artem Savkov wrote:
> > > The way it is currently done it is possible for read_symbols() to find the
> > > same symbol as parent for ".cold" functions.
> >
> > I seem to remember having this discussion for kpatch-build, but I forget
> > the details. Can you explain how this can happen (and also add that
> > detail to the commit message)?
>
> find_symbol_by_name() traverses the same lists as read_symbols and when
> we change sym->name in place without copying it it changes in the list
> as well. So if child function is before parent in sec->symbol_list the
> same function will be returned as "parent".
Ah, I see.
> It is hard for me to put it into words worthy to be included into
> commit message.
The above description made sense to me, so it sounds like you're on the
right path :-)
> > I haven't seen any bug reports, is it purely theoretical?
>
> No, 4.20-rc1 (actually anything after 4a60aa05a063 "objtool: Support
> per-function rodata sections", before that add_switch_table() doesn't
> seem to be called for those .cold. funcs) fails to build for mewith
> KCFLAGS="-ffunction-sections -fdata-sections" because objtool is
> segfaulting.
If you only see it triggered with -ffunction-sections, that would be
another useful nugget for the commit log.
Also, if it's fixing a regression, be sure to add the 'Fixes' tag.
Thanks!
--
Josh
Because find_symbol_by_name() traverses the same lists as read_symbols()
changing sym->name in place without copying it affects the result of
find_symbol_by_name() and, in case when ".cold" function precedes it's
parent in sec->symbol_list, can result in function being considered a
parent of itself. This leads to function length being set to 0 and other
consequent side-effects including a segfault in add_switch_table().
The effects of this bug are only visible when building with
-ffunction-sections in KCFLAGS.
Fix by copying the search string instead of modifying it in place.
Signed-off-by: Artem Savkov <[email protected]>
---
tools/objtool/elf.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 6dbb9fae0f9d..781c8afb29b9 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -298,6 +298,7 @@ static int read_symbols(struct elf *elf)
/* Create parent/child links for any cold subfunctions */
list_for_each_entry(sec, &elf->sections, list) {
list_for_each_entry(sym, &sec->symbol_list, list) {
+ char *pname;
if (sym->type != STT_FUNC)
continue;
sym->pfunc = sym->cfunc = sym;
@@ -305,9 +306,9 @@ static int read_symbols(struct elf *elf)
if (!coldstr)
continue;
- coldstr[0] = '\0';
- pfunc = find_symbol_by_name(elf, sym->name);
- coldstr[0] = '.';
+ pname = strndup(sym->name, coldstr - sym->name);
+ pfunc = find_symbol_by_name(elf, pname);
+ free(pname);
if (!pfunc) {
WARN("%s(): can't find parent function",
--
2.17.2
On Wed, Nov 07, 2018 at 10:45:15PM +0100, Artem Savkov wrote:
> Because find_symbol_by_name() traverses the same lists as read_symbols()
> changing sym->name in place without copying it affects the result of
> find_symbol_by_name() and, in case when ".cold" function precedes it's
> parent in sec->symbol_list, can result in function being considered a
> parent of itself. This leads to function length being set to 0 and other
> consequent side-effects including a segfault in add_switch_table().
> The effects of this bug are only visible when building with
> -ffunction-sections in KCFLAGS.
>
> Fix by copying the search string instead of modifying it in place.
>
> Signed-off-by: Artem Savkov <[email protected]>
This needs a "Fixes" tag to identify the patch which introduced the bug.
> ---
> tools/objtool/elf.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> index 6dbb9fae0f9d..781c8afb29b9 100644
> --- a/tools/objtool/elf.c
> +++ b/tools/objtool/elf.c
> @@ -298,6 +298,7 @@ static int read_symbols(struct elf *elf)
> /* Create parent/child links for any cold subfunctions */
> list_for_each_entry(sec, &elf->sections, list) {
> list_for_each_entry(sym, &sec->symbol_list, list) {
> + char *pname;
> if (sym->type != STT_FUNC)
> continue;
> sym->pfunc = sym->cfunc = sym;
> @@ -305,9 +306,9 @@ static int read_symbols(struct elf *elf)
> if (!coldstr)
> continue;
>
> - coldstr[0] = '\0';
> - pfunc = find_symbol_by_name(elf, sym->name);
> - coldstr[0] = '.';
> + pname = strndup(sym->name, coldstr - sym->name);
> + pfunc = find_symbol_by_name(elf, pname);
> + free(pname);
>
> if (!pfunc) {
> WARN("%s(): can't find parent function",
strndup()'s return code needs to be checked.
Also, for such a short-lived allocation, I think a stack-allocated
string would be better.
--
Josh
On Fri, Nov 09, 2018 at 11:23:09AM -0600, Josh Poimboeuf wrote:
> On Wed, Nov 07, 2018 at 10:45:15PM +0100, Artem Savkov wrote:
> > Because find_symbol_by_name() traverses the same lists as read_symbols()
> > changing sym->name in place without copying it affects the result of
> > find_symbol_by_name() and, in case when ".cold" function precedes it's
> > parent in sec->symbol_list, can result in function being considered a
> > parent of itself. This leads to function length being set to 0 and other
> > consequent side-effects including a segfault in add_switch_table().
> > The effects of this bug are only visible when building with
> > -ffunction-sections in KCFLAGS.
> >
> > Fix by copying the search string instead of modifying it in place.
> >
> > Signed-off-by: Artem Savkov <[email protected]>
>
> This needs a "Fixes" tag to identify the patch which introduced the bug.
Ok, will do.
> > ---
> > tools/objtool/elf.c | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> > index 6dbb9fae0f9d..781c8afb29b9 100644
> > --- a/tools/objtool/elf.c
> > +++ b/tools/objtool/elf.c
> > @@ -298,6 +298,7 @@ static int read_symbols(struct elf *elf)
> > /* Create parent/child links for any cold subfunctions */
> > list_for_each_entry(sec, &elf->sections, list) {
> > list_for_each_entry(sym, &sec->symbol_list, list) {
> > + char *pname;
> > if (sym->type != STT_FUNC)
> > continue;
> > sym->pfunc = sym->cfunc = sym;
> > @@ -305,9 +306,9 @@ static int read_symbols(struct elf *elf)
> > if (!coldstr)
> > continue;
> >
> > - coldstr[0] = '\0';
> > - pfunc = find_symbol_by_name(elf, sym->name);
> > - coldstr[0] = '.';
> > + pname = strndup(sym->name, coldstr - sym->name);
> > + pfunc = find_symbol_by_name(elf, pname);
> > + free(pname);
> >
> > if (!pfunc) {
> > WARN("%s(): can't find parent function",
>
> strndup()'s return code needs to be checked.
>
> Also, for such a short-lived allocation, I think a stack-allocated
> string would be better.
Hm, there seems to be no limit on lengths of strings in string table.
What size would you consider reasonable for this stack-allocated string?
--
Regards,
Artem
On Sat, Nov 10, 2018 at 01:18:36PM +0100, Artem Savkov wrote:
> On Fri, Nov 09, 2018 at 11:23:09AM -0600, Josh Poimboeuf wrote:
> > On Wed, Nov 07, 2018 at 10:45:15PM +0100, Artem Savkov wrote:
> > > Because find_symbol_by_name() traverses the same lists as read_symbols()
> > > changing sym->name in place without copying it affects the result of
> > > find_symbol_by_name() and, in case when ".cold" function precedes it's
> > > parent in sec->symbol_list, can result in function being considered a
> > > parent of itself. This leads to function length being set to 0 and other
> > > consequent side-effects including a segfault in add_switch_table().
> > > The effects of this bug are only visible when building with
> > > -ffunction-sections in KCFLAGS.
> > >
> > > Fix by copying the search string instead of modifying it in place.
> > >
> > > Signed-off-by: Artem Savkov <[email protected]>
> >
> > This needs a "Fixes" tag to identify the patch which introduced the bug.
>
> Ok, will do.
>
> > > ---
> > > tools/objtool/elf.c | 7 ++++---
> > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> > > index 6dbb9fae0f9d..781c8afb29b9 100644
> > > --- a/tools/objtool/elf.c
> > > +++ b/tools/objtool/elf.c
> > > @@ -298,6 +298,7 @@ static int read_symbols(struct elf *elf)
> > > /* Create parent/child links for any cold subfunctions */
> > > list_for_each_entry(sec, &elf->sections, list) {
> > > list_for_each_entry(sym, &sec->symbol_list, list) {
> > > + char *pname;
> > > if (sym->type != STT_FUNC)
> > > continue;
> > > sym->pfunc = sym->cfunc = sym;
> > > @@ -305,9 +306,9 @@ static int read_symbols(struct elf *elf)
> > > if (!coldstr)
> > > continue;
> > >
> > > - coldstr[0] = '\0';
> > > - pfunc = find_symbol_by_name(elf, sym->name);
> > > - coldstr[0] = '.';
> > > + pname = strndup(sym->name, coldstr - sym->name);
> > > + pfunc = find_symbol_by_name(elf, pname);
> > > + free(pname);
> > >
> > > if (!pfunc) {
> > > WARN("%s(): can't find parent function",
> >
> > strndup()'s return code needs to be checked.
> >
> > Also, for such a short-lived allocation, I think a stack-allocated
> > string would be better.
>
> Hm, there seems to be no limit on lengths of strings in string table.
> What size would you consider reasonable for this stack-allocated string?
I think it's fine to pick a reasonable maximum (128?) and then verify
the string fits in the array before copying it.
--
Josh
The series started with 'parent symbol search' patch, but I found another issue
in read_symbols() while testing the failure-path.
Artem Savkov (2):
objtool: fix failed cold symbol doublefree
objtool: fix .cold functions parent symbols search
tools/objtool/elf.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
--
2.17.2
Because find_symbol_by_name() traverses the same lists as read_symbols()
changing sym->name in place without copying it affects the result of
find_symbol_by_name() and, in case when ".cold" function precedes it's
parent in sec->symbol_list, can result in function being considered a
parent of itself. This leads to function length being set to 0 and other
consequent side-effects including a segfault in add_switch_table().
The effects of this bug are only visible when building with
-ffunction-sections in KCFLAGS.
Fix by copying the search string instead of modifying it in place.
Fixes: 13810435b9a7 "objtool: Support GCC 8's cold subfunctions"
Signed-off-by: Artem Savkov <[email protected]>
---
tools/objtool/elf.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 3decd43477df..15d9acfb2c97 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -31,6 +31,8 @@
#include "elf.h"
#include "warn.h"
+#define MAX_NAME_LEN 128
+
struct section *find_section_by_name(struct elf *elf, const char *name)
{
struct section *sec;
@@ -298,6 +300,8 @@ static int read_symbols(struct elf *elf)
/* Create parent/child links for any cold subfunctions */
list_for_each_entry(sec, &elf->sections, list) {
list_for_each_entry(sym, &sec->symbol_list, list) {
+ char pname[MAX_NAME_LEN + 1];
+ size_t pnamelen;
if (sym->type != STT_FUNC)
continue;
sym->pfunc = sym->cfunc = sym;
@@ -305,9 +309,16 @@ static int read_symbols(struct elf *elf)
if (!coldstr)
continue;
- coldstr[0] = '\0';
- pfunc = find_symbol_by_name(elf, sym->name);
- coldstr[0] = '.';
+ pnamelen = coldstr - sym->name;
+ if (pnamelen > MAX_NAME_LEN) {
+ WARN("%s(): parent function name exceeds maximum length of %d characters",
+ sym->name, MAX_NAME_LEN);
+ goto cold_err;
+ }
+
+ strncpy(pname, sym->name, pnamelen);
+ pname[pnamelen] = '\0';
+ pfunc = find_symbol_by_name(elf, pname);
if (!pfunc) {
WARN("%s(): can't find parent function",
--
2.17.2
If read_symbols() fails during second list traversal (the one dealing
with ".cold" subfunctions) it frees the symbol, but never deletes it
from the list/hash_table resulting in symbol being freed again in
elf_close().
Fixes: 13810435b9a7 "objtool: Support GCC 8's cold subfunctions"
Signed-off-by: Artem Savkov <[email protected]>
---
tools/objtool/elf.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 6dbb9fae0f9d..3decd43477df 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -312,7 +312,7 @@ static int read_symbols(struct elf *elf)
if (!pfunc) {
WARN("%s(): can't find parent function",
sym->name);
- goto err;
+ goto cold_err;
}
sym->pfunc = pfunc;
@@ -336,6 +336,9 @@ static int read_symbols(struct elf *elf)
return 0;
+cold_err:
+ list_del(&sym->list);
+ hash_del(&sym->hash);
err:
free(sym);
return -1;
--
2.17.2
On Mon, Nov 12, 2018 at 01:55:18PM +0100, Artem Savkov wrote:
> If read_symbols() fails during second list traversal (the one dealing
> with ".cold" subfunctions) it frees the symbol, but never deletes it
> from the list/hash_table resulting in symbol being freed again in
> elf_close().
>
> Fixes: 13810435b9a7 "objtool: Support GCC 8's cold subfunctions"
This needs parentheses, like:
Fixes: 13810435b9a7 ("objtool: Support GCC 8's cold subfunctions")
> Signed-off-by: Artem Savkov <[email protected]>
> ---
> tools/objtool/elf.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> index 6dbb9fae0f9d..3decd43477df 100644
> --- a/tools/objtool/elf.c
> +++ b/tools/objtool/elf.c
> @@ -312,7 +312,7 @@ static int read_symbols(struct elf *elf)
> if (!pfunc) {
> WARN("%s(): can't find parent function",
> sym->name);
> - goto err;
> + goto cold_err;
Since it will get freed properly in elf_close() anyway, maybe it would
be simpler to just 'return -1' here.
> }
>
> sym->pfunc = pfunc;
> @@ -336,6 +336,9 @@ static int read_symbols(struct elf *elf)
>
> return 0;
>
> +cold_err:
> + list_del(&sym->list);
> + hash_del(&sym->hash);
> err:
> free(sym);
> return -1;
> --
> 2.17.2
>
--
Josh
On Mon, Nov 12, 2018 at 01:55:19PM +0100, Artem Savkov wrote:
> Because find_symbol_by_name() traverses the same lists as read_symbols()
> changing sym->name in place without copying it affects the result of
> find_symbol_by_name() and, in case when ".cold" function precedes it's
> parent in sec->symbol_list, can result in function being considered a
> parent of itself. This leads to function length being set to 0 and other
> consequent side-effects including a segfault in add_switch_table().
> The effects of this bug are only visible when building with
> -ffunction-sections in KCFLAGS.
>
> Fix by copying the search string instead of modifying it in place.
>
> Fixes: 13810435b9a7 "objtool: Support GCC 8's cold subfunctions"
Same here, needs parentheses.
> Signed-off-by: Artem Savkov <[email protected]>
> ---
> tools/objtool/elf.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> index 3decd43477df..15d9acfb2c97 100644
> --- a/tools/objtool/elf.c
> +++ b/tools/objtool/elf.c
> @@ -31,6 +31,8 @@
> #include "elf.h"
> #include "warn.h"
>
> +#define MAX_NAME_LEN 128
> +
> struct section *find_section_by_name(struct elf *elf, const char *name)
> {
> struct section *sec;
> @@ -298,6 +300,8 @@ static int read_symbols(struct elf *elf)
> /* Create parent/child links for any cold subfunctions */
> list_for_each_entry(sec, &elf->sections, list) {
> list_for_each_entry(sym, &sec->symbol_list, list) {
> + char pname[MAX_NAME_LEN + 1];
> + size_t pnamelen;
> if (sym->type != STT_FUNC)
> continue;
> sym->pfunc = sym->cfunc = sym;
> @@ -305,9 +309,16 @@ static int read_symbols(struct elf *elf)
> if (!coldstr)
> continue;
>
> - coldstr[0] = '\0';
> - pfunc = find_symbol_by_name(elf, sym->name);
> - coldstr[0] = '.';
> + pnamelen = coldstr - sym->name;
> + if (pnamelen > MAX_NAME_LEN) {
> + WARN("%s(): parent function name exceeds maximum length of %d characters",
> + sym->name, MAX_NAME_LEN);
> + goto cold_err;
Same here, makes more sense to just return -1 and let elf_close() clean
it up I think.
--
Josh
If read_symbols() fails during second list traversal (the one dealing
with ".cold" subfunctions) it frees the symbol, but never deletes it
from the list/hash_table resulting in symbol being freed again in
elf_close(). Fix by just returning an error leaving cleanup to
elf_close().
Fixes: 13810435b9a7 ("objtool: Support GCC 8's cold subfunctions")
Signed-off-by: Artem Savkov <[email protected]>
---
tools/objtool/elf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 6dbb9fae0f9d..e7a7ac40e045 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -312,7 +312,7 @@ static int read_symbols(struct elf *elf)
if (!pfunc) {
WARN("%s(): can't find parent function",
sym->name);
- goto err;
+ return -1;
}
sym->pfunc = pfunc;
--
2.19.1
Because find_symbol_by_name() traverses the same lists as read_symbols()
changing sym->name in place without copying it affects the result of
find_symbol_by_name() and, in case when ".cold" function precedes it's
parent in sec->symbol_list, can result in function being considered a
parent of itself. This leads to function length being set to 0 and other
consequent side-effects including a segfault in add_switch_table().
The effects of this bug are only visible when building with
-ffunction-sections in KCFLAGS.
Fix by copying the search string instead of modifying it in place.
Fixes: 13810435b9a7 ("objtool: Support GCC 8's cold subfunctions")
Signed-off-by: Artem Savkov <[email protected]>
---
tools/objtool/elf.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index e7a7ac40e045..b8f3cca8e58b 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -31,6 +31,8 @@
#include "elf.h"
#include "warn.h"
+#define MAX_NAME_LEN 128
+
struct section *find_section_by_name(struct elf *elf, const char *name)
{
struct section *sec;
@@ -298,6 +300,8 @@ static int read_symbols(struct elf *elf)
/* Create parent/child links for any cold subfunctions */
list_for_each_entry(sec, &elf->sections, list) {
list_for_each_entry(sym, &sec->symbol_list, list) {
+ char pname[MAX_NAME_LEN + 1];
+ size_t pnamelen;
if (sym->type != STT_FUNC)
continue;
sym->pfunc = sym->cfunc = sym;
@@ -305,9 +309,16 @@ static int read_symbols(struct elf *elf)
if (!coldstr)
continue;
- coldstr[0] = '\0';
- pfunc = find_symbol_by_name(elf, sym->name);
- coldstr[0] = '.';
+ pnamelen = coldstr - sym->name;
+ if (pnamelen > MAX_NAME_LEN) {
+ WARN("%s(): parent function name exceeds maximum length of %d characters",
+ sym->name, MAX_NAME_LEN);
+ return -1;
+ }
+
+ strncpy(pname, sym->name, pnamelen);
+ pname[pnamelen] = '\0';
+ pfunc = find_symbol_by_name(elf, pname);
if (!pfunc) {
WARN("%s(): can't find parent function",
--
2.19.1
The series started with 'parent symbol search' patch, but I found another issue
in read_symbols() while testing the failure-path.
Artem Savkov (2):
objtool: fix failed cold symbol doublefree
objtool: fix .cold functions parent symbols search
tools/objtool/elf.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
--
2.19.1
On Tue, Nov 20, 2018 at 09:05:46AM +0100, Artem Savkov wrote:
> The series started with 'parent symbol search' patch, but I found another issue
> in read_symbols() while testing the failure-path.
>
> Artem Savkov (2):
> objtool: fix failed cold symbol doublefree
> objtool: fix .cold functions parent symbols search
>
> tools/objtool/elf.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
Thanks, looking good. I'll run the patches through 0-day testing and
then send them onto the -tip tree.
--
Josh