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

[Namhyung Kim]

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