Re: [PATCH 2/3] perf/amd/ibs: Fix kernel address leak via REGS_USER

[Ravi Bangoria <ravi.bangoria@amd.com>]

Jim,

Apologies, it took me some time to get back to this.

>> IBS hardware captures a sample and raises an NMI. Since the NMI can be
>> delivered with a delay, a privilege change may occur between sample
>> capture and NMI delivery. Because IBS only provides the sample RIP, the
>> kernel records GPRs from the NMI pt_regs, which may reflect kernel mode
>> if a privilege switch occurred in the interim. This can leak kernel
>> addresses to unprivileged users through PERF_SAMPLE_REGS_USER.
>>
>> Fix this by discarding samples where the NMI arrives in kernel mode and
>> the user has set exclude_kernel.
> 
> This will systematically under-sample instructions just before a
> transition to the kernel. For this special case, perhaps the GPRs
> could be obtained from task_pt_regs(current) instead of the NMI
> pt_regs?

Yes, that's a better approach.

>> Reported-by: Jayapal Ashwin Nair <janair@amd.com>
>> Fixes: d29e744c7167 ("perf/x86: Relax privilege filter restriction on AMD IBS")
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
>> ---
>>  arch/x86/events/amd/ibs.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
>> index bfb51f8e184f..3cb46c775eaa 100644
>> --- a/arch/x86/events/amd/ibs.c
>> +++ b/arch/x86/events/amd/ibs.c
>> @@ -1373,12 +1373,15 @@ static bool perf_ibs_is_kernel_br_target(struct perf_event *event,
>>  }
>>
>>  static bool perf_ibs_discard_sample(struct perf_ibs *perf_ibs, struct perf_event *event,
>> -                                   struct pt_regs *regs, struct perf_ibs_data *ibs_data,
>> -                                   int br_target_idx)
>> +                                   struct pt_regs *regs, struct pt_regs *iregs,
>> +                                   struct perf_ibs_data *ibs_data, int br_target_idx)
>>  {
>>         if (perf_exclude_event(event, regs))
>>                 return true;
>>
>> +       if (perf_exclude_event(event, iregs))
>> +               return true;
> 
> This excludes events with an NMI skid past a privilege change in both
> directions. Do you want to omit a kernel sample taken just before
> returning to userspace if the NMI arrives in userspace and
> exclude_user is set?

Yes, its a valid concern.

Will post a v2 with these addressed.

Thanks,
Ravi