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.

Suggested-by: Matteo Rizzo <matteorizzo@google.com>
Cc: Ravi Bangoria <ravi.bangoria@amd.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
v2)
 * fix a build error  (Boris)
 * use kernel_ip() instead  (Peter)
 * combine sw filter checks  (Ravi)

 arch/x86/events/amd/ibs.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index 7b52b8e3a185157f..fbe10b469e8b03d5 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -1267,8 +1267,13 @@ static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs)
 		regs.flags |= PERF_EFLAGS_EXACT;
 	}
 
+	if (perf_ibs == &perf_ibs_op)
+		perf_ibs_parse_ld_st_data(event->attr.sample_type, &ibs_data, &data);
+
 	if ((event->attr.config2 & IBS_SW_FILTER_MASK) &&
-	    perf_exclude_event(event, &regs)) {
+	    (perf_exclude_event(event, &regs) ||
+	     ((data.sample_flags & PERF_SAMPLE_ADDR) &&
+	      event->attr.exclude_kernel && kernel_ip(data.addr)))) {
 		throttle = perf_event_account_interrupt(event);
 		goto out;
 	}
@@ -1283,9 +1288,6 @@ static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs)
 		perf_sample_save_raw_data(&data, event, &raw);
 	}
 
-	if (perf_ibs == &perf_ibs_op)
-		perf_ibs_parse_ld_st_data(event->attr.sample_type, &ibs_data, &data);
-
 	/*
 	 * rip recorded by IbsOpRip will not be consistent with rsp and rbp
 	 * recorded as part of interrupt regs. Thus we need to use rip from
-- 
2.49.0.rc1.451.g8f38331e32-goog

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:

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

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

Hi Namhyung,

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

IBS allows PERF_SAMPLE_RAW irrespective of perf_event_paranoid. e.g.:

  $ cat /proc/sys/kernel/perf_event_paranoid
  2

  $ ./perf record -e ibs_op/swfilt=1/u --raw-samples -- make
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 1.371 MB perf.data (3957 samples) ]

  $ ./perf script -D | egrep -A2 "LdOp 1.*DcLinAddrValid 1" | egrep "IbsDCLinAd:\s*f"
  IbsDCLinAd:     fffffe00000000e8

We have two options:
1) Restrict IBS + PERF_SAMPLE_RAW to privilege users.
2) Remove all sensitive information from raw register dump before
   passing it to userspace. (Kernel data addresses and all physical
   addresses are the only sensitive info I suppose?).

2 is better IMO since it will allow unprivileged user to use IBS
with full potential. wdyt?

Thanks,
Ravi

On Wed, Mar 19, 2025 at 04:24:12PM +0530, Ravi Bangoria wrote:
> Hi Namhyung,
> 
> >>> 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.
> 
> IBS allows PERF_SAMPLE_RAW irrespective of perf_event_paranoid. e.g.:
> 
>   $ cat /proc/sys/kernel/perf_event_paranoid
>   2
> 
>   $ ./perf record -e ibs_op/swfilt=1/u --raw-samples -- make
>   [ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 1.371 MB perf.data (3957 samples) ]
> 
>   $ ./perf script -D | egrep -A2 "LdOp 1.*DcLinAddrValid 1" | egrep "IbsDCLinAd:\s*f"
>   IbsDCLinAd:     fffffe00000000e8

Oh, I thought it was enforced in the core layer but it turns out that
it's checked only by tracepoint events.

> 
> We have two options:
> 1) Restrict IBS + PERF_SAMPLE_RAW to privilege users.
> 2) Remove all sensitive information from raw register dump before
>    passing it to userspace. (Kernel data addresses and all physical
>    addresses are the only sensitive info I suppose?).
> 
> 2 is better IMO since it will allow unprivileged user to use IBS
> with full potential. wdyt?

I'm slightly inclined to #1 for simplicity and safety, but #2 is fine to
me as well.

Thanks,
Namhyung

* Namhyung Kim <namhyung@kernel.org> wrote:

> > We have two options:
> > 1) Restrict IBS + PERF_SAMPLE_RAW to privilege users.
> > 2) Remove all sensitive information from raw register dump before
> >    passing it to userspace. (Kernel data addresses and all physical
> >    addresses are the only sensitive info I suppose?).
> > 
> > 2 is better IMO since it will allow unprivileged user to use IBS
> > with full potential. wdyt?
> 
> I'm slightly inclined to #1 for simplicity and safety, but #2 is fine 
> to me as well.

I'd prefer #2 for superior usability, if it doesn't get too 
complicated.

Thanks,

	Ingo

Hi Namhyung, Ingo,

>>> We have two options:
>>> 1) Restrict IBS + PERF_SAMPLE_RAW to privilege users.
>>> 2) Remove all sensitive information from raw register dump before
>>>    passing it to userspace. (Kernel data addresses and all physical
>>>    addresses are the only sensitive info I suppose?).
>>>
>>> 2 is better IMO since it will allow unprivileged user to use IBS
>>> with full potential. wdyt?
>>
>> I'm slightly inclined to #1 for simplicity and safety, but #2 is fine 
>> to me as well.
> 
> I'd prefer #2 for superior usability, if it doesn't get too 
> complicated.

Thank you for your opinion.

I've posted a v3 based on 2nd option:
https://lore.kernel.org/r/20250321161251.1033-1-ravi.bangoria@amd.com 

@Ingo, can you please revert v2 and apply v3? (provided @Namhyung is okay
with it).

Thanks,
Ravi

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     65a99264f5e5a2bcc8c905f7b2d633e8991672ac
Gitweb:        https://git.kernel.org/tip/65a99264f5e5a2bcc8c905f7b2d633e8991672ac
Author:        Namhyung Kim <namhyung@kernel.org>
AuthorDate:    Mon, 17 Mar 2025 09:37:55 -07:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Mon, 17 Mar 2025 23:37:31 +01:00

perf/x86: Check data address for IBS software filter

The IBS software filter is filtering kernel samples for regular users in
the PMI handler.  It checks the instruction address in the IBS register to
determine if it was in kernel mode or not.

But it turns out that it's possible to report a kernel data address even
if the instruction address belongs to user-space.  Matteo Rizzo
found that when an instruction raises an exception, IBS can report some
kernel data addresses 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.

[ mingo: Clarified the changelog ]

Suggested-by: Matteo Rizzo <matteorizzo@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: https://lore.kernel.org/r/20250317163755.1842589-1-namhyung@kernel.org
---
 arch/x86/events/amd/ibs.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index e7a8b87..c465005 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -1128,8 +1128,13 @@ fail:
 		regs.flags |= PERF_EFLAGS_EXACT;
 	}
 
+	if (perf_ibs == &perf_ibs_op)
+		perf_ibs_parse_ld_st_data(event->attr.sample_type, &ibs_data, &data);
+
 	if ((event->attr.config2 & IBS_SW_FILTER_MASK) &&
-	    perf_exclude_event(event, &regs)) {
+	    (perf_exclude_event(event, &regs) ||
+	     ((data.sample_flags & PERF_SAMPLE_ADDR) &&
+	      event->attr.exclude_kernel && kernel_ip(data.addr)))) {
 		throttle = perf_event_account_interrupt(event);
 		goto out;
 	}
@@ -1144,9 +1149,6 @@ fail:
 		perf_sample_save_raw_data(&data, event, &raw);
 	}
 
-	if (perf_ibs == &perf_ibs_op)
-		perf_ibs_parse_ld_st_data(event->attr.sample_type, &ibs_data, &data);
-
 	/*
 	 * rip recorded by IbsOpRip will not be consistent with rsp and rbp
 	 * recorded as part of interrupt regs. Thus we need to use rip from