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

[Madhavan Srinivasan <maddy@linux.vnet.ibm.com>]

On Monday 05 March 2018 01:51 PM, Naveen N. Rao wrote:
> Madhavan Srinivasan wrote:
>> 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 <maddy@linux.vnet.ibm.com>
>> ---
>>  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
>
>> +        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
>>
>>