Re: [PATCH v1 2/2] perf/core: Fake regs for leaked kernel samples

peterz
2020-08-04T11:54:01Z

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.

> 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:

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

if (!regs_fake)
instruction_pointer_set(regs, -1L);

Seems simpler to me.

> + 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;

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