From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1754392AbdESJ3S (ORCPT <rfc822;w@1wt.eu>);
        Fri, 19 May 2017 05:29:18 -0400
Received: from merlin.infradead.org ([205.233.59.134]:48430 "EHLO
        merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751109AbdESJ3O (ORCPT
        <rfc822;Linux-kernel@vger.kernel.org>);
        Fri, 19 May 2017 05:29:14 -0400
Date: Fri, 19 May 2017 11:29:05 +0200
From: Peter Zijlstra <peterz@infradead.org>
To: Jin Yao <yao.jin@linux.intel.com>
Cc: acme@kernel.org, jolsa@kernel.org, mingo@redhat.com,
        alexander.shishkin@linux.intel.com, Linux-kernel@vger.kernel.org,
        ak@linux.intel.com, kan.liang@intel.com, yao.jin@intel.com,
        Will Deacon <will.deacon@arm.com>, Mark Rutland <mark.rutland@arm.com>
Subject: Re: [PATCH] perf/x86/intel: Drop kernel samples even though :u is
 specified
Message-ID: <20170519092905.yivnfjyhblm5wiap@hirez.programming.kicks-ass.net>
References: <1495189152-5766-1-git-send-email-yao.jin@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1495189152-5766-1-git-send-email-yao.jin@linux.intel.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, May 19, 2017 at 06:19:12PM +0800, Jin Yao wrote:
> When doing sampling without PEBS
> 
> perf record -e cycles:u ...
> 
> On workloads that do a lot of kernel entry/exits we see kernel
> samples, even though :u is specified. This is due to skid existing.
> 
> This is a security issue because it can leak kernel addresses even
> though kernel sampling support is disabled.
> 
> The patch drops the kernel samples if exclude_kernel is specified.
> 
> For example, test on Haswell desktop.
> 
> perf record -e cycles:u <mgen>
> perf report --stdio
> 
> Before patch applied:
> 
>     99.77%  mgen     mgen              [.] buf_read
>      0.20%  mgen     mgen              [.] rand_buf_init
>      0.01%  mgen     [kernel.vmlinux]  [k] apic_timer_interrupt


> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 580b60f..e6745e1 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -1463,6 +1463,12 @@ int x86_pmu_handle_irq(struct pt_regs *regs)
>  		if (!x86_perf_event_set_period(event))
>  			continue;
>  
> +		/*
> +		 * For security, drop the skid kernel samples.
> +		 */
> +		if (skid_kernel_samples(event, regs))
> +			continue;
> +
>  		if (perf_event_overflow(event, &data, regs))
>  			x86_pmu_stop(event, 0);
>  	}
> @@ -1679,6 +1685,24 @@ ssize_t events_ht_sysfs_show(struct device *dev, struct device_attribute *attr,
>  			pmu_attr->event_str_noht);
>  }
>  
> +bool skid_kernel_samples(struct perf_event *event, struct pt_regs *regs)
> +{
> +	u64 ip;
> +
> +	/*
> +	 * Without PEBS, we may get kernel samples even though
> +	 * exclude_kernel is specified due to skid in sampling.
> +	 */
> +	if ((event->attr.exclude_kernel) &&
> +	    (event->attr.sample_type & PERF_SAMPLE_IP)) {
> +		ip = perf_instruction_pointer(regs);
> +		if (kernel_ip(ip))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  EVENT_ATTR(cpu-cycles,			CPU_CYCLES		);
>  EVENT_ATTR(instructions,		INSTRUCTIONS		);
>  EVENT_ATTR(cache-references,		CACHE_REFERENCES	);


I would much rather see this in generic code, somewhere around
__perf_event_overflow() I suppose. That would retain proper accounting
for the interrupt rate etc..

Also it would work for all architectures. Because I'm thinking more than
just x86 will suffer from skid.

If you're really worried, I suppose you can put it behind a PERF_PMU_CAP
flag or something.