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  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:   Wed, 5 Aug 2020 10:15:26 +0800
From:   "Jin, Yao" <yao.jin@...ux.intel.com>
To:     peterz@...radead.org
Cc:     mingo@...hat.com, oleg@...hat.com, acme@...nel.org,
        jolsa@...nel.org, Linux-kernel@...r.kernel.org, ak@...ux.intel.com,
        kan.liang@...el.com, yao.jin@...el.com,
        alexander.shishkin@...ux.intel.com, mark.rutland@....com
Subject: Re: [PATCH v1 2/2] perf/core: Fake regs for leaked kernel samples

Hi Peter,

On 8/4/2020 7:49 PM, peterz@...radead.org wrote:
> On Fri, Jul 31, 2020 at 10:56:17AM +0800, Jin Yao wrote:
>> @@ -6973,7 +6973,8 @@ static struct perf_callchain_entry __empty_callchain = { .nr = 0, };
>>   struct perf_callchain_entry *
>>   perf_callchain(struct perf_event *event, struct pt_regs *regs)
>>   {
>> -	bool kernel = !event->attr.exclude_callchain_kernel;
>> +	bool kernel = !event->attr.exclude_callchain_kernel &&
>> +		      !event->attr.exclude_kernel;
> 
> This seems weird; how can we get there. Also it seems to me that if you
> have !exclude_callchain_kernel you already have permission for kernel
> bits, so who cares.
> 

In perf tool, exclude_callchain_kernel is set to 1 when perf-record only collects the user 
callchains and exclude_kernel is set to 1 when events are configured to run in user space.

So if an event is configured to run in user space, that should make sense we don't need it's kernel 
callchains.

But it seems to me there is no code logic in perf tool which can make sure !exclude_callchain_kernel 
-> !exclude_kernel.

Jiri, Arnaldo, is my understanding correct?

>>   	bool user   = !event->attr.exclude_callchain_user;
>>   	/* Disallow cross-task user callchains. */
>>   	bool crosstask = event->ctx->task && event->ctx->task != current;
>> @@ -6988,12 +6989,39 @@ perf_callchain(struct perf_event *event, struct pt_regs *regs)
>>   	return callchain ?: &__empty_callchain;
>>   }
>>   
>> +static struct pt_regs *leak_check(struct perf_event_header *header,
>> +				  struct perf_event *event,
>> +				  struct pt_regs *regs)
>> +{
>> +	struct pt_regs *regs_fake = NULL;
>> +
>> +	if (event->attr.exclude_kernel && !user_mode(regs)) {
>> +		if (!(current->flags & PF_KTHREAD)) {
>> +			regs_fake = task_pt_regs(current);
>> +			if (!user_mode(regs_fake)) {
>> +				regs_fake = NULL;
>> +				instruction_pointer_set(regs, -1L);
>> +			}
>> +		} else
>> +			instruction_pointer_set(regs, -1L);
> 
> That violates coding style, also:
> 

Thanks, I should use:

} else {
	instruction_pointer_set(regs, -1L);
}

> 		if (!(current->flags & PF_KTHREAD)) {
> 			regs_fake = task_pt_regs(current);
> 			if (!user_mode(regs_fake)) /* is this not a BUG?  */

We don't need !user_mode(regs_fake) check here, it's unnecessary check.

> 				regs_fake = NULL;
> 		}
> 
> 		if (!regs_fake)
> 			instruction_pointer_set(regs, -1L);
> 
> Seems simpler to me.
> 

So the new code looks like:

if (event->attr.exclude_kernel && !user_mode(regs)) {
	if (!(current->flags & PF_KTHREAD)) {
		regs_fake = task_pt_regs(current);
		if (!regs_fake)
			instruction_pointer_set(regs, -1L);
	} else {
		instruction_pointer_set(regs, -1L);
	}

>> +		if ((header->misc & PERF_RECORD_MISC_CPUMODE_MASK) ==
>> +		     PERF_RECORD_MISC_KERNEL) {
>> +			header->misc &= ~PERF_RECORD_MISC_CPUMODE_MASK;
>> +			header->misc |= PERF_RECORD_MISC_USER;
>> +		}
> 
> Why the conditional? At this point it had better be unconditionally
> user, no?
> 
> 		headers->misc &= ~PERF_RECORD_MISC_CPUMODE_MASK;
> 		headers->misc |= PERF_RECORD_MISC_USER;
> 

#define PERF_RECORD_MISC_CPUMODE_MASK		(7 << 0)
#define PERF_RECORD_MISC_CPUMODE_UNKNOWN	(0 << 0)
#define PERF_RECORD_MISC_KERNEL			(1 << 0)
#define PERF_RECORD_MISC_USER			(2 << 0)
#define PERF_RECORD_MISC_HYPERVISOR		(3 << 0)
#define PERF_RECORD_MISC_GUEST_KERNEL		(4 << 0)
#define PERF_RECORD_MISC_GUEST_USER		(5 << 0)

If we unconditionally set user, it will reset for hypervisor, guest kernel and guest_user.

Thanks
Jin Yao

>> +	}
>> +
>> +	return regs_fake;
>> +}

Powered by blists - more mailing lists