Re: [PATCH V2] powerpc/perf: Fix handling of privilege level checks in perf interrupt context
On Thu, 25 Feb 2021 05:10:39 -0500, Athira Rajeev wrote: > Running "perf mem record" in powerpc platforms with selinux enabled > resulted in soft lockup's. Below call-trace was seen in the logs: > > CPU: 58 PID: 3751 Comm: sssd_nss Not tainted 5.11.0-rc7+ #2 > NIP: c0dff3d4 LR: c0dff3d0 CTR: > REGS: c07fffab7d60 TRAP: 0100 Not tainted (5.11.0-rc7+) > <<>> > NIP [c0dff3d4] _raw_spin_lock_irqsave+0x94/0x120 > LR [c0dff3d0] _raw_spin_lock_irqsave+0x90/0x120 > Call Trace: > [cfd471a0] [cfd47260] 0xcfd47260 (unreliable) > [cfd471e0] [c0b5fbbc] skb_queue_tail+0x3c/0x90 > [cfd47220] [c0296edc] audit_log_end+0x6c/0x180 > [cfd47260] [c06a3f20] common_lsm_audit+0xb0/0xe0 > [cfd472a0] [c066c664] slow_avc_audit+0xa4/0x110 > [cfd47320] [c066cff4] avc_has_perm+0x1c4/0x260 > [cfd47430] [c066e064] selinux_perf_event_open+0x74/0xd0 > [cfd47450] [c0669888] security_perf_event_open+0x68/0xc0 > [cfd47490] [c013d788] record_and_restart+0x6e8/0x7f0 > [cfd476c0] [c013dabc] perf_event_interrupt+0x22c/0x560 > [cfd477d0] [c002d0fc] performance_monitor_exception0x4c/0x60 > [cfd477f0] [c000b378] > performance_monitor_common_virt+0x1c8/0x1d0 > interrupt: f00 at _raw_spin_lock_irqsave+0x38/0x120 > NIP: c0dff378 LR: c0b5fbbc CTR: c07d47f0 > REGS: cfd47860 TRAP: 0f00 Not tainted (5.11.0-rc7+) > <<>> > NIP [c0dff378] _raw_spin_lock_irqsave+0x38/0x120 > LR [c0b5fbbc] skb_queue_tail+0x3c/0x90 > interrupt: f00 > [cfd47b00] [0038] 0x38 (unreliable) > [cfd47b40] [caae6200] 0xcaae6200 > [cfd47b80] [c0296edc] audit_log_end+0x6c/0x180 > [cfd47bc0] [c029f494] audit_log_exit+0x344/0xf80 > [cfd47d10] [c02a2b00] __audit_syscall_exit+0x2c0/0x320 > [cfd47d60] [c0032878] do_syscall_trace_leave+0x148/0x200 > [cfd47da0] [c003d5b4] syscall_exit_prepare+0x324/0x390 > [cfd47e10] [c000d76c] system_call_common+0xfc/0x27c > > [...] Applied to powerpc/fixes. [1/1] powerpc/perf: Fix handling of privilege level checks in perf interrupt context https://git.kernel.org/powerpc/c/5ae5fbd2107959b68ac69a8b75412208663aea88 cheers
Re: [PATCH V2] powerpc/perf: Fix handling of privilege level checks in perf interrupt context
> On 26-Feb-2021, at 3:05 PM, Peter Zijlstra wrote: > > On Thu, Feb 25, 2021 at 05:10:39AM -0500, Athira Rajeev wrote: >> diff --git a/arch/powerpc/perf/core-book3s.c >> b/arch/powerpc/perf/core-book3s.c >> index 4b4319d8..c8be44c 100644 >> --- a/arch/powerpc/perf/core-book3s.c >> +++ b/arch/powerpc/perf/core-book3s.c >> @@ -222,7 +222,7 @@ static inline void perf_get_data_addr(struct perf_event >> *event, struct pt_regs * >> if (!(mmcra & MMCRA_SAMPLE_ENABLE) || sdar_valid) >> *addrp = mfspr(SPRN_SDAR); >> >> -if (is_kernel_addr(mfspr(SPRN_SDAR)) && perf_allow_kernel(>attr) >> != 0) >> +if (is_kernel_addr(mfspr(SPRN_SDAR)) && event->attr.exclude_kernel) >> *addrp = 0; >> } >> >> @@ -507,7 +507,7 @@ static void power_pmu_bhrb_read(struct perf_event >> *event, struct cpu_hw_events * >> * addresses, hence include a check before filtering >> code >> */ >> if (!(ppmu->flags & PPMU_ARCH_31) && >> -is_kernel_addr(addr) && >> perf_allow_kernel(>attr) != 0) >> +is_kernel_addr(addr) && event->attr.exclude_kernel) >> continue; >> >> /* Branches are read most recent first (ie. mfbhrb 0 is > > Acked-by: Peter Zijlstra (Intel) Thanks Peter for reviewing the patch. Athira.
Re: [PATCH V2] powerpc/perf: Fix handling of privilege level checks in perf interrupt context
On Thu, Feb 25, 2021 at 05:10:39AM -0500, Athira Rajeev wrote: > diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c > index 4b4319d8..c8be44c 100644 > --- a/arch/powerpc/perf/core-book3s.c > +++ b/arch/powerpc/perf/core-book3s.c > @@ -222,7 +222,7 @@ static inline void perf_get_data_addr(struct perf_event > *event, struct pt_regs * > if (!(mmcra & MMCRA_SAMPLE_ENABLE) || sdar_valid) > *addrp = mfspr(SPRN_SDAR); > > - if (is_kernel_addr(mfspr(SPRN_SDAR)) && perf_allow_kernel(>attr) > != 0) > + if (is_kernel_addr(mfspr(SPRN_SDAR)) && event->attr.exclude_kernel) > *addrp = 0; > } > > @@ -507,7 +507,7 @@ static void power_pmu_bhrb_read(struct perf_event *event, > struct cpu_hw_events * >* addresses, hence include a check before filtering > code >*/ > if (!(ppmu->flags & PPMU_ARCH_31) && > - is_kernel_addr(addr) && > perf_allow_kernel(>attr) != 0) > + is_kernel_addr(addr) && event->attr.exclude_kernel) > continue; > > /* Branches are read most recent first (ie. mfbhrb 0 is Acked-by: Peter Zijlstra (Intel)
[PATCH V2] powerpc/perf: Fix handling of privilege level checks in perf interrupt context
Running "perf mem record" in powerpc platforms with selinux enabled
resulted in soft lockup's. Below call-trace was seen in the logs:
CPU: 58 PID: 3751 Comm: sssd_nss Not tainted 5.11.0-rc7+ #2
NIP: c0dff3d4 LR: c0dff3d0 CTR:
REGS: c07fffab7d60 TRAP: 0100 Not tainted (5.11.0-rc7+)
<<>>
NIP [c0dff3d4] _raw_spin_lock_irqsave+0x94/0x120
LR [c0dff3d0] _raw_spin_lock_irqsave+0x90/0x120
Call Trace:
[cfd471a0] [cfd47260] 0xcfd47260 (unreliable)
[cfd471e0] [c0b5fbbc] skb_queue_tail+0x3c/0x90
[cfd47220] [c0296edc] audit_log_end+0x6c/0x180
[cfd47260] [c06a3f20] common_lsm_audit+0xb0/0xe0
[cfd472a0] [c066c664] slow_avc_audit+0xa4/0x110
[cfd47320] [c066cff4] avc_has_perm+0x1c4/0x260
[cfd47430] [c066e064] selinux_perf_event_open+0x74/0xd0
[cfd47450] [c0669888] security_perf_event_open+0x68/0xc0
[cfd47490] [c013d788] record_and_restart+0x6e8/0x7f0
[cfd476c0] [c013dabc] perf_event_interrupt+0x22c/0x560
[cfd477d0] [c002d0fc] performance_monitor_exception0x4c/0x60
[cfd477f0] [c000b378]
performance_monitor_common_virt+0x1c8/0x1d0
interrupt: f00 at _raw_spin_lock_irqsave+0x38/0x120
NIP: c0dff378 LR: c0b5fbbc CTR: c07d47f0
REGS: cfd47860 TRAP: 0f00 Not tainted (5.11.0-rc7+)
<<>>
NIP [c0dff378] _raw_spin_lock_irqsave+0x38/0x120
LR [c0b5fbbc] skb_queue_tail+0x3c/0x90
interrupt: f00
[cfd47b00] [0038] 0x38 (unreliable)
[cfd47b40] [caae6200] 0xcaae6200
[cfd47b80] [c0296edc] audit_log_end+0x6c/0x180
[cfd47bc0] [c029f494] audit_log_exit+0x344/0xf80
[cfd47d10] [c02a2b00] __audit_syscall_exit+0x2c0/0x320
[cfd47d60] [c0032878] do_syscall_trace_leave+0x148/0x200
[cfd47da0] [c003d5b4] syscall_exit_prepare+0x324/0x390
[cfd47e10] [c000d76c] system_call_common+0xfc/0x27c
The above trace shows that while the CPU was handling a performance
monitor exception, there was a call to "security_perf_event_open"
function. In powerpc core-book3s, this function is called from
'perf_allow_kernel' check during recording of data address in the sample
via perf_get_data_addr().
Commit da97e18458fb ("perf_event: Add support for LSM and SELinux checks")
introduced security enhancements to perf. As part of this commit, the new
security hook for perf_event_open was added in all places where perf
paranoid check was previously used. In powerpc core-book3s code, originally
had paranoid checks in 'perf_get_data_addr' and 'power_pmu_bhrb_read'. So
'perf_paranoid_kernel' checks were replaced with 'perf_allow_kernel' in
these pmu helper functions as well.
The intention of paranoid checks in core-book3s was to verify privilege
access before capturing some of the sample data. Along with paranoid
checks, 'perf_allow_kernel' also does a 'security_perf_event_open'. Since
these functions are accessed while recording sample, we end up in calling
selinux_perf_event_open in PMI context. Some of the security functions
use spinlock like sidtab_sid2str_put(). If a perf interrupt hits under
a spin lock and if we end up in calling selinux hook functions in PMI
handler, this could cause a dead lock.
Since the purpose of this security hook is to control access to
perf_event_open, it is not right to call this in interrupt context.
The paranoid checks in powerpc core-book3s were done at interrupt
time which is also not correct.
Reference commits:
Commit cd1231d7035f ("powerpc/perf: Prevent kernel address leak via
perf_get_data_addr()")
Commit bb19af816025 ("powerpc/perf: Prevent kernel address leak to
userspace via BHRB buffer")
We only allow creation of events that has already passed the privilege
checks in perf_event_open. So these paranoid checks are not needed at
event time. As a fix, patch uses 'event->attr.exclude_kernel' check
to prevent exposing kernel address for userspace only sampling.
Suggested-by: Michael Ellerman
Signed-off-by: Athira Rajeev
---
Changes in v2:
- Addressed review comments from Ondrej Mosnacek and Peter Zijlstra.
Changed the approach to use 'event->attr.exclude_kernel'
check to prevent exposing kernel address for userspace only
sampling as suggested by Ondrej Mosnacek.
arch/powerpc/perf/core-book3s.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 4b4319d8..c8be44c 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -222,7 +222,7 @@ static inline void perf_get_data_addr(struct perf_event
*event, struct pt_regs *
if (!(mmcra & MMCRA_SAMPLE_ENABLE) || sdar_valid)
*addrp = mfspr(SPRN_SDAR);
- if (is_kernel_addr(mfspr(SPRN_SDAR)) &&
