linux-kernel - Re: [PATCH] perf/core: Add macros for possible sysctl_perf_event

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 8 Jul 2022 10:10:15 +0100
From:   James Clark <james.clark@....com>
To:     Anshuman Khandual <anshuman.khandual@....com>,
        linux-kernel@...r.kernel.org
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        linux-perf-users@...r.kernel.org
Subject: Re: [PATCH] perf/core: Add macros for possible
 sysctl_perf_event_paranoid values



On 01/07/2022 07:39, Anshuman Khandual wrote:
> sysctl_perf_event_paranoid can have values from [-1, 0, 1, 2] which decides
> on perf event restrictions for unprivileged users. But using them directly
> makes it difficult to correlate exact restriction level they might impose.
> This just adds macros for those numerical restriction values, making them
> clear and improving readability.
> 
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Arnaldo Carvalho de Melo <acme@...nel.org>
> Cc: Mark Rutland <mark.rutland@....com>
> Cc: linux-perf-users@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
> ---
>  include/linux/perf_event.h | 22 ++++++++++++++++++----
>  kernel/events/core.c       |  9 +--------
>  kernel/kallsyms.c          |  3 ++-
>  3 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index da759560eec5..78156b9154df 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -1359,14 +1359,28 @@ int perf_event_max_stack_handler(struct ctl_table *table, int write,
>  #define PERF_SECURITY_KERNEL		2
>  #define PERF_SECURITY_TRACEPOINT	3
>  
> +/*
> + * perf event paranoia level:
> + *  -1 - not paranoid at all
> + *   0 - disallow raw tracepoint access for unpriv
> + *   1 - disallow cpu events for unpriv
> + *   2 - disallow kernel profiling for unpriv
> + */
> +enum {
> +	PERF_EVENT_DISALLOW_NONE	= -1,
> +	PERF_EVENT_DISALLOW_TRACE,
> +	PERF_EVENT_DISALLOW_CPU,
> +	PERF_EVENT_DISALLOW_KERNEL
> +};
> +
>  static inline int perf_is_paranoid(void)
>  {
> -	return sysctl_perf_event_paranoid > -1;
> +	return sysctl_perf_event_paranoid > PERF_EVENT_DISALLOW_NONE;
>  }
>  

Hi Anshuman,

There are quite a few other instances of integers left in the tools code.
If you search for perf_event_paranoid_check() and perf_event_paranoid()
you will find them.

I'm also wondering if it makes sense to return your new enum from all of
the helper functions instead of an int and make it explicit that it's
an instance of this new type? Although the compiler doesn't seem to warn
about using integers so maybe it's not worth doing this.

James

>  static inline int perf_allow_kernel(struct perf_event_attr *attr)
>  {
> -	if (sysctl_perf_event_paranoid > 1 && !perfmon_capable())
> +	if (sysctl_perf_event_paranoid >= PERF_EVENT_DISALLOW_KERNEL && !perfmon_capable())
>  		return -EACCES;
>  
>  	return security_perf_event_open(attr, PERF_SECURITY_KERNEL);
> @@ -1374,7 +1388,7 @@ static inline int perf_allow_kernel(struct perf_event_attr *attr)
>  
>  static inline int perf_allow_cpu(struct perf_event_attr *attr)
>  {
> -	if (sysctl_perf_event_paranoid > 0 && !perfmon_capable())
> +	if (sysctl_perf_event_paranoid >= PERF_EVENT_DISALLOW_CPU && !perfmon_capable())
>  		return -EACCES;
>  
>  	return security_perf_event_open(attr, PERF_SECURITY_CPU);
> @@ -1382,7 +1396,7 @@ static inline int perf_allow_cpu(struct perf_event_attr *attr)
>  
>  static inline int perf_allow_tracepoint(struct perf_event_attr *attr)
>  {
> -	if (sysctl_perf_event_paranoid > -1 && !perfmon_capable())
> +	if (sysctl_perf_event_paranoid >= PERF_EVENT_DISALLOW_TRACE && !perfmon_capable())
>  		return -EPERM;
>  
>  	return security_perf_event_open(attr, PERF_SECURITY_TRACEPOINT);
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 80782cddb1da..6fdfdc731bab 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -408,14 +408,7 @@ static struct srcu_struct pmus_srcu;
>  static cpumask_var_t perf_online_mask;
>  static struct kmem_cache *perf_event_cache;
>  
> -/*
> - * perf event paranoia level:
> - *  -1 - not paranoid at all
> - *   0 - disallow raw tracepoint access for unpriv
> - *   1 - disallow cpu events for unpriv
> - *   2 - disallow kernel profiling for unpriv
> - */
> -int sysctl_perf_event_paranoid __read_mostly = 2;
> +int sysctl_perf_event_paranoid __read_mostly = PERF_EVENT_DISALLOW_KERNEL;
>  
>  /* Minimum for 512 kiB + 1 user control page */
>  int sysctl_perf_event_mlock __read_mostly = 512 + (PAGE_SIZE / 1024); /* 'free' kiB per user */
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index fbdf8d3279ac..705f7d7d81dc 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -18,6 +18,7 @@
>  #include <linux/fs.h>
>  #include <linux/kdb.h>
>  #include <linux/err.h>
> +#include <linux/perf_event.h>
>  #include <linux/proc_fs.h>
>  #include <linux/sched.h>	/* for cond_resched */
>  #include <linux/ctype.h>
> @@ -803,7 +804,7 @@ static inline int kallsyms_for_perf(void)
>  {
>  #ifdef CONFIG_PERF_EVENTS
>  	extern int sysctl_perf_event_paranoid;
> -	if (sysctl_perf_event_paranoid <= 1)
> +	if (sysctl_perf_event_paranoid <= PERF_EVENT_DISALLOW_CPU)
>  		return 1;
>  #endif
>  	return 0;