Re: [PATCH] powerpc/perf: Fix handling of privilege level checks in perf interrupt context
> On 23-Feb-2021, at 6:24 PM, Michael Ellerman wrote: > > Peter Zijlstra writes: >> On Tue, Feb 23, 2021 at 01:31:49AM -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: > ... >>> >>> 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. >>> But in case of powerpc PMU, we need the privilege checks for specific >>> samples from branch history ring buffer and sampling register values. >> >> I'm confused... why would you need those checks at event time? Either >> the event has perf_event_attr::exclude_kernel and it then isn't allowed >> to expose kernel addresses, or it doesn't and it is. > > Well one of us is confused that's for sure ^_^ > > I missed/forgot that we had that logic in open. > > I think the reason we got here is that in the past we didn't have the > event in the low-level routines where we want to check, > power_pmu_bhrb_read() and perf_get_data_addr(), so we hacked in a > perf_paranoid_kernel() check. Which was wrong. > > Then Joel's patch plumbed the event through and switched those paranoid > checks to perf_allow_kernel(). > > Anyway, we'll just switch those to exclude_kernel checks. > >> There should never be an event-time question of permission like this. If >> you allow creation of an event, you're allowing the data it generates. > > Ack. Thanks for all the reviews. I will send a V2 with using 'event->attr.exclude_kernel' in the checks. Athira > > cheers
Re: [PATCH] powerpc/perf: Fix handling of privilege level checks in perf interrupt context
Peter Zijlstra writes: > On Tue, Feb 23, 2021 at 01:31:49AM -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: ... >> >> 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. >> But in case of powerpc PMU, we need the privilege checks for specific >> samples from branch history ring buffer and sampling register values. > > I'm confused... why would you need those checks at event time? Either > the event has perf_event_attr::exclude_kernel and it then isn't allowed > to expose kernel addresses, or it doesn't and it is. Well one of us is confused that's for sure ^_^ I missed/forgot that we had that logic in open. I think the reason we got here is that in the past we didn't have the event in the low-level routines where we want to check, power_pmu_bhrb_read() and perf_get_data_addr(), so we hacked in a perf_paranoid_kernel() check. Which was wrong. Then Joel's patch plumbed the event through and switched those paranoid checks to perf_allow_kernel(). Anyway, we'll just switch those to exclude_kernel checks. > There should never be an event-time question of permission like this. If > you allow creation of an event, you're allowing the data it generates. Ack. cheers
Re: [PATCH] powerpc/perf: Fix handling of privilege level checks in perf interrupt context
On Tue, Feb 23, 2021 at 01:31:49AM -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_exception+0x4c/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 is 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. > But in case of powerpc PMU, we need the privilege checks for specific > samples from branch history ring buffer and sampling register values. I'm confused... why would you need those checks at event time? Either the event has perf_event_attr::exclude_kernel and it then isn't allowed to expose kernel addresses, or it doesn't and it is. There should never be an event-time question of permission like this. If you allow creation of an event, you're allowing the data it generates.
Re: [PATCH] powerpc/perf: Fix handling of privilege level checks in perf interrupt context
(CC'ing LSM and SELinux lists; the initial message can be found here: https://lore.kernel.org/linuxppc-dev/1614061909-1734-1-git-send-email-atraj...@linux.vnet.ibm.com/T/) On Tue, Feb 23, 2021 at 7:32 AM 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_exception+0x4c/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 is 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. > But in case of powerpc PMU, we need the privilege checks for specific > samples from branch history ring buffer and sampling register values. > 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") > > As a fix, patch caches 'perf_allow_kernel' value in event_init in > 'pmu_private' field of perf_event. The cached value is used in the > PMI code path. > > Suggested-by: Michael Ellerman > Signed-off-by: Athira Rajeev > --- > arch/powerpc/perf/core-book3s.c | 16 ++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c > index 4b4319d8..9e9f67f 100644 > --- a/arch/powerpc/perf/core-book3s.c > +++ b/arch/powerpc/perf/core-book3s.c > @@ -189,6 +189,11 @@ static inline unsigned long perf_ip_adjust(struct > pt_regs *regs) > return 0; > } > > +static bool event_allow_kernel(struct perf_event *event) > +{ > +
[PATCH] 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_exception+0x4c/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 is 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. But in case of powerpc PMU, we need the privilege checks for specific samples from branch history ring buffer and sampling register values. 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") As a fix, patch caches 'perf_allow_kernel' value in event_init in 'pmu_private' field of perf_event. The cached value is used in the PMI code path. Suggested-by: Michael Ellerman Signed-off-by: Athira Rajeev --- arch/powerpc/perf/core-book3s.c | 16 ++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c index 4b4319d8..9e9f67f 100644 --- a/arch/powerpc/perf/core-book3s.c +++ b/arch/powerpc/perf/core-book3s.c @@ -189,6 +189,11 @@ static inline unsigned long perf_ip_adjust(struct pt_regs *regs) return 0; } +static bool event_allow_kernel(struct perf_event *event) +{ + return (bool)event->pmu_private; +} + /* * The user wants a data address recorded. * If we're not doing instruction sampling, give them the SDAR @@ -222,7 +227,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)) &&