From a4037ce51ea452b8d59debfcf1ef70706e5ae69d Mon Sep 17 00:00:00 2001 From: Ben Hutchings Date: Tue, 20 Mar 2012 05:03:49 +0000 Subject: [PATCH] Apply upstream changes to fix various buffer overflow bugs svn path=/dists/sid/linux-tools/; revision=18864 --- debian/changelog | 8 + ...rect-use-of-snprintf-results-in-segv.patch | 64 +++++ ...tools-use-scnprintf-where-applicable.patch | 267 ++++++++++++++++++ debian/patches/series | 2 + 4 files changed, 341 insertions(+) create mode 100644 debian/patches/perf-tools-incorrect-use-of-snprintf-results-in-segv.patch create mode 100644 debian/patches/perf-tools-use-scnprintf-where-applicable.patch diff --git a/debian/changelog b/debian/changelog index 5fd778318..2bd4a00b8 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,11 @@ +linux-tools (3.2.7-2) unstable; urgency=high + + * Apply upstream changes to fix various buffer overflow bugs: + - perf tools: Use scnprintf where applicable + - perf tools: Incorrect use of snprintf results in SEGV + + -- Ben Hutchings Tue, 20 Mar 2012 04:54:22 +0000 + linux-tools (3.2.7-1) unstable; urgency=low * New upstream stable update: diff --git a/debian/patches/perf-tools-incorrect-use-of-snprintf-results-in-segv.patch b/debian/patches/perf-tools-incorrect-use-of-snprintf-results-in-segv.patch new file mode 100644 index 000000000..ed0930f9b --- /dev/null +++ b/debian/patches/perf-tools-incorrect-use-of-snprintf-results-in-segv.patch @@ -0,0 +1,64 @@ +From b832796caa1fda8516464a003c8c7cc547bc20c2 Mon Sep 17 00:00:00 2001 +From: Anton Blanchard +Date: Wed, 7 Mar 2012 11:42:49 +1100 +Subject: perf tools: Incorrect use of snprintf results in SEGV + +From: Anton Blanchard + +commit b832796caa1fda8516464a003c8c7cc547bc20c2 upstream. + +I have a workload where perf top scribbles over the stack and we SEGV. +What makes it interesting is that an snprintf is causing this. + +The workload is a c++ gem that has method names over 3000 characters +long, but snprintf is designed to avoid overrunning buffers. So what +went wrong? + +The problem is we assume snprintf returns the number of characters +written: + + ret += repsep_snprintf(bf + ret, size - ret, "[%c] ", self->level); +... + ret += repsep_snprintf(bf + ret, size - ret, "%s", self->ms.sym->name); + +Unfortunately this is not how snprintf works. snprintf returns the +number of characters that would have been written if there was enough +space. In the above case, if the first snprintf returns a value larger +than size, we pass a negative size into the second snprintf and happily +scribble over the stack. If you have 3000 character c++ methods thats a +lot of stack to trample. + +This patch fixes repsep_snprintf by clamping the value at size - 1 which +is the maximum snprintf can write before adding the NULL terminator. + +I get the sinking feeling that there are a lot of other uses of snprintf +that have this same bug, we should audit them all. + +Cc: David Ahern +Cc: Eric B Munson +Cc: Frederic Weisbecker +Cc: Ingo Molnar +Cc: Paul Mackerras +Cc: Peter Zijlstra +Cc: Yanmin Zhang +Link: http://lkml.kernel.org/r/20120307114249.44275ca3@kryten +Signed-off-by: Anton Blanchard +Signed-off-by: Arnaldo Carvalho de Melo +Signed-off-by: Greg Kroah-Hartman + +--- + tools/perf/util/sort.c | 3 +++ + 1 file changed, 3 insertions(+) + +--- a/tools/perf/util/sort.c ++++ b/tools/perf/util/sort.c +@@ -33,6 +33,9 @@ static int repsep_snprintf(char *bf, siz + } + } + va_end(ap); ++ ++ if (n >= (int)size) ++ return size - 1; + return n; + } + diff --git a/debian/patches/perf-tools-use-scnprintf-where-applicable.patch b/debian/patches/perf-tools-use-scnprintf-where-applicable.patch new file mode 100644 index 000000000..b610e4f77 --- /dev/null +++ b/debian/patches/perf-tools-use-scnprintf-where-applicable.patch @@ -0,0 +1,267 @@ +From e7f01d1e3d8d501deb8abeaa269d5d48a703b8b0 Mon Sep 17 00:00:00 2001 +From: Arnaldo Carvalho de Melo +Date: Wed, 14 Mar 2012 12:29:29 -0300 +Subject: perf tools: Use scnprintf where applicable + +From: Arnaldo Carvalho de Melo + +commit e7f01d1e3d8d501deb8abeaa269d5d48a703b8b0 upstream. + +Several places were expecting that the value returned was the number of +characters printed, not what would be printed if there was space. + +Fix it by using the scnprintf and vscnprintf variants we inherited from +the kernel sources. + +Some corner cases where the number of printed characters were not +accounted were fixed too. + +Reported-by: Anton Blanchard +Cc: Anton Blanchard +Cc: Eric B Munson +Cc: David Ahern +Cc: Frederic Weisbecker +Cc: Mike Galbraith +Cc: Paul Mackerras +Cc: Peter Zijlstra +Cc: Stephane Eranian +Cc: Yanmin Zhang +Link: http://lkml.kernel.org/n/tip-kwxo2eh29cxmd8ilixi2005x@git.kernel.org +Signed-off-by: Arnaldo Carvalho de Melo +Signed-off-by: Greg Kroah-Hartman + +--- + tools/perf/arch/powerpc/util/header.c | 2 +- + tools/perf/arch/x86/util/header.c | 2 +- + tools/perf/util/color.c | 9 +++++---- + tools/perf/util/header.c | 4 ++-- + tools/perf/util/hist.c | 30 +++++++++++++++--------------- + tools/perf/util/strbuf.c | 7 ++++--- + tools/perf/util/ui/browsers/hists.c | 12 ++++++------ + tools/perf/util/ui/helpline.c | 2 +- + 8 files changed, 35 insertions(+), 33 deletions(-) + +--- a/tools/perf/arch/powerpc/util/header.c ++++ b/tools/perf/arch/powerpc/util/header.c +@@ -25,7 +25,7 @@ get_cpuid(char *buffer, size_t sz) + + pvr = mfspr(SPRN_PVR); + +- nb = snprintf(buffer, sz, "%lu,%lu$", PVR_VER(pvr), PVR_REV(pvr)); ++ nb = scnprintf(buffer, sz, "%lu,%lu$", PVR_VER(pvr), PVR_REV(pvr)); + + /* look for end marker to ensure the entire data fit */ + if (strchr(buffer, '$')) { +--- a/tools/perf/arch/x86/util/header.c ++++ b/tools/perf/arch/x86/util/header.c +@@ -48,7 +48,7 @@ get_cpuid(char *buffer, size_t sz) + if (family >= 0x6) + model += ((a >> 16) & 0xf) << 4; + } +- nb = snprintf(buffer, sz, "%s,%u,%u,%u$", vendor, family, model, step); ++ nb = scnprintf(buffer, sz, "%s,%u,%u,%u$", vendor, family, model, step); + + /* look for end marker to ensure the entire data fit */ + if (strchr(buffer, '$')) { +--- a/tools/perf/util/color.c ++++ b/tools/perf/util/color.c +@@ -1,3 +1,4 @@ ++#include + #include "cache.h" + #include "color.h" + +@@ -182,12 +183,12 @@ static int __color_vsnprintf(char *bf, s + } + + if (perf_use_color_default && *color) +- r += snprintf(bf, size, "%s", color); +- r += vsnprintf(bf + r, size - r, fmt, args); ++ r += scnprintf(bf, size, "%s", color); ++ r += vscnprintf(bf + r, size - r, fmt, args); + if (perf_use_color_default && *color) +- r += snprintf(bf + r, size - r, "%s", PERF_COLOR_RESET); ++ r += scnprintf(bf + r, size - r, "%s", PERF_COLOR_RESET); + if (trail) +- r += snprintf(bf + r, size - r, "%s", trail); ++ r += scnprintf(bf + r, size - r, "%s", trail); + return r; + } + +--- a/tools/perf/util/header.c ++++ b/tools/perf/util/header.c +@@ -1227,7 +1227,7 @@ int build_id_cache__add_s(const char *sb + if (realname == NULL || filename == NULL || linkname == NULL) + goto out_free; + +- len = snprintf(filename, size, "%s%s%s", ++ len = scnprintf(filename, size, "%s%s%s", + debugdir, is_kallsyms ? "/" : "", realname); + if (mkdir_p(filename, 0755)) + goto out_free; +@@ -1242,7 +1242,7 @@ int build_id_cache__add_s(const char *sb + goto out_free; + } + +- len = snprintf(linkname, size, "%s/.build-id/%.2s", ++ len = scnprintf(linkname, size, "%s/.build-id/%.2s", + debugdir, sbuild_id); + + if (access(linkname, X_OK) && mkdir_p(linkname, 0755)) +--- a/tools/perf/util/hist.c ++++ b/tools/perf/util/hist.c +@@ -767,7 +767,7 @@ static int hist_entry__pcnt_snprintf(str + sep ? "%.2f" : " %6.2f%%", + (period * 100.0) / total); + else +- ret = snprintf(s, size, sep ? "%.2f" : " %6.2f%%", ++ ret = scnprintf(s, size, sep ? "%.2f" : " %6.2f%%", + (period * 100.0) / total); + if (symbol_conf.show_cpu_utilization) { + ret += percent_color_snprintf(s + ret, size - ret, +@@ -790,20 +790,20 @@ static int hist_entry__pcnt_snprintf(str + } + } + } else +- ret = snprintf(s, size, sep ? "%" PRIu64 : "%12" PRIu64 " ", period); ++ ret = scnprintf(s, size, sep ? "%" PRIu64 : "%12" PRIu64 " ", period); + + if (symbol_conf.show_nr_samples) { + if (sep) +- ret += snprintf(s + ret, size - ret, "%c%" PRIu64, *sep, nr_events); ++ ret += scnprintf(s + ret, size - ret, "%c%" PRIu64, *sep, nr_events); + else +- ret += snprintf(s + ret, size - ret, "%11" PRIu64, nr_events); ++ ret += scnprintf(s + ret, size - ret, "%11" PRIu64, nr_events); + } + + if (symbol_conf.show_total_period) { + if (sep) +- ret += snprintf(s + ret, size - ret, "%c%" PRIu64, *sep, period); ++ ret += scnprintf(s + ret, size - ret, "%c%" PRIu64, *sep, period); + else +- ret += snprintf(s + ret, size - ret, " %12" PRIu64, period); ++ ret += scnprintf(s + ret, size - ret, " %12" PRIu64, period); + } + + if (pair_hists) { +@@ -818,25 +818,25 @@ static int hist_entry__pcnt_snprintf(str + diff = new_percent - old_percent; + + if (fabs(diff) >= 0.01) +- snprintf(bf, sizeof(bf), "%+4.2F%%", diff); ++ ret += scnprintf(bf, sizeof(bf), "%+4.2F%%", diff); + else +- snprintf(bf, sizeof(bf), " "); ++ ret += scnprintf(bf, sizeof(bf), " "); + + if (sep) +- ret += snprintf(s + ret, size - ret, "%c%s", *sep, bf); ++ ret += scnprintf(s + ret, size - ret, "%c%s", *sep, bf); + else +- ret += snprintf(s + ret, size - ret, "%11.11s", bf); ++ ret += scnprintf(s + ret, size - ret, "%11.11s", bf); + + if (show_displacement) { + if (displacement) +- snprintf(bf, sizeof(bf), "%+4ld", displacement); ++ ret += scnprintf(bf, sizeof(bf), "%+4ld", displacement); + else +- snprintf(bf, sizeof(bf), " "); ++ ret += scnprintf(bf, sizeof(bf), " "); + + if (sep) +- ret += snprintf(s + ret, size - ret, "%c%s", *sep, bf); ++ ret += scnprintf(s + ret, size - ret, "%c%s", *sep, bf); + else +- ret += snprintf(s + ret, size - ret, "%6.6s", bf); ++ ret += scnprintf(s + ret, size - ret, "%6.6s", bf); + } + } + +@@ -854,7 +854,7 @@ int hist_entry__snprintf(struct hist_ent + if (se->elide) + continue; + +- ret += snprintf(s + ret, size - ret, "%s", sep ?: " "); ++ ret += scnprintf(s + ret, size - ret, "%s", sep ?: " "); + ret += se->se_snprintf(he, s + ret, size - ret, + hists__col_len(hists, se->se_width_idx)); + } +--- a/tools/perf/util/strbuf.c ++++ b/tools/perf/util/strbuf.c +@@ -1,4 +1,5 @@ + #include "cache.h" ++#include + + int prefixcmp(const char *str, const char *prefix) + { +@@ -89,14 +90,14 @@ void strbuf_addf(struct strbuf *sb, cons + if (!strbuf_avail(sb)) + strbuf_grow(sb, 64); + va_start(ap, fmt); +- len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap); ++ len = vscnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap); + va_end(ap); + if (len < 0) +- die("your vsnprintf is broken"); ++ die("your vscnprintf is broken"); + if (len > strbuf_avail(sb)) { + strbuf_grow(sb, len); + va_start(ap, fmt); +- len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap); ++ len = vscnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap); + va_end(ap); + if (len > strbuf_avail(sb)) { + die("this should not happen, your snprintf is broken"); +--- a/tools/perf/util/ui/browsers/hists.c ++++ b/tools/perf/util/ui/browsers/hists.c +@@ -839,15 +839,15 @@ static int hists__browser_title(struct h + unsigned long nr_events = self->stats.nr_events[PERF_RECORD_SAMPLE]; + + nr_events = convert_unit(nr_events, &unit); +- printed = snprintf(bf, size, "Events: %lu%c %s", nr_events, unit, ev_name); ++ printed = scnprintf(bf, size, "Events: %lu%c %s", nr_events, unit, ev_name); + + if (thread) +- printed += snprintf(bf + printed, size - printed, ++ printed += scnprintf(bf + printed, size - printed, + ", Thread: %s(%d)", + (thread->comm_set ? thread->comm : ""), + thread->pid); + if (dso) +- printed += snprintf(bf + printed, size - printed, ++ printed += scnprintf(bf + printed, size - printed, + ", DSO: %s", dso->short_name); + return printed; + } +@@ -1097,7 +1097,7 @@ static void perf_evsel_menu__write(struc + HE_COLORSET_NORMAL); + + nr_events = convert_unit(nr_events, &unit); +- printed = snprintf(bf, sizeof(bf), "%lu%c%s%s", nr_events, ++ printed = scnprintf(bf, sizeof(bf), "%lu%c%s%s", nr_events, + unit, unit == ' ' ? "" : " ", ev_name); + slsmg_printf("%s", bf); + +@@ -1107,8 +1107,8 @@ static void perf_evsel_menu__write(struc + if (!current_entry) + ui_browser__set_color(browser, HE_COLORSET_TOP); + nr_events = convert_unit(nr_events, &unit); +- snprintf(bf, sizeof(bf), ": %ld%c%schunks LOST!", nr_events, +- unit, unit == ' ' ? "" : " "); ++ printed += scnprintf(bf, sizeof(bf), ": %ld%c%schunks LOST!", ++ nr_events, unit, unit == ' ' ? "" : " "); + warn = bf; + } + +--- a/tools/perf/util/ui/helpline.c ++++ b/tools/perf/util/ui/helpline.c +@@ -65,7 +65,7 @@ int ui_helpline__show_help(const char *f + static int backlog; + + pthread_mutex_lock(&ui__lock); +- ret = vsnprintf(ui_helpline__last_msg + backlog, ++ ret = vscnprintf(ui_helpline__last_msg + backlog, + sizeof(ui_helpline__last_msg) - backlog, format, ap); + backlog += ret; + diff --git a/debian/patches/series b/debian/patches/series index fd1f7d4cc..17b8cc3b3 100644 --- a/debian/patches/series +++ b/debian/patches/series @@ -1,3 +1,5 @@ +perf-tools-incorrect-use-of-snprintf-results-in-segv.patch +perf-tools-use-scnprintf-where-applicable.patch modpost-symbol-prefix.patch tools-perf-version.patch tools-perf-install.patch