* [PATCH] objtool: fix .cold. functions parent symbols search @ 2018-11-07 14:05 Artem Savkov 2018-11-07 17:08 ` Josh Poimboeuf 0 siblings, 1 reply; 17+ messages in thread From: Artem Savkov @ 2018-11-07 14:05 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: Peter Zijlstra, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] objtool: fix .cold. functions parent symbols search 2018-11-07 14:05 [PATCH] objtool: fix .cold. functions parent symbols search Artem Savkov @ 2018-11-07 17:08 ` Josh Poimboeuf 2018-11-07 18:42 ` Artem Savkov 0 siblings, 1 reply; 17+ messages in thread From: Josh Poimboeuf @ 2018-11-07 17:08 UTC (permalink / raw) To: Artem Savkov; +Cc: Peter Zijlstra, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] objtool: fix .cold. functions parent symbols search 2018-11-07 17:08 ` Josh Poimboeuf @ 2018-11-07 18:42 ` Artem Savkov 2018-11-07 20:29 ` Josh Poimboeuf 0 siblings, 1 reply; 17+ messages in thread From: Artem Savkov @ 2018-11-07 18:42 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: Peter Zijlstra, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] objtool: fix .cold. functions parent symbols search 2018-11-07 18:42 ` Artem Savkov @ 2018-11-07 20:29 ` Josh Poimboeuf 2018-11-07 21:45 ` [PATCH v2] " Artem Savkov 0 siblings, 1 reply; 17+ messages in thread From: Josh Poimboeuf @ 2018-11-07 20:29 UTC (permalink / raw) To: Artem Savkov; +Cc: Peter Zijlstra, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2] objtool: fix .cold. functions parent symbols search 2018-11-07 20:29 ` Josh Poimboeuf @ 2018-11-07 21:45 ` Artem Savkov 2018-11-09 17:23 ` Josh Poimboeuf 0 siblings, 1 reply; 17+ messages in thread From: Artem Savkov @ 2018-11-07 21:45 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: Peter Zijlstra, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2] objtool: fix .cold. functions parent symbols search 2018-11-07 21:45 ` [PATCH v2] " Artem Savkov @ 2018-11-09 17:23 ` Josh Poimboeuf 2018-11-10 12:18 ` Artem Savkov 0 siblings, 1 reply; 17+ messages in thread From: Josh Poimboeuf @ 2018-11-09 17:23 UTC (permalink / raw) To: Artem Savkov; +Cc: Peter Zijlstra, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] objtool: fix .cold. functions parent symbols search 2018-11-09 17:23 ` Josh Poimboeuf @ 2018-11-10 12:18 ` Artem Savkov 2018-11-12 3:38 ` Josh Poimboeuf 0 siblings, 1 reply; 17+ messages in thread From: Artem Savkov @ 2018-11-10 12:18 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: Peter Zijlstra, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] objtool: fix .cold. functions parent symbols search 2018-11-10 12:18 ` Artem Savkov @ 2018-11-12 3:38 ` Josh Poimboeuf 2018-11-12 12:55 ` [PATCH v3 0/2] objtool: read_symbols() fixes Artem Savkov 0 siblings, 1 reply; 17+ messages in thread From: Josh Poimboeuf @ 2018-11-12 3:38 UTC (permalink / raw) To: Artem Savkov; +Cc: Peter Zijlstra, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 0/2] objtool: read_symbols() fixes 2018-11-12 3:38 ` Josh Poimboeuf @ 2018-11-12 12:55 ` Artem Savkov 2018-11-12 12:55 ` [PATCH v3 1/2] objtool: fix failed cold symbol doublefree Artem Savkov ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Artem Savkov @ 2018-11-12 12:55 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: Peter Zijlstra, linux-kernel, 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 1/2] objtool: fix failed cold symbol doublefree 2018-11-12 12:55 ` [PATCH v3 0/2] objtool: read_symbols() fixes Artem Savkov @ 2018-11-12 12:55 ` Artem Savkov 2018-11-19 17:57 ` Josh Poimboeuf 2018-11-12 12:55 ` [PATCH v3 2/2] objtool: fix .cold functions parent symbols search Artem Savkov 2018-11-20 8:05 ` [PATCH v4 0/2] objtool: read_symbols() fixes Artem Savkov 2 siblings, 1 reply; 17+ messages in thread From: Artem Savkov @ 2018-11-12 12:55 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: Peter Zijlstra, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/2] objtool: fix failed cold symbol doublefree 2018-11-12 12:55 ` [PATCH v3 1/2] objtool: fix failed cold symbol doublefree Artem Savkov @ 2018-11-19 17:57 ` Josh Poimboeuf 0 siblings, 0 replies; 17+ messages in thread From: Josh Poimboeuf @ 2018-11-19 17:57 UTC (permalink / raw) To: Artem Savkov; +Cc: Peter Zijlstra, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 2/2] objtool: fix .cold functions parent symbols search 2018-11-12 12:55 ` [PATCH v3 0/2] objtool: read_symbols() fixes Artem Savkov 2018-11-12 12:55 ` [PATCH v3 1/2] objtool: fix failed cold symbol doublefree Artem Savkov @ 2018-11-12 12:55 ` Artem Savkov 2018-11-19 18:02 ` Josh Poimboeuf 2018-11-20 8:05 ` [PATCH v4 0/2] objtool: read_symbols() fixes Artem Savkov 2 siblings, 1 reply; 17+ messages in thread From: Artem Savkov @ 2018-11-12 12:55 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: Peter Zijlstra, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] objtool: fix .cold functions parent symbols search 2018-11-12 12:55 ` [PATCH v3 2/2] objtool: fix .cold functions parent symbols search Artem Savkov @ 2018-11-19 18:02 ` Josh Poimboeuf 0 siblings, 0 replies; 17+ messages in thread From: Josh Poimboeuf @ 2018-11-19 18:02 UTC (permalink / raw) To: Artem Savkov; +Cc: Peter Zijlstra, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4 0/2] objtool: read_symbols() fixes 2018-11-12 12:55 ` [PATCH v3 0/2] objtool: read_symbols() fixes Artem Savkov 2018-11-12 12:55 ` [PATCH v3 1/2] objtool: fix failed cold symbol doublefree Artem Savkov 2018-11-12 12:55 ` [PATCH v3 2/2] objtool: fix .cold functions parent symbols search Artem Savkov @ 2018-11-20 8:05 ` Artem Savkov 2018-11-20 8:05 ` [PATCH v4 1/2] objtool: fix failed cold symbol doublefree Artem Savkov ` (2 more replies) 2 siblings, 3 replies; 17+ messages in thread From: Artem Savkov @ 2018-11-20 8:05 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: Peter Zijlstra, linux-kernel, 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4 1/2] objtool: fix failed cold symbol doublefree 2018-11-20 8:05 ` [PATCH v4 0/2] objtool: read_symbols() fixes Artem Savkov @ 2018-11-20 8:05 ` Artem Savkov 2018-11-20 8:05 ` [PATCH v4 2/2] objtool: fix .cold functions parent symbols search Artem Savkov 2018-11-20 14:12 ` [PATCH v4 0/2] objtool: read_symbols() fixes Josh Poimboeuf 2 siblings, 0 replies; 17+ messages in thread From: Artem Savkov @ 2018-11-20 8:05 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: Peter Zijlstra, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 2/2] objtool: fix .cold functions parent symbols search 2018-11-20 8:05 ` [PATCH v4 0/2] objtool: read_symbols() fixes Artem Savkov 2018-11-20 8:05 ` [PATCH v4 1/2] objtool: fix failed cold symbol doublefree Artem Savkov @ 2018-11-20 8:05 ` Artem Savkov 2018-11-20 14:12 ` [PATCH v4 0/2] objtool: read_symbols() fixes Josh Poimboeuf 2 siblings, 0 replies; 17+ messages in thread From: Artem Savkov @ 2018-11-20 8:05 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: Peter Zijlstra, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v4 0/2] objtool: read_symbols() fixes 2018-11-20 8:05 ` [PATCH v4 0/2] objtool: read_symbols() fixes Artem Savkov 2018-11-20 8:05 ` [PATCH v4 1/2] objtool: fix failed cold symbol doublefree Artem Savkov 2018-11-20 8:05 ` [PATCH v4 2/2] objtool: fix .cold functions parent symbols search Artem Savkov @ 2018-11-20 14:12 ` Josh Poimboeuf 2 siblings, 0 replies; 17+ messages in thread From: Josh Poimboeuf @ 2018-11-20 14:12 UTC (permalink / raw) To: Artem Savkov; +Cc: Peter Zijlstra, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2018-11-20 14:12 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-07 14:05 [PATCH] objtool: fix .cold. functions parent symbols search Artem Savkov 2018-11-07 17:08 ` Josh Poimboeuf 2018-11-07 18:42 ` Artem Savkov 2018-11-07 20:29 ` Josh Poimboeuf 2018-11-07 21:45 ` [PATCH v2] " Artem Savkov 2018-11-09 17:23 ` Josh Poimboeuf 2018-11-10 12:18 ` Artem Savkov 2018-11-12 3:38 ` Josh Poimboeuf 2018-11-12 12:55 ` [PATCH v3 0/2] objtool: read_symbols() fixes Artem Savkov 2018-11-12 12:55 ` [PATCH v3 1/2] objtool: fix failed cold symbol doublefree Artem Savkov 2018-11-19 17:57 ` Josh Poimboeuf 2018-11-12 12:55 ` [PATCH v3 2/2] objtool: fix .cold functions parent symbols search Artem Savkov 2018-11-19 18:02 ` Josh Poimboeuf 2018-11-20 8:05 ` [PATCH v4 0/2] objtool: read_symbols() fixes Artem Savkov 2018-11-20 8:05 ` [PATCH v4 1/2] objtool: fix failed cold symbol doublefree Artem Savkov 2018-11-20 8:05 ` [PATCH v4 2/2] objtool: fix .cold functions parent symbols search Artem Savkov 2018-11-20 14:12 ` [PATCH v4 0/2] objtool: read_symbols() fixes Josh Poimboeuf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).