linux-kernel - Re: [PATCH 2/8] perf: Sanitize get_callchain

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:	Thu, 1 Aug 2013 16:30:07 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Jiri Olsa <jolsa@...hat.com>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Namhyung Kim <namhyung@...nel.org>,
	Ingo Molnar <mingo@...nel.org>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH 2/8] perf: Sanitize get_callchain_buffer()

On Thu, Aug 01, 2013 at 03:51:02PM +0200, Jiri Olsa wrote:
> On Thu, Aug 01, 2013 at 03:42:28PM +0200, Frederic Weisbecker wrote:
> > On Thu, Aug 01, 2013 at 03:29:34PM +0200, Jiri Olsa wrote:
> > > On Tue, Jul 23, 2013 at 02:31:00AM +0200, Frederic Weisbecker wrote:
> > > SNIP
> > > 
> > > >  		if (event->attach_state & PERF_ATTACH_TASK)
> > > >  			static_key_slow_inc(&perf_sched_events.key);
> > > >  		if (event->attr.mmap || event->attr.mmap_data)
> > > > @@ -6572,16 +6570,19 @@ done:
> > > >  				atomic_inc(&per_cpu(perf_branch_stack_events,
> > > >  						    event->cpu));
> > > >  		}
> > > > -		if (event->attr.sample_type & PERF_SAMPLE_CALLCHAIN) {
> > > > -			err = get_callchain_buffers();
> > > > -			if (err) {
> > > > -				free_event(event);
> > > > -				return ERR_PTR(err);
> > > > -			}
> > > > -		}
> > > >  	}
> > > >  
> > > >  	return event;
> > > > +
> > > > +err_pmu:
> > > > +	if (event->destroy)
> > > > +		event->destroy(event);
> > > > +err_ns:
> > > > +	if (event->ns)
> > > > +		put_pid_ns(event->ns);
> > > > +	kfree(event);
> > > > +
> > > > +	return ERR_PTR(err);
> > > 
> > > could we call __free_filter(event) here?
> > 
> > Hmm, the filters are installed from ioctl time so there shouldn't be any yet. But there should be
> > an exception with inherited events. I fail to find where the filter is inherited though. Do
> > we actually inherit those?
> 
> ouch.. last I checked was freeing filter before writing this... :)
> 
> what I meant was the __free_event(event)

free_event() doesn't work either because we want several level of rollback depending
of where the error triggered:

   +err_pmu:
           if (event->destroy)
                 event->destroy(event);
   +err_ns:
           if (event->ns)
                 put_pid_ns(event->ns);
           kfree(event);

           return ERR_PTR(err)

If we fail after pmu init we want to call destroy, free pid ns and the event.
If we fail before the pmu init, we want to only free pid ns and the event, ...

_free_event() does the whole in any case, which is not what we want.

But...

OTOH it might work due to the if (event->destroy) and if (event->ns) before freeing the
resource associated.

So may be I can replace the labels with a single call to __free_event() after all as it
checks what needs to be freed.  What do you think?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/