public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] perf/x86: Check data address for IBS software filter
@ 2025-03-17  8:10 Namhyung Kim
  2025-03-17  9:15 ` [tip: perf/urgent] " tip-bot2 for Namhyung Kim
  2025-03-17 13:29 ` [PATCH] " Ravi Bangoria
  0 siblings, 2 replies; 10+ messages in thread
From: Namhyung Kim @ 2025-03-17  8:10 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar
  Cc: Kan Liang, Mark Rutland, Alexander Shishkin,
	Arnaldo Carvalho de Melo, LKML, Matteo Rizzo, Ravi Bangoria

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>
---
 arch/x86/events/amd/ibs.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index 7b52b8e3a185157f..8b3b76fad587b3ff 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -1286,6 +1286,13 @@ static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs)
 	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) &&
+	    (event->attr.sample_type & PERF_SAMPLE_ADDR) &&
+	    event->attr.exclude_kernel && !access_ok(data.addr)) {
+		throttle = perf_event_account_interrupt(event);
+		goto out;
+	}
+
 	/*
 	 * 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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [tip: perf/urgent] perf/x86: Check data address for IBS software filter
  2025-03-17  8:10 [PATCH] perf/x86: Check data address for IBS software filter Namhyung Kim
@ 2025-03-17  9:15 ` tip-bot2 for Namhyung Kim
  2025-03-17 10:10   ` Borislav Petkov
  2025-03-17 10:15   ` Peter Zijlstra
  2025-03-17 13:29 ` [PATCH] " Ravi Bangoria
  1 sibling, 2 replies; 10+ messages in thread
From: tip-bot2 for Namhyung Kim @ 2025-03-17  9:15 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Matteo Rizzo, Namhyung Kim, Ingo Molnar, Peter Zijlstra, x86,
	linux-kernel

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

Commit-ID:     b0be17d8108bf3448a58be319d085155a128cf3a
Gitweb:        https://git.kernel.org/tip/b0be17d8108bf3448a58be319d085155a128cf3a
Author:        Namhyung Kim <namhyung@kernel.org>
AuthorDate:    Mon, 17 Mar 2025 01:10:58 -07:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Mon, 17 Mar 2025 10:04:31 +01:00

perf/x86: Check data address for IBS software filter

The IBS software filter is filtering 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 mode 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>
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/20250317081058.1794729-1-namhyung@kernel.org
---
 arch/x86/events/amd/ibs.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index e7a8b87..24985c7 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -1147,6 +1147,13 @@ fail:
 	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) &&
+	    (event->attr.sample_type & PERF_SAMPLE_ADDR) &&
+	    event->attr.exclude_kernel && !access_ok(data.addr)) {
+		throttle = perf_event_account_interrupt(event);
+		goto out;
+	}
+
 	/*
 	 * 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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [tip: perf/urgent] perf/x86: Check data address for IBS software filter
  2025-03-17  9:15 ` [tip: perf/urgent] " tip-bot2 for Namhyung Kim
@ 2025-03-17 10:10   ` Borislav Petkov
  2025-03-17 15:57     ` Namhyung Kim
  2025-03-17 10:15   ` Peter Zijlstra
  1 sibling, 1 reply; 10+ messages in thread
From: Borislav Petkov @ 2025-03-17 10:10 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-tip-commits, Matteo Rizzo, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, x86

On Mon, Mar 17, 2025 at 09:15:05AM -0000, tip-bot2 for Namhyung Kim wrote:
> The following commit has been merged into the perf/urgent branch of tip:
> 
> Commit-ID:     b0be17d8108bf3448a58be319d085155a128cf3a
> Gitweb:        https://git.kernel.org/tip/b0be17d8108bf3448a58be319d085155a128cf3a
> Author:        Namhyung Kim <namhyung@kernel.org>
> AuthorDate:    Mon, 17 Mar 2025 01:10:58 -07:00
> Committer:     Ingo Molnar <mingo@kernel.org>
> CommitterDate: Mon, 17 Mar 2025 10:04:31 +01:00
> 
> perf/x86: Check data address for IBS software filter
> 
> The IBS software filter is filtering 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 mode 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>
> 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/20250317081058.1794729-1-namhyung@kernel.org
> ---
>  arch/x86/events/amd/ibs.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
> index e7a8b87..24985c7 100644
> --- a/arch/x86/events/amd/ibs.c
> +++ b/arch/x86/events/amd/ibs.c
> @@ -1147,6 +1147,13 @@ fail:
>  	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) &&
> +	    (event->attr.sample_type & PERF_SAMPLE_ADDR) &&
> +	    event->attr.exclude_kernel && !access_ok(data.addr)) {
> +		throttle = perf_event_account_interrupt(event);
> +		goto out;
> +	}

Did anyone build this?

arch/x86/events/amd/ibs.c: In function ‘perf_ibs_handle_irq’:
arch/x86/events/amd/ibs.c:1291:63: error: macro "access_ok" requires 2 arguments, but only 1 given
 1291 |             event->attr.exclude_kernel && !access_ok(data.addr)) {
      |                                                               ^
In file included from ./arch/x86/include/asm/uaccess.h:25,
                 from ./include/linux/uaccess.h:12,
                 from ./include/linux/sched/task.h:13,
                 from ./include/linux/sched/signal.h:9,
                 from ./include/linux/ptrace.h:7,
                 from ./include/uapi/asm-generic/bpf_perf_event.h:4,
                 from ./arch/x86/include/generated/uapi/asm/bpf_perf_event.h:1,
                 from ./include/uapi/linux/bpf_perf_event.h:11,
                 from ./include/linux/perf_event.h:18,
                 from arch/x86/events/amd/ibs.c:9:
./include/asm-generic/access_ok.h:45: note: macro "access_ok" defined here
   45 | #define access_ok(addr, size) likely(__access_ok(addr, size))
      | 
arch/x86/events/amd/ibs.c:1291:44: error: ‘access_ok’ undeclared (first use in this function)
 1291 |             event->attr.exclude_kernel && !access_ok(data.addr)) {
      |                                            ^~~~~~~~~
arch/x86/events/amd/ibs.c:1291:44: note: each undeclared identifier is reported only once for each function it appears in
make[5]: *** [scripts/Makefile.build:207: arch/x86/events/amd/ibs.o] Error 1
make[4]: *** [scripts/Makefile.build:465: arch/x86/events/amd] Error 2
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [scripts/Makefile.build:465: arch/x86/events] Error 2
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [scripts/Makefile.build:465: arch/x86] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/mnt/kernel/kernel/6th/linux/Makefile:1997: .] Error 2
make: *** [Makefile:251: __sub-make] Error 2

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [tip: perf/urgent] perf/x86: Check data address for IBS software filter
  2025-03-17  9:15 ` [tip: perf/urgent] " tip-bot2 for Namhyung Kim
  2025-03-17 10:10   ` Borislav Petkov
@ 2025-03-17 10:15   ` Peter Zijlstra
  2025-03-17 16:01     ` Namhyung Kim
  1 sibling, 1 reply; 10+ messages in thread
From: Peter Zijlstra @ 2025-03-17 10:15 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-tip-commits, Matteo Rizzo, Namhyung Kim, Ingo Molnar, x86

On Mon, Mar 17, 2025 at 09:15:05AM -0000, tip-bot2 for Namhyung Kim wrote:
> The following commit has been merged into the perf/urgent branch of tip:
> 
> Commit-ID:     b0be17d8108bf3448a58be319d085155a128cf3a
> Gitweb:        https://git.kernel.org/tip/b0be17d8108bf3448a58be319d085155a128cf3a
> Author:        Namhyung Kim <namhyung@kernel.org>
> AuthorDate:    Mon, 17 Mar 2025 01:10:58 -07:00
> Committer:     Ingo Molnar <mingo@kernel.org>
> CommitterDate: Mon, 17 Mar 2025 10:04:31 +01:00
> 
> perf/x86: Check data address for IBS software filter
> 
> The IBS software filter is filtering 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 mode 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>
> 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/20250317081058.1794729-1-namhyung@kernel.org
> ---
>  arch/x86/events/amd/ibs.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
> index e7a8b87..24985c7 100644
> --- a/arch/x86/events/amd/ibs.c
> +++ b/arch/x86/events/amd/ibs.c
> @@ -1147,6 +1147,13 @@ fail:
>  	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) &&
> +	    (event->attr.sample_type & PERF_SAMPLE_ADDR) &&
> +	    event->attr.exclude_kernel && !access_ok(data.addr)) {

If only you'd looked at all the other filter code :/ everybody uses
kernel_ip() helper for this, not access_ok().

> +		throttle = perf_event_account_interrupt(event);
> +		goto out;
> +	}
> +
>  	/*
>  	 * 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] perf/x86: Check data address for IBS software filter
  2025-03-17  8:10 [PATCH] perf/x86: Check data address for IBS software filter Namhyung Kim
  2025-03-17  9:15 ` [tip: perf/urgent] " tip-bot2 for Namhyung Kim
@ 2025-03-17 13:29 ` Ravi Bangoria
  2025-03-17 16:02   ` Namhyung Kim
  1 sibling, 1 reply; 10+ messages in thread
From: Ravi Bangoria @ 2025-03-17 13:29 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Peter Zijlstra, Ingo Molnar, Kan Liang, Mark Rutland,
	Alexander Shishkin, Arnaldo Carvalho de Melo, LKML, Matteo Rizzo,
	Ravi Bangoria

Hi Namhyung,

> @@ -1286,6 +1286,13 @@ static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs)
>  	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) &&
> +	    (event->attr.sample_type & PERF_SAMPLE_ADDR) &&
> +	    event->attr.exclude_kernel && !access_ok(data.addr)) {
> +		throttle = perf_event_account_interrupt(event);
> +		goto out;
> +	}

Can this move up where it checks perf_exclude_event()? Use
ibs_data.regs[ibs_op_msr_idx(MSR_AMD64_IBSDCLINAD)] instead
of data.addr.

Thanks,
Ravi

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [tip: perf/urgent] perf/x86: Check data address for IBS software filter
  2025-03-17 10:10   ` Borislav Petkov
@ 2025-03-17 15:57     ` Namhyung Kim
  0 siblings, 0 replies; 10+ messages in thread
From: Namhyung Kim @ 2025-03-17 15:57 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: linux-kernel, linux-tip-commits, Matteo Rizzo, Ingo Molnar,
	Peter Zijlstra, x86

On Mon, Mar 17, 2025 at 11:10:48AM +0100, Borislav Petkov wrote:
> On Mon, Mar 17, 2025 at 09:15:05AM -0000, tip-bot2 for Namhyung Kim wrote:
> > The following commit has been merged into the perf/urgent branch of tip:
> > 
> > Commit-ID:     b0be17d8108bf3448a58be319d085155a128cf3a
> > Gitweb:        https://git.kernel.org/tip/b0be17d8108bf3448a58be319d085155a128cf3a
> > Author:        Namhyung Kim <namhyung@kernel.org>
> > AuthorDate:    Mon, 17 Mar 2025 01:10:58 -07:00
> > Committer:     Ingo Molnar <mingo@kernel.org>
> > CommitterDate: Mon, 17 Mar 2025 10:04:31 +01:00
> > 
> > perf/x86: Check data address for IBS software filter
> > 
> > The IBS software filter is filtering 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 mode 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>
> > 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/20250317081058.1794729-1-namhyung@kernel.org
> > ---
> >  arch/x86/events/amd/ibs.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
> > index e7a8b87..24985c7 100644
> > --- a/arch/x86/events/amd/ibs.c
> > +++ b/arch/x86/events/amd/ibs.c
> > @@ -1147,6 +1147,13 @@ fail:
> >  	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) &&
> > +	    (event->attr.sample_type & PERF_SAMPLE_ADDR) &&
> > +	    event->attr.exclude_kernel && !access_ok(data.addr)) {
> > +		throttle = perf_event_account_interrupt(event);
> > +		goto out;
> > +	}
> 
> Did anyone build this?

Oops, sorry about this.  I fixed it locally but sent the one before
adding the change.  Will send v2.

Thanks,
Namhyung


> 
> arch/x86/events/amd/ibs.c: In function ‘perf_ibs_handle_irq’:
> arch/x86/events/amd/ibs.c:1291:63: error: macro "access_ok" requires 2 arguments, but only 1 given
>  1291 |             event->attr.exclude_kernel && !access_ok(data.addr)) {
>       |                                                               ^
> In file included from ./arch/x86/include/asm/uaccess.h:25,
>                  from ./include/linux/uaccess.h:12,
>                  from ./include/linux/sched/task.h:13,
>                  from ./include/linux/sched/signal.h:9,
>                  from ./include/linux/ptrace.h:7,
>                  from ./include/uapi/asm-generic/bpf_perf_event.h:4,
>                  from ./arch/x86/include/generated/uapi/asm/bpf_perf_event.h:1,
>                  from ./include/uapi/linux/bpf_perf_event.h:11,
>                  from ./include/linux/perf_event.h:18,
>                  from arch/x86/events/amd/ibs.c:9:
> ./include/asm-generic/access_ok.h:45: note: macro "access_ok" defined here
>    45 | #define access_ok(addr, size) likely(__access_ok(addr, size))
>       | 
> arch/x86/events/amd/ibs.c:1291:44: error: ‘access_ok’ undeclared (first use in this function)
>  1291 |             event->attr.exclude_kernel && !access_ok(data.addr)) {
>       |                                            ^~~~~~~~~
> arch/x86/events/amd/ibs.c:1291:44: note: each undeclared identifier is reported only once for each function it appears in
> make[5]: *** [scripts/Makefile.build:207: arch/x86/events/amd/ibs.o] Error 1
> make[4]: *** [scripts/Makefile.build:465: arch/x86/events/amd] Error 2
> make[4]: *** Waiting for unfinished jobs....
> make[3]: *** [scripts/Makefile.build:465: arch/x86/events] Error 2
> make[3]: *** Waiting for unfinished jobs....
> make[2]: *** [scripts/Makefile.build:465: arch/x86] Error 2
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [/mnt/kernel/kernel/6th/linux/Makefile:1997: .] Error 2
> make: *** [Makefile:251: __sub-make] Error 2
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [tip: perf/urgent] perf/x86: Check data address for IBS software filter
  2025-03-17 10:15   ` Peter Zijlstra
@ 2025-03-17 16:01     ` Namhyung Kim
  0 siblings, 0 replies; 10+ messages in thread
From: Namhyung Kim @ 2025-03-17 16:01 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: linux-kernel, linux-tip-commits, Matteo Rizzo, Ingo Molnar, x86

Hi Peter,

On Mon, Mar 17, 2025 at 11:15:04AM +0100, Peter Zijlstra wrote:
> On Mon, Mar 17, 2025 at 09:15:05AM -0000, tip-bot2 for Namhyung Kim wrote:
> > The following commit has been merged into the perf/urgent branch of tip:
> > 
> > Commit-ID:     b0be17d8108bf3448a58be319d085155a128cf3a
> > Gitweb:        https://git.kernel.org/tip/b0be17d8108bf3448a58be319d085155a128cf3a
> > Author:        Namhyung Kim <namhyung@kernel.org>
> > AuthorDate:    Mon, 17 Mar 2025 01:10:58 -07:00
> > Committer:     Ingo Molnar <mingo@kernel.org>
> > CommitterDate: Mon, 17 Mar 2025 10:04:31 +01:00
> > 
> > perf/x86: Check data address for IBS software filter
> > 
> > The IBS software filter is filtering 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 mode 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>
> > 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/20250317081058.1794729-1-namhyung@kernel.org
> > ---
> >  arch/x86/events/amd/ibs.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
> > index e7a8b87..24985c7 100644
> > --- a/arch/x86/events/amd/ibs.c
> > +++ b/arch/x86/events/amd/ibs.c
> > @@ -1147,6 +1147,13 @@ fail:
> >  	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) &&
> > +	    (event->attr.sample_type & PERF_SAMPLE_ADDR) &&
> > +	    event->attr.exclude_kernel && !access_ok(data.addr)) {
> 
> If only you'd looked at all the other filter code :/ everybody uses
> kernel_ip() helper for this, not access_ok().

I thought it also needs __KERNEL_CS but now I see it only checks the
address.  Will change in v2.

Thanks,
Namhyung


> 
> > +		throttle = perf_event_account_interrupt(event);
> > +		goto out;
> > +	}
> > +
> >  	/*
> >  	 * 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] perf/x86: Check data address for IBS software filter
  2025-03-17 13:29 ` [PATCH] " Ravi Bangoria
@ 2025-03-17 16:02   ` Namhyung Kim
  2025-03-17 16:23     ` Namhyung Kim
  0 siblings, 1 reply; 10+ messages in thread
From: Namhyung Kim @ 2025-03-17 16:02 UTC (permalink / raw)
  To: Ravi Bangoria
  Cc: Peter Zijlstra, Ingo Molnar, Kan Liang, Mark Rutland,
	Alexander Shishkin, Arnaldo Carvalho de Melo, LKML, Matteo Rizzo

Hi Ravi,

On Mon, Mar 17, 2025 at 06:59:33PM +0530, Ravi Bangoria wrote:
> Hi Namhyung,
> 
> > @@ -1286,6 +1286,13 @@ static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs)
> >  	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) &&
> > +	    (event->attr.sample_type & PERF_SAMPLE_ADDR) &&
> > +	    event->attr.exclude_kernel && !access_ok(data.addr)) {
> > +		throttle = perf_event_account_interrupt(event);
> > +		goto out;
> > +	}
> 
> Can this move up where it checks perf_exclude_event()? Use
> ibs_data.regs[ibs_op_msr_idx(MSR_AMD64_IBSDCLINAD)] instead
> of data.addr.

Sure, will do.

Thanks,
Namhyung


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] perf/x86: Check data address for IBS software filter
  2025-03-17 16:02   ` Namhyung Kim
@ 2025-03-17 16:23     ` Namhyung Kim
  0 siblings, 0 replies; 10+ messages in thread
From: Namhyung Kim @ 2025-03-17 16:23 UTC (permalink / raw)
  To: Ravi Bangoria
  Cc: Peter Zijlstra, Ingo Molnar, Kan Liang, Mark Rutland,
	Alexander Shishkin, Arnaldo Carvalho de Melo, LKML, Matteo Rizzo

On Mon, Mar 17, 2025 at 09:02:12AM -0700, Namhyung Kim wrote:
> Hi Ravi,
> 
> On Mon, Mar 17, 2025 at 06:59:33PM +0530, Ravi Bangoria wrote:
> > Hi Namhyung,
> > 
> > > @@ -1286,6 +1286,13 @@ static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs)
> > >  	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) &&
> > > +	    (event->attr.sample_type & PERF_SAMPLE_ADDR) &&
> > > +	    event->attr.exclude_kernel && !access_ok(data.addr)) {
> > > +		throttle = perf_event_account_interrupt(event);
> > > +		goto out;
> > > +	}
> > 
> > Can this move up where it checks perf_exclude_event()? Use
> > ibs_data.regs[ibs_op_msr_idx(MSR_AMD64_IBSDCLINAD)] instead
> > of data.addr.
> 
> Sure, will do.

Hmm.. but I think it also needs to check op_data3_dc_lin_addr_valid bit.
Can we move up the perf_ibs_parse_ld_st_data() too and check data.addr?

Thanks,
Namhyung


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [tip: perf/urgent] perf/x86: Check data address for IBS software filter
  2025-03-17 16:37 [PATCH v2] " Namhyung Kim
@ 2025-03-17 22:51 ` tip-bot2 for Namhyung Kim
  0 siblings, 0 replies; 10+ messages in thread
From: tip-bot2 for Namhyung Kim @ 2025-03-17 22:51 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Matteo Rizzo, Namhyung Kim, Ingo Molnar, Peter Zijlstra, x86,
	linux-kernel

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2025-03-17 22:51 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-17  8:10 [PATCH] perf/x86: Check data address for IBS software filter Namhyung Kim
2025-03-17  9:15 ` [tip: perf/urgent] " tip-bot2 for Namhyung Kim
2025-03-17 10:10   ` Borislav Petkov
2025-03-17 15:57     ` Namhyung Kim
2025-03-17 10:15   ` Peter Zijlstra
2025-03-17 16:01     ` Namhyung Kim
2025-03-17 13:29 ` [PATCH] " Ravi Bangoria
2025-03-17 16:02   ` Namhyung Kim
2025-03-17 16:23     ` Namhyung Kim
  -- strict thread matches above, loose matches on Subject: below --
2025-03-17 16:37 [PATCH v2] " Namhyung Kim
2025-03-17 22:51 ` [tip: perf/urgent] " tip-bot2 for Namhyung Kim

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox