| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20161007022200.GB31113@sejong>
Date: Fri, 7 Oct 2016 11:22:00 +0900
From: Namhyung Kim <namhyung@...nel.org>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
CC: Krister Johansen <kjlx@...pleofstupid.com>,
Masami Hiramatsu <mhiramat@...nel.org>,
Frédéric Weisbecker <fweisbec@...il.com>,
<linux-kernel@...r.kernel.org>
Subject: Re: callchain map refcounting fixes was Re: [PATCH perf/core] perf
script: fix a use after free crash.
Hi Arnaldo and Krister,
On Wed, Oct 05, 2016 at 08:45:24AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sat, Oct 01, 2016 at 08:13:36PM -0700, Krister Johansen escreveu:
> > If dso__load_kcore frees all of the existing maps, but one has already
> > been attached to a callchain cursor node, then we can get a SIGSEGV in
> > any function that happens to try to use this cursor with the invalid
> > map. Use the existing map refcount mechanism to forestall cleanup of a
> > map until the cursor iterates past the node.
>
> Seems ok, thanks for working on this! Can you provide a test case that
> causes the SEGV so that I can, in addition to reviewing your changes and
> auditing the code to check if all cases ara plugged, to reproduce the
> problem?
>
> Frédéric, Namhyung, Ack?
>
> Masami, is this a case that your refcount validator can catch?
>
> - Arnaldo
>
> > Signed-off-by: Krister Johansen <kjlx@...pleofstupid.com>
> > ---
> > tools/perf/util/callchain.c | 12 ++++++++++--
> > tools/perf/util/callchain.h | 20 ++++++++++++++++++++
> > tools/perf/util/hist.c | 4 ++++
> > 3 files changed, 34 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> > index 07fd30b..15c89b2 100644
> > --- a/tools/perf/util/callchain.c
> > +++ b/tools/perf/util/callchain.c
> > @@ -439,7 +439,7 @@ fill_node(struct callchain_node *node, struct callchain_cursor *cursor)
> > }
> > call->ip = cursor_node->ip;
> > call->ms.sym = cursor_node->sym;
> > - call->ms.map = cursor_node->map;
> > + call->ms.map = map__get(cursor_node->map);
> > list_add_tail(&call->list, &node->val);
> >
> > callchain_cursor_advance(cursor);
> > @@ -464,6 +464,7 @@ add_child(struct callchain_node *parent,
> >
> > list_for_each_entry_safe(call, tmp, &new->val, list) {
> > list_del(&call->list);
> > + map__zput(call->ms.map);
> > free(call);
> > }
> > free(new);
> > @@ -732,6 +733,7 @@ merge_chain_branch(struct callchain_cursor *cursor,
> > callchain_cursor_append(cursor, list->ip,
> > list->ms.map, list->ms.sym);
> > list_del(&list->list);
> > + map__zput(list->ms.map);
> > free(list);
> > }
> >
> > @@ -780,7 +782,8 @@ int callchain_cursor_append(struct callchain_cursor *cursor,
> > }
> >
> > node->ip = ip;
> > - node->map = map;
> > + map__zput(node->map);
> > + node->map = map__get(map);
> > node->sym = sym;
> >
> > cursor->nr++;
> > @@ -830,6 +833,8 @@ int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node *
> > goto out;
> > }
> >
> > + map__get(al->map);
> > +
> > if (al->map->groups == &al->machine->kmaps) {
> > if (machine__is_host(al->machine)) {
> > al->cpumode = PERF_RECORD_MISC_KERNEL;
> > @@ -947,11 +952,13 @@ static void free_callchain_node(struct callchain_node *node)
> >
> > list_for_each_entry_safe(list, tmp, &node->parent_val, list) {
> > list_del(&list->list);
> > + map__zput(list->ms.map);
> > free(list);
> > }
> >
> > list_for_each_entry_safe(list, tmp, &node->val, list) {
> > list_del(&list->list);
> > + map__zput(list->ms.map);
> > free(list);
> > }
> >
> > @@ -1035,6 +1042,7 @@ int callchain_node__make_parent_list(struct callchain_node *node)
> > out:
> > list_for_each_entry_safe(chain, new, &head, list) {
> > list_del(&chain->list);
> > + map__zput(chain->ms.map);
I think you need to grab the refcnt in the "while (parent)" loop above.
> > free(chain);
> > }
> > return -ENOMEM;
> > diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
> > index 13e7554..0d944ef 100644
> > --- a/tools/perf/util/callchain.h
> > +++ b/tools/perf/util/callchain.h
> > @@ -5,6 +5,7 @@
> > #include <linux/list.h>
> > #include <linux/rbtree.h>
> > #include "event.h"
> > +#include "map.h"
> > #include "symbol.h"
> >
> > #define HELP_PAD "\t\t\t\t"
> > @@ -178,8 +179,13 @@ int callchain_merge(struct callchain_cursor *cursor,
> > */
> > static inline void callchain_cursor_reset(struct callchain_cursor *cursor)
> > {
> > + struct callchain_cursor_node *node;
> > +
> > cursor->nr = 0;
> > cursor->last = &cursor->first;
> > +
> > + for (node = cursor->first; node != NULL; node = node->next)
> > + map__zput(node->map);
> > }
> >
> > int callchain_cursor_append(struct callchain_cursor *cursor, u64 ip,
> > @@ -238,12 +244,26 @@ int perf_callchain_config(const char *var, const char *value);
> > static inline void callchain_cursor_snapshot(struct callchain_cursor *dest,
> > struct callchain_cursor *src)
> > {
> > + struct callchain_cursor_node *node;
> > +
> > *dest = *src;
> >
> > dest->first = src->curr;
> > dest->nr -= src->pos;
> > +
> > + for (node = dest->first; node != NULL; node = node->next)
> > + map__get(node->map);
> > }
> >
> > +static inline void callchain_cursor_snapshot_rele(struct callchain_cursor *curs)
> > +{
> > + struct callchain_cursor_node *node;
> > +
> > + for (node = curs->first; node != NULL; node = node->next)
> > + map__put(node->map);
> > +}
> > +
> > +
> > #ifdef HAVE_SKIP_CALLCHAIN_IDX
> > int arch_skip_callchain_idx(struct thread *thread, struct ip_callchain *chain);
> > #else
> > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > index b02992e..f8335e8 100644
> > --- a/tools/perf/util/hist.c
> > +++ b/tools/perf/util/hist.c
> > @@ -1,6 +1,7 @@
> > #include "util.h"
> > #include "build-id.h"
> > #include "hist.h"
> > +#include "map.h"
> > #include "session.h"
> > #include "sort.h"
> > #include "evlist.h"
> > @@ -970,6 +971,8 @@ iter_add_next_cumulative_entry(struct hist_entry_iter *iter,
> >
> > if (symbol_conf.use_callchain)
> > callchain_append(he->callchain, &cursor, sample->period);
> > + /* Cleanup temporary cursor. */
> > + callchain_cursor_snapshot_rele(&cursor);
This callchain shotshot is used in a short period of time, and it's
guaranteed that the maps in callchains will not freed due to refcnt in
the orignal callchain cursor. So I think we can skip to get/put
refcnt on the snapshot cursor. Also "rele" seems not a good name..
> > return 0;
> > }
> >
> > @@ -979,6 +982,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
> > {
> > zfree(&iter->priv);
> > iter->he = NULL;
> > + map__zput(al->map);
What is this needed? Why other places like iter_finish_normal_entry
isn't?
> >
> > return 0;
> > }
> > --
> > 2.7.4
Powered by blists - more mailing lists