[PATCH 1/2] powerpc/perf: Fix kernel address leak to userspace via BHRB buffer

The current Branch History Rolling Buffer (BHRB) code does
not check for any privilege levels before updating the data
from BHRB. This leaks kernel addresses to userspace even when
profiling only with userspace privileges. Add proper checks
to prevent it.

Signed-off-by: Madhavan Srinivasan <redacted>
---
 arch/powerpc/perf/core-book3s.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index f89bbd54ecec..337db5831749 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c

@@ -457,6 +457,10 @@ static void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
 				/* invalid entry */
 				continue;
 
+			if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN) &&
+				is_kernel_addr(addr))
+				continue;
+
 			/* Branches are read most recent first (ie. mfbhrb 0 is
 			 * the most recent branch).
 			 * There are two types of valid entries:

-- 
2.7.4

Sampled Data Address Register (SDAR) is a 64-bit
register that contains the effective address of
the storage operand of an instruction that was
being executed, possibly out-of-order, at or around
the time that the Performance Monitor alert occurred.

In certain scenario SDAR happen to contain the kernel
address even for userspace only sampling. Add checks
to prevent it.

Signed-off-by: Madhavan Srinivasan <redacted>
---
 arch/powerpc/perf/core-book3s.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 337db5831749..c4525323d691 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c

@@ -95,7 +95,7 @@ static inline unsigned long perf_ip_adjust(struct pt_regs *regs)
 {
 	return 0;
 }
-static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp) { }
+static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp, struct perf_event *event) { }
 static inline u32 perf_get_misc_flags(struct pt_regs *regs)
 {
 	return 0;

@@ -174,7 +174,7 @@ static inline unsigned long perf_ip_adjust(struct pt_regs *regs)
  * pointed to by SIAR; this is indicated by the [POWER6_]MMCRA_SDSYNC, the
  * [POWER7P_]MMCRA_SDAR_VALID bit in MMCRA, or the SDAR_VALID bit in SIER.
  */
-static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp)
+static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp, struct perf_event *event)
 {
 	unsigned long mmcra = regs->dsisr;
 	bool sdar_valid;

@@ -198,6 +198,11 @@ static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp)
 
 	if (!(mmcra & MMCRA_SAMPLE_ENABLE) || sdar_valid)
 		*addrp = mfspr(SPRN_SDAR);
+
+	if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN) &&
+		(event->attr.exclude_kernel || event->attr.exclude_hv) &&
+		is_kernel_addr(mfspr(SPRN_SDAR)))
+		*addrp = 0;
 }
 
 static bool regs_sihv(struct pt_regs *regs)

@@ -2054,7 +2059,7 @@ static void record_and_restart(struct perf_event *event, unsigned long val,
 
 		if (event->attr.sample_type &
 		    (PERF_SAMPLE_ADDR | PERF_SAMPLE_PHYS_ADDR))
-			perf_get_data_addr(regs, &data.addr);
+			perf_get_data_addr(regs, &data.addr, event);
 
 		if (event->attr.sample_type & PERF_SAMPLE_BRANCH_STACK) {
 			struct cpu_hw_events *cpuhw;

-- 
2.7.4

On Sun, Mar 4, 2018 at 10:55 PM, Madhavan Srinivasan
[off-list ref] wrote:

Looks good to me. The scope of the leaks concern is KASLR related or
something else (figuring out what's in the cache?)

Acked-by: Balbir Singh <bsingharora@gmail.com>

Balbir Singh.

Madhavan Srinivasan wrote:

k3s.c

gs *regs)

 {
 	return 0;
 }
-static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp) =

{ }

+static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp, =

struct perf_event *event) { }

regs *regs)

  * pointed to by SIAR; this is indicated by the [POWER6_]MMCRA_SDSYNC, t=

he

  * [POWER7P_]MMCRA_SDAR_VALID bit in MMCRA, or the SDAR_VALID bit in SIE=

R.

  */
-static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp)
+static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp, =

struct perf_event *event)

 *regs, u64 *addrp)

=20
 	if (!(mmcra & MMCRA_SAMPLE_ENABLE) || sdar_valid)
 		*addrp =3D mfspr(SPRN_SDAR);
+
+	if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN) &&
+		(event->attr.exclude_kernel || event->attr.exclude_hv) &&

I may be missing something, but if !capable(CAP_SYS_ADMIN), should we=20
still check the exclude_kernel/exclude_hv fields in the event attribute? =20
Aren't those user controlled?

- Naveen

vent, unsigned long val,

=20
 		if (event->attr.sample_type &
 		    (PERF_SAMPLE_ADDR | PERF_SAMPLE_PHYS_ADDR))
