Re: [PATCH 2/2] powerpc/perf: Fix the kernel address leak to userspace via SDAR

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