linux-kernel - Re: [PATCH v2] perf/core: Drop kernel samples even though :u is specified

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] [day] [month] [year] [list]

Date:   Tue, 23 May 2017 20:32:37 +0800
From:   "Jin, Yao" <yao.jin@...ux.intel.com>
To:     Mark Rutland <mark.rutland@....com>
Cc:     acme@...nel.org, jolsa@...nel.org, peterz@...radead.org,
        mingo@...hat.com, alexander.shishkin@...ux.intel.com,
        Linux-kernel@...r.kernel.org, ak@...ux.intel.com,
        kan.liang@...el.com, yao.jin@...el.com, will.deacon@....com
Subject: Re: [PATCH v2] perf/core: Drop kernel samples even though :u is
 specified

SNIP
>> +static bool skid_kernel_samples(struct perf_event *event, struct pt_regs *regs)
> The name is a bit opaque, especially where it is used in
> __perf_event_overflow().
>
> How about we invert the polarity and call this sample_is_allowed() ?

That's OK, thanks.

>> +{
>> +	/*
>> +	 * We may get kernel samples even though exclude_kernel
>> +	 * is specified due to potential skid in sampling.
>> +	 * The skid kernel samples could be dropped or just do
>> +	 * nothing by testing the flag PERF_PMU_CAP_NO_SKID.
>> +	 */
>> +	if (event->pmu->capabilities & PERF_PMU_CAP_NO_SKID)
>> +		return false;
> Do we need this new cap?
>
> I'd expect user_mode(regs) to be about as cheap as testing the cap, and
> the common case is going to be that we we have test both.
>
> For those PMUs without skid, when not sampling the kernel,
> user_mode(regs) should always be true.
>
> IMO, it would make more sense to just check user_mode(regs), which also
> avoids any surprises with unexpected skid...
I guess the reason which Peter recommends to use a new cap is to have a 
way to keep original behavior.

If we don't need to keep original behavior, I think the new cap is not 
necessary. Could Peter provide comment? Thanks!

>> +
>> +	if (event->attr.exclude_kernel &&
>> +	    !user_mode(regs) &&
>> +	    (event->attr.sample_type & PERF_SAMPLE_IP)) {
>> +		return true;
>> +	}
>> +
>> +	return false;
>> +}
> How about:
>
> static bool sample_is_allowed(struct perf_event *event, struct pt_regs *regs)
> {
> 	/*
> 	 * Due to interrupt latency (AKA "skid"), we may enter the
> 	 * kernel before taking an overflow, even if the PMU is only
> 	 * counting user events.
> 	 *
> 	 * To avoid leaking information to userspace, we must always
> 	 * reject kernel samples when exclude_kernel is set.
> 	 */
> 	if (!user_mode(regs) && event->attr.exclude_kernel &&
> 	    (event->attr.sample_type & PERF_SAMPLE_IP))
> 		return false;
>
> 	return true;
> }
>
> ... do we need to reject any other sample types, or do we definitely
> avoid leaks by other means?
I just think only when the PERF_SAMPLE_IP is applied, we can get correct 
ip. So I check the PERF_SAMPLE_IP here.

>> +
>>   /*
>>    * Generic event overflow handling, sampling.
>>    */
>> @@ -7337,6 +7357,12 @@ static int __perf_event_overflow(struct perf_event *event,
>>   	ret = __perf_event_account_interrupt(event, throttle);
>>   
>>   	/*
>> +	 * For security, drop the skid kernel samples if necessary.
>> +	 */
>> +	if (skid_kernel_samples(event, regs))
>> +		return ret;
>> +
> .. with the above changes, this can be:
>
> 	if (!sample_is_allowed(event, regs))
> 		return ret;
>
> Thanks,
> Mark.
OK, thanks! I will change the patch according to your comments.

Thanks
Jin Yao