Re: [PATCH v2] perf/x86: Check data address for IBS software filter

Namhyung Kim
2025-03-18T18:09:23Z

Hi Ravi,

On Tue, Mar 18, 2025 at 04:02:20PM +0530, Ravi Bangoria wrote:
> Hi Namhyung,
>
> On 17-Mar-25 10:07 PM, Namhyung Kim wrote:
> > IBS software filter was to filter kernel samples for regular users in
> > PMI handler. It checks the instruction address in the IBS register to
> > determine if it was in the kernel more or not.
> >
> > But it turns out that it's possible to report a kernel data address even
> > if the instruction address belongs to the user space. Matteo Rizzo
> > found that when an instruction raises an exception, IBS can report some
> > kernel data address like IDT while holding the faulting instruction's
> > RIP. To prevent an information leak, it should double check if the data
> > address in PERF_SAMPLE_DATA is in the kernel space as well.
>
> PERF_SAMPLE_RAW can also leak kernel data address. How about:

Thanks for your review.

I think RAW is different as it requires perf_event_paranoid == -1.
This is normally not allowed to regular users and having this means
you can profile kernel with detailed tracepoints info already.

Thanks,
Namhyung

>
> --- a/arch/x86/events/amd/ibs.c
> +++ b/arch/x86/events/amd/ibs.c
> @@ -1159,6 +1159,25 @@ static int perf_ibs_get_offset_max(struct perf_ibs *perf_ibs,
> return 1;
> }
>
> +static bool perf_ibs_swfilt_discard(struct perf_ibs *perf_ibs, struct perf_event *event,
> + struct pt_regs *regs, struct perf_ibs_data *ibs_data)
> +{
> + union ibs_op_data3 op_data3;
> +
> + if (perf_exclude_event(event, regs))
> + return true;
> +
> + if (perf_ibs != &perf_ibs_op || !event->attr.exclude_kernel)
> + return false;
> +
> + op_data3.val = ibs_data->regs[ibs_op_msr_idx(MSR_AMD64_IBSOPDATA3)];
> +
> + /* Prevent leaking kernel 'data' addresses to unprivileged users. */
> + return unlikely(event->attr.sample_type & (PERF_SAMPLE_ADDR | PERF_SAMPLE_RAW) &&
> + op_data3.dc_lin_addr_valid &&
> + kernel_ip(ibs_data->regs[ibs_op_msr_idx(MSR_AMD64_IBSDCLINAD)]));
> +}
> +
> static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs)
> {
> struct cpu_perf_ibs *pcpu = this_cpu_ptr(perf_ibs->pcpu);
> @@ -1268,7 +1287,7 @@ static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs)
> }
>
> if ((event->attr.config2 & IBS_SW_FILTER_MASK) &&
> - perf_exclude_event(event, &regs)) {
> + perf_ibs_swfilt_discard(perf_ibs, event, &regs, &ibs_data)) {
> throttle = perf_event_account_interrupt(event);
> goto out;
> }
> --
>
> Thanks,
> Ravi