-			perf_get_data_addr(regs, &data.addr);
+			perf_get_data_addr(regs, &data.addr, event);
=20
 		if (event->attr.sample_type & PERF_SAMPLE_BRANCH_STACK) {
 			struct cpu_hw_events *cpuhw;
--=20
2.7.4
=20
=20

=

On Monday 05 March 2018 01:51 PM, Naveen N. Rao wrote:

Madhavan Srinivasan wrote:

quoted

Sampled Data Address Register (SDAR) is a 64-bit
register that contains the effective address of
the storage operand of an instruction that was
being executed, possibly out-of-order, at or around
the time that the Performance Monitor alert occurred.

In certain scenario SDAR happen to contain the kernel
address even for userspace only sampling. Add checks
to prevent it.

Signed-off-by: Madhavan Srinivasan <redacted>
---
 arch/powerpc/perf/core-book3s.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/perf/core-book3s.c 

b/arch/powerpc/perf/core-book3s.c
index 337db5831749..c4525323d691 100644

--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c

@@ -95,7 +95,7 @@ static inline unsigned long perf_ip_adjust(struct 

pt_regs *regs)
 {
     return 0;
 }
-static inline void perf_get_data_addr(struct pt_regs *regs, u64 
*addrp) { }
+static inline void perf_get_data_addr(struct pt_regs *regs, u64 
*addrp, struct perf_event *event) { }
 static inline u32 perf_get_misc_flags(struct pt_regs *regs)
 {
     return 0;

@@ -174,7 +174,7 @@ static inline unsigned long perf_ip_adjust(struct 

pt_regs *regs)
  * pointed to by SIAR; this is indicated by the 
[POWER6_]MMCRA_SDSYNC, the
  * [POWER7P_]MMCRA_SDAR_VALID bit in MMCRA, or the SDAR_VALID bit in 
SIER.
  */
-static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp)
+static inline void perf_get_data_addr(struct pt_regs *regs, u64 
*addrp, struct perf_event *event)
 {
     unsigned long mmcra = regs->dsisr;
     bool sdar_valid;

@@ -198,6 +198,11 @@ static inline void perf_get_data_addr(struct 

pt_regs *regs, u64 *addrp)

     if (!(mmcra & MMCRA_SAMPLE_ENABLE) || sdar_valid)
         *addrp = mfspr(SPRN_SDAR);
+
+    if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN) &&
+        (event->attr.exclude_kernel || event->attr.exclude_hv) &&

I may be missing something, but if !capable(CAP_SYS_ADMIN), should we 
still check the exclude_kernel/exclude_hv fields in the event 
attribute?  Aren't those user controlled?

Yes that right. But i also want to handle the case when we sampling only 
for userspace even with higher privilege level. May be I should handle 
that as a separate patch.
I will respin this patch to check only for the privilege level and 
change the commit message accordingly.

Thanks for review
Maddy

- Naveen

quoted

+        is_kernel_addr(mfspr(SPRN_SDAR)))
+        *addrp = 0;
 }

 static bool regs_sihv(struct pt_regs *regs)

@@ -2054,7 +2059,7 @@ static void record_and_restart(struct 

perf_event *event, unsigned long val,

         if (event->attr.sample_type &
             (PERF_SAMPLE_ADDR | PERF_SAMPLE_PHYS_ADDR))
-            perf_get_data_addr(regs, &data.addr);
+            perf_get_data_addr(regs, &data.addr, event);

         if (event->attr.sample_type & PERF_SAMPLE_BRANCH_STACK) {
             struct cpu_hw_events *cpuhw;
-- 
2.7.4

On Monday 05 March 2018 11:46 AM, Balbir Singh wrote:

On Sun, Mar 4, 2018 at 10:55 PM, Madhavan Srinivasan
[off-list ref] wrote:

quoted

The current Branch History Rolling Buffer (BHRB) code does
not check for any privilege levels before updating the data
from BHRB. This leaks kernel addresses to userspace even when
profiling only with userspace privileges. Add proper checks
to prevent it.

Signed-off-by: Madhavan Srinivasan <redacted>
---
  arch/powerpc/perf/core-book3s.c | 4 ++++
  1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index f89bbd54ecec..337db5831749 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c

@@ -457,6 +457,10 @@ static void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
                                 /* invalid entry */
                                 continue;

+                       if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN) &&
+                               is_kernel_addr(addr))
+                               continue;
+

Looks good to me. The scope of the leaks concern is KASLR related or
something else (figuring out what's in the cache?)

I did not look at it closely. But will get the information.

Thanks for the review
Maddy

Acked-by: Balbir Singh <bsingharora@gmail.com>

Balbir Singh.