[PATCH] objtool: fix .cold. functions parent symbols search

[PATCH] objtool: fix .cold. functions parent symbols search

[Artem Savkov]

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 <asavkov@redhat.com>
---
 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

Re: [PATCH] objtool: fix .cold. functions parent symbols search

[Josh Poimboeuf]

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 <asavkov@redhat.com>
> ---
>  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

Re: [PATCH] objtool: fix .cold. functions parent symbols search

[Artem Savkov]

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 <asavkov@redhat.com>
> > ---
> >  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

Re: [PATCH] objtool: fix .cold. functions parent symbols search

[Josh Poimboeuf]

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

[PATCH v2] objtool: fix .cold. functions parent symbols search

[Artem Savkov]

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 <asavkov@redhat.com>
---
 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

Re: [PATCH v2] objtool: fix .cold. functions parent symbols search

[Josh Poimboeuf]

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 <asavkov@redhat.com>

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

Re: [PATCH v2] objtool: fix .cold. functions parent symbols search

[Artem Savkov]

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 <asavkov@redhat.com>
> 
> 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

Re: [PATCH v2] objtool: fix .cold. functions parent symbols search

[Josh Poimboeuf]

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 <asavkov@redhat.com>
> > 
> > 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

[PATCH v3 0/2] objtool: read_symbols() fixes

[Artem Savkov]

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

[PATCH v3 1/2] objtool: fix failed cold symbol doublefree

[Artem Savkov]

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 <asavkov@redhat.com>
---
 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

[PATCH v3 2/2] objtool: fix .cold functions parent symbols search

[Artem Savkov]

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 <asavkov@redhat.com>
---
 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

Re: [PATCH v3 1/2] objtool: fix failed cold symbol doublefree

[Josh Poimboeuf]

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 <asavkov@redhat.com>
> ---
>  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

Re: [PATCH v3 2/2] objtool: fix .cold functions parent symbols search

[Josh Poimboeuf]

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 <asavkov@redhat.com>
> ---
>  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

[PATCH v4 0/2] objtool: read_symbols() fixes

[Artem Savkov]

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

[PATCH v4 1/2] objtool: fix failed cold symbol doublefree

[Artem Savkov]

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 <asavkov@redhat.com>
---
 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

[PATCH v4 2/2] objtool: fix .cold functions parent symbols search

[Artem Savkov]

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 <asavkov@redhat.com>
---
 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

Re: [PATCH v4 0/2] objtool: read_symbols() fixes

[Josh Poimboeuf]

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