Re: [PATCH 0/5] perf: Treat perf_event_paranoid and kptr_restrict like the kernel does it

[Mathieu Poirier]

On Mon, 26 Aug 2019 at 19:40, Igor Lubashev <ilubashe@xxxxxxxxxx> wrote:
>
> This is a follow up series to the ensure perf treats perf_event_paranoid and
> kptr_restrict in a way that is similar to the kernel's.  That includes use of
> capabilities instead of euid==0, when possible, as well as adjusting the logic
> and fixing bugs.
>
> Prior discussion: https://lkml.kernel.org/lkml/cover.1565188228.git.ilubashe@xxxxxxxxxx
>
> ===  Testing notes ===
>
> I have tested on x86 with perf binary installed according to
> Documentation/admin-guide/perf-security.rst (cap_sys_admin, cap_sys_ptrace,
> cap_syslog assigned to the perf executable).
>
> I tested each permutation of:
>
>   * 7 commits:
>       1. HEAD of perf/core
>       2. patch 01 on top of perf/core
>       3. patches 01-02 on top of perf/core
>       4. patches 01-03 on top of perf/core
>       5. patches 01-04 on top of perf/core
>       6. patches 01-05 on top of perf/core
>       7. HEAD of perf/cap (with known bug fixed by patch 01 of this series)
>
>   * 2 build environments: with and without libcap-dev
>
>   * 3 kernel.kptr_restrict values: 0, 1, 2
>
>   * 4 kernel.perf_event_paranoid values: -1, 0, 1, 2
>
>   * 2 users: root and non-root
>
> Total: 336 permutations
>
> Each permutation consisted of:
>   perf test
>   perf record -e instructions -- sleep 1
>
> All test runs were expected.  Also, as expected, the following permutation (just
> that permutation) resulted in segmentation failure:
>  commit:                     perf/cap
>  build:                      no libcap-dev
>  kernel.kptr_restrict:       0
>  kernel.perf_event_paranoid: 2
>  user:                       non-root
>
> The perf/cap commit was included in the test to ensure that we can reproduce the
> crash and hence test that the patch series fixes the crash, while retaining the
> desired behavior of perf/cap.
>
> === Series Contents ===
>
>   01: perf event: Check ref_reloc_sym before using it
>     Fix the pre-existing cause of the crash above: use of ref_reloc_sym without
>     a check for NULL
>
>   02: perf tools: Use CAP_SYS_ADMIN with perf_event_paranoid checks
>     Replace the use of euid==0 with a check for CAP_SYS_ADMIN whenever
>     perf_event_paranoid level is verified.
>     * This patch has been reviewed previously and is unchanged.
>     * I kept Acks and Sign-offs.
>
>   03: perf util: kernel profiling is disallowed only when perf_event_paranoid>1
>     Align perf logic regarding perf_event_paranoid to match kernel's.
>     This has been reported by Arnaldo.
>
>   04: perf symbols: Use CAP_SYSLOG with kptr_restrict checks
>     Replace the use of uid and euid with a check for CAP_SYSLOG when
>     kptr_restrict is verified (similar to kernel/kallsyms.c and lib/vsprintf.c).
>     Consult perf_event_paranoid when kptr_restrict==0 (see kernel/kallsyms.c).
>     * A previous version of this patch has been reviewed previously, but I
>     * modified it in a non-trivial way, so I removed Acks.
>
>   05: perf: warn perf_event_paranoid restrict kernel symbols
>     Warn that /proc/sys/kernel/perf_event_paranoid can also restrict kernel
>     symbols.
>
> Igor Lubashev (5):
>   perf event: Check ref_reloc_sym before using it
>   perf tools: Use CAP_SYS_ADMIN with perf_event_paranoid checks
>   perf util: kernel profiling is disallowed only when perf_event_paranoid > 1
>   perf symbols: Use CAP_SYSLOG with kptr_restrict checks
>   perf: warn that perf_event_paranoid can restrict kernel symbols
>
>  tools/perf/arch/arm/util/cs-etm.c    |  3 ++-

For the coresight part:

Reviewed-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>

>  tools/perf/arch/arm64/util/arm-spe.c |  3 ++-
>  tools/perf/arch/x86/util/intel-bts.c |  3 ++-
>  tools/perf/arch/x86/util/intel-pt.c  |  2 +-
>  tools/perf/builtin-record.c          |  2 +-
>  tools/perf/builtin-top.c             |  2 +-
>  tools/perf/builtin-trace.c           |  2 +-
>  tools/perf/util/event.c              |  7 ++++---
>  tools/perf/util/evsel.c              |  2 +-
>  tools/perf/util/symbol.c             | 15 ++++++++++++---
>  10 files changed, 27 insertions(+), 14 deletions(-)

For the set:

Tested-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>

>
> --
> 2.7.4
>