From: Ondrej Mosnacek <omosnace@redhat.com> To: atrajeev@linux.vnet.ibm.com Cc: Michael Ellerman <mpe@ellerman.id.au>, linuxppc-dev@lists.ozlabs.org, maddy@linux.ibm.com, Jiri Olsa <jolsa@kernel.org>, kan.liang@linux.intel.com, Peter Zijlstra <peterz@infradead.org>, Arnaldo Carvalho de Melo <acme@kernel.org>, Linux Security Module list <linux-security-module@vger.kernel.org>, SElinux list <selinux@vger.kernel.org> Subject: Re: [PATCH] powerpc/perf: Fix handling of privilege level checks in perf interrupt context Date: Tue, 23 Feb 2021 11:36:18 +0100 [thread overview] Message-ID: <CAFqZXNuRSC1fnMduNC=mt2-H8gJsoXbm-fgQtNHg2kHvD__-xg@mail.gmail.com> (raw) In-Reply-To: <1614061909-1734-1-git-send-email-atrajeev@linux.vnet.ibm.com> (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-atrajeev@linux.vnet.ibm.com/T/) On Tue, Feb 23, 2021 at 7:32 AM Athira Rajeev <atrajeev@linux.vnet.ibm.com> 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: c000000000dff3d4 LR: c000000000dff3d0 CTR: 0000000000000000 > REGS: c000007fffab7d60 TRAP: 0100 Not tainted (5.11.0-rc7+) > <<>> > NIP [c000000000dff3d4] _raw_spin_lock_irqsave+0x94/0x120 > LR [c000000000dff3d0] _raw_spin_lock_irqsave+0x90/0x120 > Call Trace: > [c00000000fd471a0] [c00000000fd47260] 0xc00000000fd47260 (unreliable) > [c00000000fd471e0] [c000000000b5fbbc] skb_queue_tail+0x3c/0x90 > [c00000000fd47220] [c000000000296edc] audit_log_end+0x6c/0x180 > [c00000000fd47260] [c0000000006a3f20] common_lsm_audit+0xb0/0xe0 > [c00000000fd472a0] [c00000000066c664] slow_avc_audit+0xa4/0x110 > [c00000000fd47320] [c00000000066cff4] avc_has_perm+0x1c4/0x260 > [c00000000fd47430] [c00000000066e064] selinux_perf_event_open+0x74/0xd0 > [c00000000fd47450] [c000000000669888] security_perf_event_open+0x68/0xc0 > [c00000000fd47490] [c00000000013d788] record_and_restart+0x6e8/0x7f0 > [c00000000fd476c0] [c00000000013dabc] perf_event_interrupt+0x22c/0x560 > [c00000000fd477d0] [c00000000002d0fc] performance_monitor_exception+0x4c/0x60 > [c00000000fd477f0] [c00000000000b378] performance_monitor_common_virt+0x1c8/0x1d0 > interrupt: f00 at _raw_spin_lock_irqsave+0x38/0x120 > NIP: c000000000dff378 LR: c000000000b5fbbc CTR: c0000000007d47f0 > REGS: c00000000fd47860 TRAP: 0f00 Not tainted (5.11.0-rc7+) > <<>> > NIP [c000000000dff378] _raw_spin_lock_irqsave+0x38/0x120 > LR [c000000000b5fbbc] skb_queue_tail+0x3c/0x90 > interrupt: f00 > [c00000000fd47b00] [0000000000000038] 0x38 (unreliable) > [c00000000fd47b40] [c00000000aae6200] 0xc00000000aae6200 > [c00000000fd47b80] [c000000000296edc] audit_log_end+0x6c/0x180 > [c00000000fd47bc0] [c00000000029f494] audit_log_exit+0x344/0xf80 > [c00000000fd47d10] [c0000000002a2b00] __audit_syscall_exit+0x2c0/0x320 > [c00000000fd47d60] [c000000000032878] do_syscall_trace_leave+0x148/0x200 > [c00000000fd47da0] [c00000000003d5b4] syscall_exit_prepare+0x324/0x390 > [c00000000fd47e10] [c00000000000d76c] 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 <mpe@ellerman.id.au> > Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> > --- > 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)) && perf_allow_kernel(&event->attr) != 0) > + if (is_kernel_addr(mfspr(SPRN_SDAR)) && !event_allow_kernel(event)) > *addrp = 0; > } > > @@ -507,7 +512,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(&event->attr) != 0) > + is_kernel_addr(addr) && !event_allow_kernel(event)) > continue; > > /* Branches are read most recent first (ie. mfbhrb 0 is > @@ -2049,6 +2054,13 @@ static int power_pmu_event_init(struct perf_event *event) > if (err) > return -EINVAL; > > + /* > + * We (ab)use pmu_private to cache the result of perf_allow_kernel(). We > + * need access to that result at interrupt time, but can't call > + * perf_allow_kernel() directly from interrupt context. > + */ > + event->pmu_private = (void *)(long)(perf_allow_kernel(&event->attr) == 0); I don't think you need this. Unless I'm missing something, you can simply use "event->attr.exclude_kernel" in place of "!event_allow_kernel(event)". If it is set, then there must have been a successful perf_allow_kernel() check in perf_event_open(2) before the event was created. power_pmu_event_init() would be called shortly after via perf_event_alloc() -> perf_init_event(), so I don't think this additional check would add much value. > + > event->hw.config = events[n]; > event->hw.event_base = cflags[n]; > event->hw.last_period = event->hw.sample_period; > -- > 1.8.3.1 > -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
WARNING: multiple messages have this Message-ID (diff)
From: Ondrej Mosnacek <omosnace@redhat.com> To: atrajeev@linux.vnet.ibm.com Cc: maddy@linux.ibm.com, Peter Zijlstra <peterz@infradead.org>, Arnaldo Carvalho de Melo <acme@kernel.org>, SElinux list <selinux@vger.kernel.org>, Linux Security Module list <linux-security-module@vger.kernel.org>, Jiri Olsa <jolsa@kernel.org>, linuxppc-dev@lists.ozlabs.org, kan.liang@linux.intel.com Subject: Re: [PATCH] powerpc/perf: Fix handling of privilege level checks in perf interrupt context Date: Tue, 23 Feb 2021 11:36:18 +0100 [thread overview] Message-ID: <CAFqZXNuRSC1fnMduNC=mt2-H8gJsoXbm-fgQtNHg2kHvD__-xg@mail.gmail.com> (raw) In-Reply-To: <1614061909-1734-1-git-send-email-atrajeev@linux.vnet.ibm.com> (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-atrajeev@linux.vnet.ibm.com/T/) On Tue, Feb 23, 2021 at 7:32 AM Athira Rajeev <atrajeev@linux.vnet.ibm.com> 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: c000000000dff3d4 LR: c000000000dff3d0 CTR: 0000000000000000 > REGS: c000007fffab7d60 TRAP: 0100 Not tainted (5.11.0-rc7+) > <<>> > NIP [c000000000dff3d4] _raw_spin_lock_irqsave+0x94/0x120 > LR [c000000000dff3d0] _raw_spin_lock_irqsave+0x90/0x120 > Call Trace: > [c00000000fd471a0] [c00000000fd47260] 0xc00000000fd47260 (unreliable) > [c00000000fd471e0] [c000000000b5fbbc] skb_queue_tail+0x3c/0x90 > [c00000000fd47220] [c000000000296edc] audit_log_end+0x6c/0x180 > [c00000000fd47260] [c0000000006a3f20] common_lsm_audit+0xb0/0xe0 > [c00000000fd472a0] [c00000000066c664] slow_avc_audit+0xa4/0x110 > [c00000000fd47320] [c00000000066cff4] avc_has_perm+0x1c4/0x260 > [c00000000fd47430] [c00000000066e064] selinux_perf_event_open+0x74/0xd0 > [c00000000fd47450] [c000000000669888] security_perf_event_open+0x68/0xc0 > [c00000000fd47490] [c00000000013d788] record_and_restart+0x6e8/0x7f0 > [c00000000fd476c0] [c00000000013dabc] perf_event_interrupt+0x22c/0x560 > [c00000000fd477d0] [c00000000002d0fc] performance_monitor_exception+0x4c/0x60 > [c00000000fd477f0] [c00000000000b378] performance_monitor_common_virt+0x1c8/0x1d0 > interrupt: f00 at _raw_spin_lock_irqsave+0x38/0x120 > NIP: c000000000dff378 LR: c000000000b5fbbc CTR: c0000000007d47f0 > REGS: c00000000fd47860 TRAP: 0f00 Not tainted (5.11.0-rc7+) > <<>> > NIP [c000000000dff378] _raw_spin_lock_irqsave+0x38/0x120 > LR [c000000000b5fbbc] skb_queue_tail+0x3c/0x90 > interrupt: f00 > [c00000000fd47b00] [0000000000000038] 0x38 (unreliable) > [c00000000fd47b40] [c00000000aae6200] 0xc00000000aae6200 > [c00000000fd47b80] [c000000000296edc] audit_log_end+0x6c/0x180 > [c00000000fd47bc0] [c00000000029f494] audit_log_exit+0x344/0xf80 > [c00000000fd47d10] [c0000000002a2b00] __audit_syscall_exit+0x2c0/0x320 > [c00000000fd47d60] [c000000000032878] do_syscall_trace_leave+0x148/0x200 > [c00000000fd47da0] [c00000000003d5b4] syscall_exit_prepare+0x324/0x390 > [c00000000fd47e10] [c00000000000d76c] 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 <mpe@ellerman.id.au> > Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> > --- > 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)) && perf_allow_kernel(&event->attr) != 0) > + if (is_kernel_addr(mfspr(SPRN_SDAR)) && !event_allow_kernel(event)) > *addrp = 0; > } > > @@ -507,7 +512,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(&event->attr) != 0) > + is_kernel_addr(addr) && !event_allow_kernel(event)) > continue; > > /* Branches are read most recent first (ie. mfbhrb 0 is > @@ -2049,6 +2054,13 @@ static int power_pmu_event_init(struct perf_event *event) > if (err) > return -EINVAL; > > + /* > + * We (ab)use pmu_private to cache the result of perf_allow_kernel(). We > + * need access to that result at interrupt time, but can't call > + * perf_allow_kernel() directly from interrupt context. > + */ > + event->pmu_private = (void *)(long)(perf_allow_kernel(&event->attr) == 0); I don't think you need this. Unless I'm missing something, you can simply use "event->attr.exclude_kernel" in place of "!event_allow_kernel(event)". If it is set, then there must have been a successful perf_allow_kernel() check in perf_event_open(2) before the event was created. power_pmu_event_init() would be called shortly after via perf_event_alloc() -> perf_init_event(), so I don't think this additional check would add much value. > + > event->hw.config = events[n]; > event->hw.event_base = cflags[n]; > event->hw.last_period = event->hw.sample_period; > -- > 1.8.3.1 > -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
next prev parent reply other threads:[~2021-02-23 10:38 UTC|newest] Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-02-23 6:31 [PATCH] powerpc/perf: Fix handling of privilege level checks in perf interrupt context Athira Rajeev 2021-02-23 10:36 ` Ondrej Mosnacek [this message] 2021-02-23 10:36 ` Ondrej Mosnacek 2021-02-23 11:03 ` Peter Zijlstra 2021-02-23 12:54 ` Michael Ellerman 2021-02-24 11:10 ` Athira Rajeev
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAFqZXNuRSC1fnMduNC=mt2-H8gJsoXbm-fgQtNHg2kHvD__-xg@mail.gmail.com' \ --to=omosnace@redhat.com \ --cc=acme@kernel.org \ --cc=atrajeev@linux.vnet.ibm.com \ --cc=jolsa@kernel.org \ --cc=kan.liang@linux.intel.com \ --cc=linux-security-module@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=maddy@linux.ibm.com \ --cc=mpe@ellerman.id.au \ --cc=peterz@infradead.org \ --cc=selinux@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.