linux-kernel - RE: perf: fuzzer KASAN slab-out-of-bounds in snb_uncore_imc_event

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 15 Nov 2016 17:04:23 +0000
From:   "Liang, Kan" <kan.liang@...el.com>
To:     Peter Zijlstra <peterz@...radead.org>,
        Vince Weaver <vincent.weaver@...ne.edu>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        "davej@...emonkey.org.uk" <davej@...emonkey.org.uk>,
        "dvyukov@...gle.com" <dvyukov@...gle.com>,
        Stephane Eranian <eranian@...il.com>
Subject: RE: perf: fuzzer KASAN slab-out-of-bounds in
 snb_uncore_imc_event_del

> 
> On Tue, Nov 15, 2016 at 12:57:31AM -0500, Vince Weaver wrote:
> > On Mon, 14 Nov 2016, Vince Weaver wrote:
> >
> > > Anyway as per the suggestion at Linux Plumbers I enabled KASAN and
> > > on my haswell machine it falls over in a few minutes of running the
> perf_fuzzer.
> > >
> > > [  205.740194]
> > >
> ===============================================================
> ===
> > > [  205.748005] BUG: KASAN: slab-out-of-bounds in
> > > snb_uncore_imc_event_del+0x6c/0xa0 at addr ffff8800caa43768 [
> > > 205.758324] Read of size 8 by task perf_fuzzer/6618 [  205.763589]
> > > CPU: 0 PID: 6618 Comm: perf_fuzzer Not tainted 4.9.0-rc5 #4 [
> > > 205.770721] Hardware name: LENOVO 10AM000AUS/SHARKBAY, BIOS
> > > FBKT72AUS 01/26/2014 [  205.778689]  ffff8800c3c479b8
> > > ffffffff816bb796 ffff88011ec00600 ffff8800caa43580 [  205.786759]
> > > ffff8800c3c479e0 ffffffff812fb961 ffff8800c3c47a78 ffff8800caa43580
> [  205.794850]  ffff8800caa43580 ffff8800c3c47a68 ffffffff812fbbd8
> ffff8800c3c47a28 [  205.802911] Call Trace:
> > > [  205.805559]  [<ffffffff816bb796>] dump_stack+0x63/0x8d [
> > > 205.811135]  [<ffffffff812fb961>] kasan_object_err+0x21/0x70 [
> > > 205.817267]  [<ffffffff812fbbd8>] kasan_report_error+0x1d8/0x4c0 [
> > > 205.823752]  [<ffffffff81133275>] ? __lock_is_held+0x75/0xc0 [
> > > 205.829868]  [<ffffffff81025b12>] ?
> > > snb_uncore_imc_read_counter+0x42/0x50
> > > [  205.837198]  [<ffffffff810222e2>] ?
> > > uncore_perf_event_update+0xe2/0x160
> > > [  205.844337]  [<ffffffff812fc319>] kasan_report+0x39/0x40 [
> > > 205.850085]  [<ffffffff81025e3c>] ?
> > > snb_uncore_imc_event_del+0x6c/0xa0
> >
> > The best I can tell this maps to:
> >
> > static void snb_uncore_imc_event_del(struct perf_event *event, int
> > flags) {
> >         struct intel_uncore_box *box = uncore_event_to_box(event);
> >         int i;
> >
> >         snb_uncore_imc_event_stop(event, PERF_EF_UPDATE);
> >
> >         for (i = 0; i < box->n_events; i++) {
> > >>>             if (event == box->event_list[i]) {
> >                         --box->n_events;
> >                         break;
> >                 }
> >         }
> > }
> >
> > Can this code be right?  Does it actually remove the event?
> > The similar code in
> >
> > static void uncore_pmu_event_del(struct perf_event *event, int flags)
> >
> > ....
> >
> > 	for (i = 0; i < box->n_events; i++) {
> >                 if (event == box->event_list[i]) {
> >                         uncore_put_event_constraint(box, event);
> >
> >                         for (++i; i < box->n_events; i++)
> >                                 box->event_list[i - 1] =
> > box->event_list[i];
> >
> >                         --box->n_events;
> >                         break;
> >                 }
> >         }
> >
> >
> > seems like it is more likely to be correct.
> 
> Kan, can you look at this?

For client IMC, there is no generic counters.
Current implementation defines its own fixed free running counters. 
event_list and n_events are unused.  
I think we can just remove them.

Vince, could you please try the patch as below?

------

diff --git a/arch/x86/events/intel/uncore_snb.c b/arch/x86/events/intel/uncore_snb.c
index 81195cc..a3dcc12 100644
--- a/arch/x86/events/intel/uncore_snb.c
+++ b/arch/x86/events/intel/uncore_snb.c
@@ -490,24 +490,12 @@ static int snb_uncore_imc_event_add(struct perf_event *event, int flags)
 
 	snb_uncore_imc_event_start(event, 0);
 
-	box->n_events++;
-
 	return 0;
 }
 
 static void snb_uncore_imc_event_del(struct perf_event *event, int flags)
 {
-	struct intel_uncore_box *box = uncore_event_to_box(event);
-	int i;
-
 	snb_uncore_imc_event_stop(event, PERF_EF_UPDATE);
-
-	for (i = 0; i < box->n_events; i++) {
-		if (event == box->event_list[i]) {
-			--box->n_events;
-			break;
-		}
-	}
 }
 
 int snb_pci2phy_map_init(int devid)