| 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: <20170224133253.GH3595@kernel.org>
Date: Fri, 24 Feb 2017 10:32:53 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: "Reshetova, Elena" <elena.reshetova@...el.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
"peterz@...radead.org" <peterz@...radead.org>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"mingo@...hat.com" <mingo@...hat.com>,
"alexander.shishkin@...ux.intel.com"
<alexander.shishkin@...ux.intel.com>,
"jolsa@...nel.org" <jolsa@...nel.org>,
"mark.rutland@....com" <mark.rutland@....com>,
"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
"matija.glavinic-pecotic.ext@...ia.com"
<matija.glavinic-pecotic.ext@...ia.com>
Subject: Re: [PATCH 0/9] tools subsystem refcounter conversions
Em Fri, Feb 24, 2017 at 07:27:32AM +0000, Reshetova, Elena escreveu:
> > Em Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
> > > > Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo
> > > > escreveu:
> > > > > Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo
> > > > escreveu:
> > > > > > Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
> > > > > > > Now when new refcount_t type and API are finally merged
> > > > > > > (see include/linux/refcount.h), the following
> > > > > > > patches convert various refcounters in the tools susystem from
> > atomic_t
> > > > > > > to refcount_t. By doing this we prevent intentional or accidental
> > > > > > > underflows or overflows that can led to use-after-free vulnerabilities.
> > > > > >
> > > > > > Thanks for working on this! I was almost going to jump on doing this
> > > > > > myself!
> > > > > >
> > > > > > I'll try and get this merged ASAP.
> > > > >
> > > > > So, please take a look at my tmp.perf/refcount branch at:
> > > > >
> > > > > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
> > >
> > > I took a look on it and it looks good. Just one thing I want to double check
> > with regards to this commit:
> > >
> > https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/58d
> > 561002587bf2572f9e6f4d222659e4068fadf%5E%21/#F0
> > >
> > > And more specifically to this chunk:
> > >
> > > @@ -937,7 +937,7 @@
> > > munmap(map->base,
> > perf_mmap__mmap_len(map));
> > > map->base = NULL;
> > > map->fd = -1;
> > > - atomic_set(&map->refcnt, 0);
> > > + refcount_set(&map->refcnt, 0);
> > > }
> > > auxtrace_mmap__munmap(&map->auxtrace_mmap);
> > > }
> > >
> > > So, when the refcount set to zero in this place, what exactly happens to the
> > perf_map object after?
> > > I just want to double check that we don't have another hiding reusage case
> > here when refcounter later on is simply incremented vs. set to "2."
> >
> > So, this is an odd use of a reference count, the patch below should help
> > understand it?
>
> Yes, it helps, indeed, but I think we have an issue here. See below inline in patch.
>
> >
> > Those perf_mmap objects are created in a batch fashion, it being zero
> > just means it isn't yet mmaped at all, and we check for that before
> > using it.
> >
> > So, it remains a bug to do a dec for a zeroed refcount, and the
> > refcount_t infrastructure will catch it, which helps tools/.
> >
> > - Arnaldo
> >
> > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > index 564b924fb48a..5a70f08d2518 100644
> > --- a/tools/perf/util/evlist.c
> > +++ b/tools/perf/util/evlist.c
> > @@ -974,8 +974,19 @@ static struct perf_mmap
> > *perf_evlist__alloc_mmap(struct perf_evlist *evlist)
> > if (!map)
> > return NULL;
> >
> > - for (i = 0; i < evlist->nr_mmaps; i++)
> > + for (i = 0; i < evlist->nr_mmaps; i++) {
> > map[i].fd = -1;
> > + /*
> > + * When the perf_mmap() call is made we grab
> > one refcount, plus
> > + * one extra to let perf_evlist__mmap_consume()
> > get the last
> > + * events after all real references
> > (perf_mmap__get()) are
> > + * dropped.
> > + *
> > + * Each PERF_EVENT_IOC_SET_OUTPUT points to
> > this mmap and
> > + * thus does perf_mmap__get() on it.
> > + */
> > + refcount_set(&map[i].refcnt, 0);
> > + }
> > return map;
> > }
> >
> > @@ -988,6 +999,7 @@ struct mmap_params {
> > static int perf_mmap__mmap(struct perf_mmap *map,
> > struct mmap_params *mp, int fd)
> > {
> > + perf_mmap__get(map);
> > /*
> > * The last one will be done at perf_evlist__mmap_consume(),
> > so that we
> > * make sure we don't prevent tools from consuming every last
> > event in
> > @@ -1001,7 +1013,7 @@ static int perf_mmap__mmap(struct perf_mmap
> > *map,
> > * evlist layer can't just drop it when filtering events in
> > * perf_evlist__filter_pollfd().
> > */
> > - refcount_set(&map->refcnt, 2);
> > + perf_mmap__get(map); /* This is not a dup, see the comment
> > above! */
> This change now means that instead of doing refcount_set to 2 when refcount value is "0", you are doing two
> refcount_inc() via perf_mmap__get(), which is not going to do any increments when refcount value is zero.
> So, in that sense having just one refcount_set to 2 with a good explanation why it is needed is better :)
Duh, thanks for pointint it out, will leave the comments, remove the
pair of perf_mmap__get()
> When I asked about it initially, I just wanted to make sure there are no other refcount_inc()s happening on the object when its refcount value is zero. Which looks to be the case apart from the one above.
>
> Best Regards,
> Elena.
>
> > map->prev = 0;
> > map->mask = mp->mask;
> > map->base = mmap(NULL, perf_mmap__mmap_len(map), mp-
> > >prot,
> >
> > > > > There are multiple fixes in it to get it to build and test it, so far,
> > > > > with:
> > > > >
> > > > > perf top -F 15000 -d 0
> > > > >
> > > > > while doing kernel builds and tight usleep 1 loops to create lots of
> > > > > short lived threads with its map_groups, maps, dsos, etc.
> > > > >
> > > > > Now running some build tests in some 36 containers with assorted distros
> > > > > and cross compilers.
> > > >
> > > > Tomorrow I'll inject some refcount errors to test this all.
> > >
> > >
> > > Thank you!
> > >
> > > Best Regards,
> > > Elena.
Powered by blists - more mailing lists