[PATCH v2 1/2] selinux: add tracepoint on denials

[PATCH v2 1/2] selinux: add tracepoint on denials

[Thiébaud Weksteen]

The audit data currently captures which process and which target
is responsible for a denial. There is no data on where exactly in the
process that call occurred. Debugging can be made easier by being able to
reconstruct the unified kernel and userland stack traces [1]. Add a
tracepoint on the SELinux denials which can then be used by userland
(i.e. perf).

Although this patch could manually be added by each OS developer to
trouble shoot a denial, adding it to the kernel streamlines the
developers workflow.

It is possible to use perf for monitoring the event:
  # perf record -e avc:selinux_audited -g -a
  ^C
  # perf report -g
  [...]
      6.40%     6.40%  audited=800000 tclass=4
               |
                  __libc_start_main
                  |
                  |--4.60%--__GI___ioctl
                  |          entry_SYSCALL_64
                  |          do_syscall_64
                  |          __x64_sys_ioctl
                  |          ksys_ioctl
                  |          binder_ioctl
                  |          binder_set_nice
                  |          can_nice
                  |          capable
                  |          security_capable
                  |          cred_has_capability.isra.0
                  |          slow_avc_audit
                  |          common_lsm_audit
                  |          avc_audit_post_callback
                  |          avc_audit_post_callback
                  |

It is also possible to use the ftrace interface:
  # echo 1 > /sys/kernel/debug/tracing/events/avc/selinux_audited/enable
  # cat /sys/kernel/debug/tracing/trace
  tracer: nop
  entries-in-buffer/entries-written: 1/1   #P:8
  [...]
  dmesg-3624  [001] 13072.325358: selinux_denied: audited=800000 tclass=4

[1] https://source.android.com/devices/tech/debug/native_stack_dump

Signed-off-by: Thiébaud Weksteen <tweek@google.com>
Suggested-by: Joel Fernandes <joelaf@google.com>
Reviewed-by: Peter Enderborg <peter.enderborg@sony.com>
---
v2 changes:
- update changelog to include usage examples

 MAINTAINERS                |  1 +
 include/trace/events/avc.h | 37 +++++++++++++++++++++++++++++++++++++
 security/selinux/avc.c     |  5 +++++
 3 files changed, 43 insertions(+)
 create mode 100644 include/trace/events/avc.h

diff --git a/MAINTAINERS b/MAINTAINERS
index c8e8232c65da..0efaea0e144c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15426,6 +15426,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
 F:	Documentation/ABI/obsolete/sysfs-selinux-checkreqprot
 F:	Documentation/ABI/obsolete/sysfs-selinux-disable
 F:	Documentation/admin-guide/LSM/SELinux.rst
+F:	include/trace/events/avc.h
 F:	include/uapi/linux/selinux_netlink.h
 F:	scripts/selinux/
 F:	security/selinux/
diff --git a/include/trace/events/avc.h b/include/trace/events/avc.h
new file mode 100644
index 000000000000..07c058a9bbcd
--- /dev/null
+++ b/include/trace/events/avc.h
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Author: Thiébaud Weksteen <tweek@google.com>
+ */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM avc
+
+#if !defined(_TRACE_SELINUX_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_SELINUX_H
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(selinux_audited,
+
+	TP_PROTO(struct selinux_audit_data *sad),
+
+	TP_ARGS(sad),
+
+	TP_STRUCT__entry(
+		__field(unsigned int, tclass)
+		__field(unsigned int, audited)
+	),
+
+	TP_fast_assign(
+		__entry->tclass = sad->tclass;
+		__entry->audited = sad->audited;
+	),
+
+	TP_printk("tclass=%u audited=%x",
+		__entry->tclass,
+		__entry->audited)
+);
+
+#endif
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index d18cb32a242a..b0a0af778b70 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -31,6 +31,9 @@
 #include "avc_ss.h"
 #include "classmap.h"
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/avc.h>
+
 #define AVC_CACHE_SLOTS			512
 #define AVC_DEF_CACHE_THRESHOLD		512
 #define AVC_CACHE_RECLAIM		16
@@ -706,6 +709,8 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
 	u32 scontext_len;
 	int rc;
 
+	trace_selinux_audited(sad);
+
 	rc = security_sid_to_context(sad->state, sad->ssid, &scontext,
 				     &scontext_len);
 	if (rc)
-- 
2.28.0.236.gb10cc79966-goog

[PATCH v2 2/2] selinux: add basic filtering for audit trace events

[Thiébaud Weksteen]

From: Peter Enderborg <peter.enderborg@sony.com>

This patch adds further attributes to the event. These attributes are
helpful to understand the context of the message and can be used
to filter the events.

There are three common items. Source context, target context and tclass.
There are also items from the outcome of operation performed.

An event is similar to:
           <...>-1309  [002] ....  6346.691689: selinux_audited:
       requested=0x4000000 denied=0x4000000 audited=0x4000000
       result=-13 ssid=315 tsid=61
       scontext=system_u:system_r:cupsd_t:s0-s0:c0.c1023
       tcontext=system_u:object_r:bin_t:s0 tclass=file

With systems where many denials are occurring, it is useful to apply a
filter. The filtering is a set of logic that is inserted with
the filter file. Example:
 echo "tclass==\"file\" && ssid!=42" > events/avc/selinux_audited/filter

This adds that we only get tclass=file but not for ssid 42.

The trace can also have extra properties. Adding the user stack
can be done with
   echo 1 > options/userstacktrace

Now the output will be
         runcon-1365  [003] ....  6960.955530: selinux_audited:
     requested=0x4000000 denied=0x4000000 audited=0x4000000
     result=-13 ssid=315 tsid=61
     scontext=system_u:system_r:cupsd_t:s0-s0:c0.c1023
     tcontext=system_u:object_r:bin_t:s0 tclass=file
          runcon-1365  [003] ....  6960.955560: <user stack trace>
 =>  <00007f325b4ce45b>
 =>  <00005607093efa57>

Note that the ssid is the internal numeric representation of scontext
and tsid is numeric for tcontext. They are useful for filtering.

Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
Reviewed-by: Thiébaud Weksteen <tweek@google.com>
---
v2 changes:
- update changelog to include usage examples

 include/trace/events/avc.h | 41 ++++++++++++++++++++++++++++----------
 security/selinux/avc.c     | 22 +++++++++++---------
 2 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/include/trace/events/avc.h b/include/trace/events/avc.h
index 07c058a9bbcd..ac5ef2e1c2c5 100644
--- a/include/trace/events/avc.h
+++ b/include/trace/events/avc.h
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /*
- * Author: Thiébaud Weksteen <tweek@google.com>
+ * Authors:	Thiébaud Weksteen <tweek@google.com>
+ *		Peter Enderborg <Peter.Enderborg@sony.com>
  */
 #undef TRACE_SYSTEM
 #define TRACE_SYSTEM avc
@@ -12,23 +13,43 @@
 
 TRACE_EVENT(selinux_audited,
 
-	TP_PROTO(struct selinux_audit_data *sad),
+	TP_PROTO(struct selinux_audit_data *sad,
+		char *scontext,
+		char *tcontext,
+		const char *tclass
+	),
 
-	TP_ARGS(sad),
+	TP_ARGS(sad, scontext, tcontext, tclass),
 
 	TP_STRUCT__entry(
-		__field(unsigned int, tclass)
-		__field(unsigned int, audited)
+		__field(u32, requested)
+		__field(u32, denied)
+		__field(u32, audited)
+		__field(int, result)
+		__string(scontext, scontext)
+		__string(tcontext, tcontext)
+		__string(tclass, tclass)
+		__field(u32, ssid)
+		__field(u32, tsid)
 	),
 
 	TP_fast_assign(
-		__entry->tclass = sad->tclass;
-		__entry->audited = sad->audited;
+		__entry->requested	= sad->requested;
+		__entry->denied		= sad->denied;
+		__entry->audited	= sad->audited;
+		__entry->result		= sad->result;
+		__entry->ssid		= sad->ssid;
+		__entry->tsid		= sad->tsid;
+		__assign_str(tcontext, tcontext);
+		__assign_str(scontext, scontext);
+		__assign_str(tclass, tclass);
 	),
 
-	TP_printk("tclass=%u audited=%x",
-		__entry->tclass,
-		__entry->audited)
+	TP_printk("requested=0x%x denied=0x%x audited=0x%x result=%d ssid=%u tsid=%u scontext=%s tcontext=%s tclass=%s",
+		__entry->requested, __entry->denied, __entry->audited, __entry->result,
+		__entry->ssid, __entry->tsid, __get_str(scontext), __get_str(tcontext),
+		__get_str(tclass)
+	)
 );
 
 #endif
diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index b0a0af778b70..7de5cc5169af 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -705,35 +705,39 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
 {
 	struct common_audit_data *ad = a;
 	struct selinux_audit_data *sad = ad->selinux_audit_data;
-	char *scontext;
+	char *scontext = NULL;
+	char *tcontext = NULL;
+	const char *tclass = NULL;
 	u32 scontext_len;
+	u32 tcontext_len;
 	int rc;
 
-	trace_selinux_audited(sad);
-
 	rc = security_sid_to_context(sad->state, sad->ssid, &scontext,
 				     &scontext_len);
 	if (rc)
 		audit_log_format(ab, " ssid=%d", sad->ssid);
 	else {
 		audit_log_format(ab, " scontext=%s", scontext);
-		kfree(scontext);
 	}
 
-	rc = security_sid_to_context(sad->state, sad->tsid, &scontext,
-				     &scontext_len);
+	rc = security_sid_to_context(sad->state, sad->tsid, &tcontext,
+				     &tcontext_len);
 	if (rc)
 		audit_log_format(ab, " tsid=%d", sad->tsid);
 	else {
-		audit_log_format(ab, " tcontext=%s", scontext);
-		kfree(scontext);
+		audit_log_format(ab, " tcontext=%s", tcontext);
 	}
 
-	audit_log_format(ab, " tclass=%s", secclass_map[sad->tclass-1].name);
+	tclass = secclass_map[sad->tclass-1].name;
+	audit_log_format(ab, " tclass=%s", tclass);
 
 	if (sad->denied)
 		audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1);
 
+	trace_selinux_audited(sad, scontext, tcontext, tclass);
+	kfree(tcontext);
+	kfree(scontext);
+
 	/* in case of invalid context report also the actual context string */
 	rc = security_sid_to_context_inval(sad->state, sad->ssid, &scontext,
 					   &scontext_len);
-- 
2.28.0.236.gb10cc79966-goog

Re: [PATCH v2 2/2] selinux: add basic filtering for audit trace events

[Casey Schaufler]

On 8/13/2020 7:48 AM, Thiébaud Weksteen wrote:
> From: Peter Enderborg <peter.enderborg@sony.com>
>
> This patch adds further attributes to the event. These attributes are
> helpful to understand the context of the message and can be used
> to filter the events.
>
> There are three common items. Source context, target context and tclass.
> There are also items from the outcome of operation performed.
>
> An event is similar to:
>            <...>-1309  [002] ....  6346.691689: selinux_audited:
>        requested=0x4000000 denied=0x4000000 audited=0x4000000
>        result=-13 ssid=315 tsid=61

It may not be my place to ask, but *please please please* don't
externalize secids. I understand that it's easier to type "42"
than "system_r:cupsd_t:s0-s0:c0.c1023", and that it's easier for
your tools to parse and store the number. Once you start training
people that system_r:cupsd_t:s0-s0:c0.c1023 is secid 42 you'll
never be able to change it. The secid will start showing up in
scripts. Bad  Things  Will  Happen.

>        scontext=system_u:system_r:cupsd_t:s0-s0:c0.c1023
>        tcontext=system_u:object_r:bin_t:s0 tclass=file
>
> With systems where many denials are occurring, it is useful to apply a
> filter. The filtering is a set of logic that is inserted with
> the filter file. Example:
>  echo "tclass==\"file\" && ssid!=42" > events/avc/selinux_audited/filter
>
> This adds that we only get tclass=file but not for ssid 42.
>
> The trace can also have extra properties. Adding the user stack
> can be done with
>    echo 1 > options/userstacktrace
>
> Now the output will be
>          runcon-1365  [003] ....  6960.955530: selinux_audited:
>      requested=0x4000000 denied=0x4000000 audited=0x4000000
>      result=-13 ssid=315 tsid=61
>      scontext=system_u:system_r:cupsd_t:s0-s0:c0.c1023
>      tcontext=system_u:object_r:bin_t:s0 tclass=file
>           runcon-1365  [003] ....  6960.955560: <user stack trace>
>  =>  <00007f325b4ce45b>
>  =>  <00005607093efa57>
>
> Note that the ssid is the internal numeric representation of scontext
> and tsid is numeric for tcontext. They are useful for filtering.
>
> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
> Reviewed-by: Thiébaud Weksteen <tweek@google.com>
> ---
> v2 changes:
> - update changelog to include usage examples
>
>  include/trace/events/avc.h | 41 ++++++++++++++++++++++++++++----------
>  security/selinux/avc.c     | 22 +++++++++++---------
>  2 files changed, 44 insertions(+), 19 deletions(-)
>
> diff --git a/include/trace/events/avc.h b/include/trace/events/avc.h
> index 07c058a9bbcd..ac5ef2e1c2c5 100644
> --- a/include/trace/events/avc.h
> +++ b/include/trace/events/avc.h
> @@ -1,6 +1,7 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  /*
> - * Author: Thiébaud Weksteen <tweek@google.com>
> + * Authors:	Thiébaud Weksteen <tweek@google.com>
> + *		Peter Enderborg <Peter.Enderborg@sony.com>
>   */
>  #undef TRACE_SYSTEM
>  #define TRACE_SYSTEM avc
> @@ -12,23 +13,43 @@
>  
>  TRACE_EVENT(selinux_audited,
>  
> -	TP_PROTO(struct selinux_audit_data *sad),
> +	TP_PROTO(struct selinux_audit_data *sad,
> +		char *scontext,
> +		char *tcontext,
> +		const char *tclass
> +	),
>  
> -	TP_ARGS(sad),
> +	TP_ARGS(sad, scontext, tcontext, tclass),
>  
>  	TP_STRUCT__entry(
> -		__field(unsigned int, tclass)
> -		__field(unsigned int, audited)
> +		__field(u32, requested)
> +		__field(u32, denied)
> +		__field(u32, audited)
> +		__field(int, result)
> +		__string(scontext, scontext)
> +		__string(tcontext, tcontext)
> +		__string(tclass, tclass)
> +		__field(u32, ssid)
> +		__field(u32, tsid)
>  	),
>  
>  	TP_fast_assign(
> -		__entry->tclass = sad->tclass;
> -		__entry->audited = sad->audited;
> +		__entry->requested	= sad->requested;
> +		__entry->denied		= sad->denied;
> +		__entry->audited	= sad->audited;
> +		__entry->result		= sad->result;
> +		__entry->ssid		= sad->ssid;
> +		__entry->tsid		= sad->tsid;
> +		__assign_str(tcontext, tcontext);
> +		__assign_str(scontext, scontext);
> +		__assign_str(tclass, tclass);
>  	),
>  
> -	TP_printk("tclass=%u audited=%x",
> -		__entry->tclass,
> -		__entry->audited)
> +	TP_printk("requested=0x%x denied=0x%x audited=0x%x result=%d ssid=%u tsid=%u scontext=%s tcontext=%s tclass=%s",
> +		__entry->requested, __entry->denied, __entry->audited, __entry->result,
> +		__entry->ssid, __entry->tsid, __get_str(scontext), __get_str(tcontext),
> +		__get_str(tclass)
> +	)
>  );
>  
>  #endif
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index b0a0af778b70..7de5cc5169af 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -705,35 +705,39 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
>  {
>  	struct common_audit_data *ad = a;
>  	struct selinux_audit_data *sad = ad->selinux_audit_data;
> -	char *scontext;
> +	char *scontext = NULL;
> +	char *tcontext = NULL;
> +	const char *tclass = NULL;
>  	u32 scontext_len;
> +	u32 tcontext_len;
>  	int rc;
>  
> -	trace_selinux_audited(sad);
> -
>  	rc = security_sid_to_context(sad->state, sad->ssid, &scontext,
>  				     &scontext_len);
>  	if (rc)
>  		audit_log_format(ab, " ssid=%d", sad->ssid);
>  	else {
>  		audit_log_format(ab, " scontext=%s", scontext);
> -		kfree(scontext);
>  	}
>  
> -	rc = security_sid_to_context(sad->state, sad->tsid, &scontext,
> -				     &scontext_len);
> +	rc = security_sid_to_context(sad->state, sad->tsid, &tcontext,
> +				     &tcontext_len);
>  	if (rc)
>  		audit_log_format(ab, " tsid=%d", sad->tsid);
>  	else {
> -		audit_log_format(ab, " tcontext=%s", scontext);
> -		kfree(scontext);
> +		audit_log_format(ab, " tcontext=%s", tcontext);
>  	}
>  
> -	audit_log_format(ab, " tclass=%s", secclass_map[sad->tclass-1].name);
> +	tclass = secclass_map[sad->tclass-1].name;
> +	audit_log_format(ab, " tclass=%s", tclass);
>  
>  	if (sad->denied)
>  		audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1);
>  
> +	trace_selinux_audited(sad, scontext, tcontext, tclass);
> +	kfree(tcontext);
> +	kfree(scontext);
> +
>  	/* in case of invalid context report also the actual context string */
>  	rc = security_sid_to_context_inval(sad->state, sad->ssid, &scontext,
>  					   &scontext_len);

Re: [PATCH v2 2/2] selinux: add basic filtering for audit trace events

[peter enderborg]

On 8/13/20 5:05 PM, Casey Schaufler wrote:
> On 8/13/2020 7:48 AM, Thiébaud Weksteen wrote:
>> From: Peter Enderborg <peter.enderborg@sony.com>
>>
>> This patch adds further attributes to the event. These attributes are
>> helpful to understand the context of the message and can be used
>> to filter the events.
>>
>> There are three common items. Source context, target context and tclass.
>> There are also items from the outcome of operation performed.
>>
>> An event is similar to:
>>            <...>-1309  [002] ....  6346.691689: selinux_audited:
>>        requested=0x4000000 denied=0x4000000 audited=0x4000000
>>        result=-13 ssid=315 tsid=61
> It may not be my place to ask, but *please please please* don't
> externalize secids. I understand that it's easier to type "42"
> than "system_r:cupsd_t:s0-s0:c0.c1023", and that it's easier for
> your tools to parse and store the number. Once you start training
> people that system_r:cupsd_t:s0-s0:c0.c1023 is secid 42 you'll
> never be able to change it. The secid will start showing up in
> scripts. Bad  Things  Will  Happen.

Ok, it seems to mostly against having this performance options.
Yes, it is a kernel internal data. So is most of the kernel tracing.
I see it is a primary tool for kernel debugging but than can also be
used for user-space debugging tools.  Hiding data for debuggers
does not make any sense too me.


>>        scontext=system_u:system_r:cupsd_t:s0-s0:c0.c1023
>>        tcontext=system_u:object_r:bin_t:s0 tclass=file
>>
>> With systems where many denials are occurring, it is useful to apply a
>> filter. The filtering is a set of logic that is inserted with
>> the filter file. Example:
>>  echo "tclass==\"file\" && ssid!=42" > events/avc/selinux_audited/filter
>>
>> This adds that we only get tclass=file but not for ssid 42.
>>
>> The trace can also have extra properties. Adding the user stack
>> can be done with
>>    echo 1 > options/userstacktrace
>>
>> Now the output will be
>>          runcon-1365  [003] ....  6960.955530: selinux_audited:
>>      requested=0x4000000 denied=0x4000000 audited=0x4000000
>>      result=-13 ssid=315 tsid=61
>>      scontext=system_u:system_r:cupsd_t:s0-s0:c0.c1023
>>      tcontext=system_u:object_r:bin_t:s0 tclass=file
>>           runcon-1365  [003] ....  6960.955560: <user stack trace>
>>  =>  <00007f325b4ce45b>
>>  =>  <00005607093efa57>
>>
>> Note that the ssid is the internal numeric representation of scontext
>> and tsid is numeric for tcontext. They are useful for filtering.
>>
>> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
>> Reviewed-by: Thiébaud Weksteen <tweek@google.com>
>> ---
>> v2 changes:
>> - update changelog to include usage examples
>>
>>  include/trace/events/avc.h | 41 ++++++++++++++++++++++++++++----------
>>  security/selinux/avc.c     | 22 +++++++++++---------
>>  2 files changed, 44 insertions(+), 19 deletions(-)
>>
>> diff --git a/include/trace/events/avc.h b/include/trace/events/avc.h
>> index 07c058a9bbcd..ac5ef2e1c2c5 100644
>> --- a/include/trace/events/avc.h
>> +++ b/include/trace/events/avc.h
>> @@ -1,6 +1,7 @@
>>  /* SPDX-License-Identifier: GPL-2.0 */
>>  /*
>> - * Author: Thiébaud Weksteen <tweek@google.com>
>> + * Authors:	Thiébaud Weksteen <tweek@google.com>
>> + *		Peter Enderborg <Peter.Enderborg@sony.com>
>>   */
>>  #undef TRACE_SYSTEM
>>  #define TRACE_SYSTEM avc
>> @@ -12,23 +13,43 @@
>>  
>>  TRACE_EVENT(selinux_audited,
>>  
>> -	TP_PROTO(struct selinux_audit_data *sad),
>> +	TP_PROTO(struct selinux_audit_data *sad,
>> +		char *scontext,
>> +		char *tcontext,
>> +		const char *tclass
>> +	),
>>  
>> -	TP_ARGS(sad),
>> +	TP_ARGS(sad, scontext, tcontext, tclass),
>>  
>>  	TP_STRUCT__entry(
>> -		__field(unsigned int, tclass)
>> -		__field(unsigned int, audited)
>> +		__field(u32, requested)
>> +		__field(u32, denied)
>> +		__field(u32, audited)
>> +		__field(int, result)
>> +		__string(scontext, scontext)
>> +		__string(tcontext, tcontext)
>> +		__string(tclass, tclass)
>> +		__field(u32, ssid)
>> +		__field(u32, tsid)
>>  	),
>>  
>>  	TP_fast_assign(
>> -		__entry->tclass = sad->tclass;
>> -		__entry->audited = sad->audited;
>> +		__entry->requested	= sad->requested;
>> +		__entry->denied		= sad->denied;
>> +		__entry->audited	= sad->audited;
>> +		__entry->result		= sad->result;
>> +		__entry->ssid		= sad->ssid;
>> +		__entry->tsid		= sad->tsid;
>> +		__assign_str(tcontext, tcontext);
>> +		__assign_str(scontext, scontext);
>> +		__assign_str(tclass, tclass);
>>  	),
>>  
>> -	TP_printk("tclass=%u audited=%x",
>> -		__entry->tclass,
>> -		__entry->audited)
>> +	TP_printk("requested=0x%x denied=0x%x audited=0x%x result=%d ssid=%u tsid=%u scontext=%s tcontext=%s tclass=%s",
>> +		__entry->requested, __entry->denied, __entry->audited, __entry->result,
>> +		__entry->ssid, __entry->tsid, __get_str(scontext), __get_str(tcontext),
>> +		__get_str(tclass)
>> +	)
>>  );
>>  
>>  #endif
>> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
>> index b0a0af778b70..7de5cc5169af 100644
>> --- a/security/selinux/avc.c
>> +++ b/security/selinux/avc.c
>> @@ -705,35 +705,39 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
>>  {
>>  	struct common_audit_data *ad = a;
>>  	struct selinux_audit_data *sad = ad->selinux_audit_data;
>> -	char *scontext;
>> +	char *scontext = NULL;
>> +	char *tcontext = NULL;
>> +	const char *tclass = NULL;
>>  	u32 scontext_len;
>> +	u32 tcontext_len;
>>  	int rc;
>>  
>> -	trace_selinux_audited(sad);
>> -
>>  	rc = security_sid_to_context(sad->state, sad->ssid, &scontext,
>>  				     &scontext_len);
>>  	if (rc)
>>  		audit_log_format(ab, " ssid=%d", sad->ssid);
>>  	else {
>>  		audit_log_format(ab, " scontext=%s", scontext);
>> -		kfree(scontext);
>>  	}
>>  
>> -	rc = security_sid_to_context(sad->state, sad->tsid, &scontext,
>> -				     &scontext_len);
>> +	rc = security_sid_to_context(sad->state, sad->tsid, &tcontext,
>> +				     &tcontext_len);
>>  	if (rc)
>>  		audit_log_format(ab, " tsid=%d", sad->tsid);
>>  	else {
>> -		audit_log_format(ab, " tcontext=%s", scontext);
>> -		kfree(scontext);
>> +		audit_log_format(ab, " tcontext=%s", tcontext);
>>  	}
>>  
>> -	audit_log_format(ab, " tclass=%s", secclass_map[sad->tclass-1].name);
>> +	tclass = secclass_map[sad->tclass-1].name;
>> +	audit_log_format(ab, " tclass=%s", tclass);
>>  
>>  	if (sad->denied)
>>  		audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1);
>>  
>> +	trace_selinux_audited(sad, scontext, tcontext, tclass);
>> +	kfree(tcontext);
>> +	kfree(scontext);
>> +
>>  	/* in case of invalid context report also the actual context string */
>>  	rc = security_sid_to_context_inval(sad->state, sad->ssid, &scontext,
>>  					   &scontext_len);

Re: [PATCH v2 1/2] selinux: add tracepoint on denials

[Stephen Smalley]

On 8/13/20 10:48 AM, Thiébaud Weksteen wrote:

> The audit data currently captures which process and which target
> is responsible for a denial. There is no data on where exactly in the
> process that call occurred. Debugging can be made easier by being able to
> reconstruct the unified kernel and userland stack traces [1]. Add a
> tracepoint on the SELinux denials which can then be used by userland
> (i.e. perf).
>
> Although this patch could manually be added by each OS developer to
> trouble shoot a denial, adding it to the kernel streamlines the
> developers workflow.
>
> It is possible to use perf for monitoring the event:
>    # perf record -e avc:selinux_audited -g -a
>    ^C
>    # perf report -g
>    [...]
>        6.40%     6.40%  audited=800000 tclass=4
>                 |
>                    __libc_start_main
>                    |
>                    |--4.60%--__GI___ioctl
>                    |          entry_SYSCALL_64
>                    |          do_syscall_64
>                    |          __x64_sys_ioctl
>                    |          ksys_ioctl
>                    |          binder_ioctl
>                    |          binder_set_nice
>                    |          can_nice
>                    |          capable
>                    |          security_capable
>                    |          cred_has_capability.isra.0
>                    |          slow_avc_audit
>                    |          common_lsm_audit
>                    |          avc_audit_post_callback
>                    |          avc_audit_post_callback
>                    |
>
> It is also possible to use the ftrace interface:
>    # echo 1 > /sys/kernel/debug/tracing/events/avc/selinux_audited/enable
>    # cat /sys/kernel/debug/tracing/trace
>    tracer: nop
>    entries-in-buffer/entries-written: 1/1   #P:8
>    [...]
>    dmesg-3624  [001] 13072.325358: selinux_denied: audited=800000 tclass=4

An explanation here of how one might go about decoding audited and 
tclass would be helpful to users (even better would be a script to do it 
for them).  Again, I know how to do that but not everyone using 
perf/ftrace will.

Re: [PATCH v2 2/2] selinux: add basic filtering for audit trace events

[Stephen Smalley]

On 8/13/20 11:35 AM, peter enderborg wrote:

> On 8/13/20 5:05 PM, Casey Schaufler wrote:
>> On 8/13/2020 7:48 AM, Thiébaud Weksteen wrote:
>>> From: Peter Enderborg <peter.enderborg@sony.com>
>>>
>>> This patch adds further attributes to the event. These attributes are
>>> helpful to understand the context of the message and can be used
>>> to filter the events.
>>>
>>> There are three common items. Source context, target context and tclass.
>>> There are also items from the outcome of operation performed.
>>>
>>> An event is similar to:
>>>             <...>-1309  [002] ....  6346.691689: selinux_audited:
>>>         requested=0x4000000 denied=0x4000000 audited=0x4000000
>>>         result=-13 ssid=315 tsid=61
>> It may not be my place to ask, but *please please please* don't
>> externalize secids. I understand that it's easier to type "42"
>> than "system_r:cupsd_t:s0-s0:c0.c1023", and that it's easier for
>> your tools to parse and store the number. Once you start training
>> people that system_r:cupsd_t:s0-s0:c0.c1023 is secid 42 you'll
>> never be able to change it. The secid will start showing up in
>> scripts. Bad  Things  Will  Happen.
> Ok, it seems to mostly against having this performance options.
> Yes, it is a kernel internal data. So is most of the kernel tracing.
> I see it is a primary tool for kernel debugging but than can also be
> used for user-space debugging tools.  Hiding data for debuggers
> does not make any sense too me.

To be clear, userspace tools can't use fixed secid values because secids 
are dynamically assigned by SELinux and thus secid 42 need not 
correspond to the same security context across different boots even with 
the same kernel and policy.  I wouldn't include them in the event unless 
it is common practice to include fields that can only be interpreted if 
you can debug the running kernel.  It would be akin to including kernel 
pointers in the event (albeit without the KASLR ramifications).

Re: [PATCH v2 2/2] selinux: add basic filtering for audit trace events

[peter enderborg]

On 8/13/20 5:49 PM, Stephen Smalley wrote:
> On 8/13/20 11:35 AM, peter enderborg wrote:
>
>> On 8/13/20 5:05 PM, Casey Schaufler wrote:
>>> On 8/13/2020 7:48 AM, Thiébaud Weksteen wrote:
>>>> From: Peter Enderborg <peter.enderborg@sony.com>
>>>>
>>>> This patch adds further attributes to the event. These attributes are
>>>> helpful to understand the context of the message and can be used
>>>> to filter the events.
>>>>
>>>> There are three common items. Source context, target context and tclass.
>>>> There are also items from the outcome of operation performed.
>>>>
>>>> An event is similar to:
>>>>             <...>-1309  [002] ....  6346.691689: selinux_audited:
>>>>         requested=0x4000000 denied=0x4000000 audited=0x4000000
>>>>         result=-13 ssid=315 tsid=61
>>> It may not be my place to ask, but *please please please* don't
>>> externalize secids. I understand that it's easier to type "42"
>>> than "system_r:cupsd_t:s0-s0:c0.c1023", and that it's easier for
>>> your tools to parse and store the number. Once you start training
>>> people that system_r:cupsd_t:s0-s0:c0.c1023 is secid 42 you'll
>>> never be able to change it. The secid will start showing up in
>>> scripts. Bad  Things  Will  Happen.
>> Ok, it seems to mostly against having this performance options.
>> Yes, it is a kernel internal data. So is most of the kernel tracing.
>> I see it is a primary tool for kernel debugging but than can also be
>> used for user-space debugging tools.  Hiding data for debuggers
>> does not make any sense too me.
>
> To be clear, userspace tools can't use fixed secid values because secids are dynamically assigned by SELinux and thus secid 42 need not correspond to the same security context across different boots even with the same kernel and policy.  I wouldn't include them in the event unless it is common practice to include fields that can only be interpreted if you can debug the running kernel.  It would be akin to including kernel pointers in the event (albeit without the KASLR ramifications).
>
Yes of course. Trace debugging is about running kernel. Would i make more sense if the was a debugfs entry with the sid's? This filter are a reminisce  of the same filter used not only to catch denials. Doing a string compare
for all syscalls keep the cpu busy.  I will do an update without it.

Re: [PATCH v2 2/2] selinux: add basic filtering for audit trace events

[peter enderborg]

On 8/13/20 5:49 PM, Stephen Smalley wrote:
> On 8/13/20 11:35 AM, peter enderborg wrote:
>
>> On 8/13/20 5:05 PM, Casey Schaufler wrote:
>>> On 8/13/2020 7:48 AM, Thiébaud Weksteen wrote:
>>>> From: Peter Enderborg <peter.enderborg@sony.com>
>>>>
>>>> This patch adds further attributes to the event. These attributes are
>>>> helpful to understand the context of the message and can be used
>>>> to filter the events.
>>>>
>>>> There are three common items. Source context, target context and tclass.
>>>> There are also items from the outcome of operation performed.
>>>>
>>>> An event is similar to:
>>>>             <...>-1309  [002] ....  6346.691689: selinux_audited:
>>>>         requested=0x4000000 denied=0x4000000 audited=0x4000000
>>>>         result=-13 ssid=315 tsid=61
>>> It may not be my place to ask, but *please please please* don't
>>> externalize secids. I understand that it's easier to type "42"
>>> than "system_r:cupsd_t:s0-s0:c0.c1023", and that it's easier for
>>> your tools to parse and store the number. Once you start training
>>> people that system_r:cupsd_t:s0-s0:c0.c1023 is secid 42 you'll
>>> never be able to change it. The secid will start showing up in
>>> scripts. Bad  Things  Will  Happen.
>> Ok, it seems to mostly against having this performance options.
>> Yes, it is a kernel internal data. So is most of the kernel tracing.
>> I see it is a primary tool for kernel debugging but than can also be
>> used for user-space debugging tools.  Hiding data for debuggers
>> does not make any sense too me.
>
> To be clear, userspace tools can't use fixed secid values because secids are dynamically assigned by SELinux and thus secid 42 need not correspond to the same security context across different boots even with the same kernel and policy.  I wouldn't include them in the event unless it is common practice to include fields that can only be interpreted if you can debug the running kernel.  It would be akin to including kernel pointers in the event (albeit without the KASLR ramifications).
>
>
Just as a reference on my fedora system; out of 1808 events 244 as a pointer print. I don't see that there is any obfuscating aka "%pK" as there is for logs.

Re: [PATCH v2 2/2] selinux: add basic filtering for audit trace events

[Steven Rostedt]

On Thu, 13 Aug 2020 19:14:10 +0200
peter enderborg <peter.enderborg@sony.com> wrote:

> > To be clear, userspace tools can't use fixed secid values because
> > secids are dynamically assigned by SELinux and thus secid 42 need
> > not correspond to the same security context across different boots
> > even with the same kernel and policy.  I wouldn't include them in
> > the event unless it is common practice to include fields that can
> > only be interpreted if you can debug the running kernel.  It would
> > be akin to including kernel pointers in the event (albeit without
> > the KASLR ramifications).
> >
> >  
> Just as a reference on my fedora system; out of 1808 events 244 as a
> pointer print. I don't see that there is any obfuscating aka "%pK" as
> there is for logs.

Which is a reason why tracefs is root only.

The "%p" gets obfuscated when printed from the trace file by default
now. But they are consistent (where the same pointer shows up as the
same hash).

It's used mainly to map together events. For example, if you print the
address of a skb in the networking events, it's good to know what
events reference the same skb, and the pointer is used for that.

-- Steve

Re: [PATCH v2 2/2] selinux: add basic filtering for audit trace events

[peter enderborg]

On 8/13/20 7:38 PM, Steven Rostedt wrote:
> On Thu, 13 Aug 2020 19:14:10 +0200
> peter enderborg <peter.enderborg@sony.com> wrote:
>
>>> To be clear, userspace tools can't use fixed secid values because
>>> secids are dynamically assigned by SELinux and thus secid 42 need
>>> not correspond to the same security context across different boots
>>> even with the same kernel and policy.  I wouldn't include them in
>>> the event unless it is common practice to include fields that can
>>> only be interpreted if you can debug the running kernel.  It would
>>> be akin to including kernel pointers in the event (albeit without
>>> the KASLR ramifications).
>>>
>>>  
>> Just as a reference on my fedora system; out of 1808 events 244 as a
>> pointer print. I don't see that there is any obfuscating aka "%pK" as
>> there is for logs.
> Which is a reason why tracefs is root only.
>
> The "%p" gets obfuscated when printed from the trace file by default
> now. But they are consistent (where the same pointer shows up as the
> same hash).
>
> It's used mainly to map together events. For example, if you print the
> address of a skb in the networking events, it's good to know what
> events reference the same skb, and the pointer is used for that.

So what is your opinion on ssid? I dont mind removing them
now since people dont like it and the strong use-case is not
strong (yet). Is there any problem to put getting them back
later if useful? And then before the strings so the evaluation
of filter first come on number before stings Or is there already
some mechanism that optimize for that?


> -- Steve

Re: [PATCH v2 2/2] selinux: add basic filtering for audit trace events

[Steven Rostedt]

On Thu, 13 Aug 2020 20:18:55 +0200
peter enderborg <peter.enderborg@sony.com> wrote:

> > The "%p" gets obfuscated when printed from the trace file by default
> > now. But they are consistent (where the same pointer shows up as the
> > same hash).
> >
> > It's used mainly to map together events. For example, if you print the
> > address of a skb in the networking events, it's good to know what
> > events reference the same skb, and the pointer is used for that.  
> 
> So what is your opinion on ssid? I dont mind removing them
> now since people dont like it and the strong use-case is not
> strong (yet). Is there any problem to put getting them back
> later if useful? And then before the strings so the evaluation
> of filter first come on number before stings Or is there already
> some mechanism that optimize for that?

It's up to the owner of the trace event. I only replied to why pointers
in general are useful, but they are mostly just "ids" to map to other
trace events.

We have the libtraceevent that should be used for parsing raw trace
events in binary form. The library (which currently lives in the
kernel's tools/lib/traceeevnt directory) I'm trying to get to have its
own home that distros can package. It should never be an issue adding
another field to an event, as the library gives the tools the ability
to find a field of an event regardless of where it is positioned, and
also let the tools know if the field exists or not.

If that's what you are asking.

-- Steve

Re: [PATCH v2 1/2] selinux: add tracepoint on denials

[Thiébaud Weksteen]

On Thu, Aug 13, 2020 at 5:41 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> An explanation here of how one might go about decoding audited and
> tclass would be helpful to users (even better would be a script to do it
> for them).  Again, I know how to do that but not everyone using
> perf/ftrace will.

What about something along those lines:

The tclass value can be mapped to a class by searching
security/selinux/flask.h. The audited value is a bit field of the
permissions described in security/selinux/av_permissions.h for the
corresponding class.

Re: [PATCH v2 1/2] selinux: add tracepoint on denials

[Stephen Smalley]

On Fri, Aug 14, 2020 at 9:05 AM Thiébaud Weksteen <tweek@google.com> wrote:
>
> On Thu, Aug 13, 2020 at 5:41 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > An explanation here of how one might go about decoding audited and
> > tclass would be helpful to users (even better would be a script to do it
> > for them).  Again, I know how to do that but not everyone using
> > perf/ftrace will.
>
> What about something along those lines:
>
> The tclass value can be mapped to a class by searching
> security/selinux/flask.h. The audited value is a bit field of the
> permissions described in security/selinux/av_permissions.h for the
> corresponding class.

Sure, I guess that works.  Would be nice if we just included the class
and permission name(s) in the event itself but I guess you viewed that
as too heavyweight?

Re: [PATCH v2 1/2] selinux: add tracepoint on denials

[peter enderborg]

On 8/14/20 6:51 PM, Stephen Smalley wrote:
> On Fri, Aug 14, 2020 at 9:05 AM Thiébaud Weksteen <tweek@google.com> wrote:
>> On Thu, Aug 13, 2020 at 5:41 PM Stephen Smalley
>> <stephen.smalley.work@gmail.com> wrote:
>>> An explanation here of how one might go about decoding audited and
>>> tclass would be helpful to users (even better would be a script to do it
>>> for them).  Again, I know how to do that but not everyone using
>>> perf/ftrace will.
>> What about something along those lines:
>>
>> The tclass value can be mapped to a class by searching
>> security/selinux/flask.h. The audited value is a bit field of the
>> permissions described in security/selinux/av_permissions.h for the
>> corresponding class.
> Sure, I guess that works.  Would be nice if we just included the class
> and permission name(s) in the event itself but I guess you viewed that
> as too heavyweight?

The class name is added in part 2. Im not sure how a proper format for permission
would look like in trace terms. It is a list, right?

Re: [PATCH v2 1/2] selinux: add tracepoint on denials

[Stephen Smalley]

On Fri, Aug 14, 2020 at 1:07 PM peter enderborg
<peter.enderborg@sony.com> wrote:
>
> On 8/14/20 6:51 PM, Stephen Smalley wrote:
> > On Fri, Aug 14, 2020 at 9:05 AM Thiébaud Weksteen <tweek@google.com> wrote:
> >> On Thu, Aug 13, 2020 at 5:41 PM Stephen Smalley
> >> <stephen.smalley.work@gmail.com> wrote:
> >>> An explanation here of how one might go about decoding audited and
> >>> tclass would be helpful to users (even better would be a script to do it
> >>> for them).  Again, I know how to do that but not everyone using
> >>> perf/ftrace will.
> >> What about something along those lines:
> >>
> >> The tclass value can be mapped to a class by searching
> >> security/selinux/flask.h. The audited value is a bit field of the
> >> permissions described in security/selinux/av_permissions.h for the
> >> corresponding class.
> > Sure, I guess that works.  Would be nice if we just included the class
> > and permission name(s) in the event itself but I guess you viewed that
> > as too heavyweight?
>
> The class name is added in part 2. Im not sure how a proper format for permission
> would look like in trace terms. It is a list, right?

Yes.  See avc_audit_pre_callback() for example code to log the permission names.

Re: [PATCH v2 1/2] selinux: add tracepoint on denials

[peter enderborg]

On 8/14/20 7:08 PM, Stephen Smalley wrote:
> On Fri, Aug 14, 2020 at 1:07 PM peter enderborg
> <peter.enderborg@sony.com> wrote:
>> On 8/14/20 6:51 PM, Stephen Smalley wrote:
>>> On Fri, Aug 14, 2020 at 9:05 AM Thiébaud Weksteen <tweek@google.com> wrote:
>>>> On Thu, Aug 13, 2020 at 5:41 PM Stephen Smalley
>>>> <stephen.smalley.work@gmail.com> wrote:
>>>>> An explanation here of how one might go about decoding audited and
>>>>> tclass would be helpful to users (even better would be a script to do it
>>>>> for them).  Again, I know how to do that but not everyone using
>>>>> perf/ftrace will.
>>>> What about something along those lines:
>>>>
>>>> The tclass value can be mapped to a class by searching
>>>> security/selinux/flask.h. The audited value is a bit field of the
>>>> permissions described in security/selinux/av_permissions.h for the
>>>> corresponding class.
>>> Sure, I guess that works.  Would be nice if we just included the class
>>> and permission name(s) in the event itself but I guess you viewed that
>>> as too heavyweight?
>> The class name is added in part 2. Im not sure how a proper format for permission
>> would look like in trace terms. It is a list, right?
> Yes.  See avc_audit_pre_callback() for example code to log the permission names.

I wrote about that on some of the previous sets. The problem is that trace format is quite fixed. So it is lists are not
that easy to handle if you want to filter in them. You can have a trace event for each of them. You can also add
additional trace event "selinux_audied_permission" for each permission. With that you can filter out tclass or permissions.

But the basic thing we would like at the moment is a event that we can debug in user space.

Re: [PATCH v2 1/2] selinux: add tracepoint on denials

[Steven Rostedt]

On Fri, 14 Aug 2020 19:22:13 +0200
peter enderborg <peter.enderborg@sony.com> wrote:

> On 8/14/20 7:08 PM, Stephen Smalley wrote:
> > On Fri, Aug 14, 2020 at 1:07 PM peter enderborg
> > <peter.enderborg@sony.com> wrote:  
> >> On 8/14/20 6:51 PM, Stephen Smalley wrote:  
> >>> On Fri, Aug 14, 2020 at 9:05 AM Thiébaud Weksteen <tweek@google.com> wrote:  
> >>>> On Thu, Aug 13, 2020 at 5:41 PM Stephen Smalley
> >>>> <stephen.smalley.work@gmail.com> wrote:  
> >>>>> An explanation here of how one might go about decoding audited and
> >>>>> tclass would be helpful to users (even better would be a script to do it
> >>>>> for them).  Again, I know how to do that but not everyone using
> >>>>> perf/ftrace will.  
> >>>> What about something along those lines:
> >>>>
> >>>> The tclass value can be mapped to a class by searching
> >>>> security/selinux/flask.h. The audited value is a bit field of the
> >>>> permissions described in security/selinux/av_permissions.h for the
> >>>> corresponding class.  
> >>> Sure, I guess that works.  Would be nice if we just included the class
> >>> and permission name(s) in the event itself but I guess you viewed that
> >>> as too heavyweight?  
> >> The class name is added in part 2. Im not sure how a proper format for permission
> >> would look like in trace terms. It is a list, right?  
> > Yes.  See avc_audit_pre_callback() for example code to log the permission names.  
> 
> I wrote about that on some of the previous sets. The problem is that trace format is quite fixed. So it is lists are not
> that easy to handle if you want to filter in them. You can have a trace event for each of them. You can also add
> additional trace event "selinux_audied_permission" for each permission. With that you can filter out tclass or permissions.
> 
> But the basic thing we would like at the moment is a event that we can debug in user space.

We have a trace_seq p helper, that lets you create strings in
TP_printk(). I should document this more. Thus you can do:

extern const char *audit_perm_to_name(struct trace_seq *p, u16 class, u32 audited);
#define __perm_to_name(p, class, audited) audit_perm_to_name(p, class, audited)

	TP_printk("tclass=%u audited=%x (%s)",
		__entry->tclass,
		__entry->audited,
		__perm_to_name(__entry->tclass, __entry->audited))


const char *audit_perm_to_name(struct trace_seq *p, u16 tclass, u32 av)
{
	const char *ret = trace_seq_buffer_ptr(p);
	int i, perm;

	( some check for tclass integrity here)

	perms = secclass_map[tclass-1].perms;

	i = 0;
	perm = 1;
	while (i < (sizeof(av) * 8)) {
		if ((perm & av) && perms[i]) {
			trace_seq_printf(p, " %s", perms[i]);
			av &= ~perm;
		}
		i++;
		perm <<= 1;
	}

	return ret;
}

Note, this wont work for perf and trace-cmd as it wouldn't know how to
parse it, but if the tclass perms are stable, you could create a plugin
to libtraceevent that can do the above as well.

-- Steve

Re: [PATCH v2 1/2] selinux: add tracepoint on denials

[peter enderborg]

On 8/14/20 7:46 PM, Steven Rostedt wrote:
> On Fri, 14 Aug 2020 19:22:13 +0200
> peter enderborg <peter.enderborg@sony.com> wrote:
>
>> On 8/14/20 7:08 PM, Stephen Smalley wrote:
>>> On Fri, Aug 14, 2020 at 1:07 PM peter enderborg
>>> <peter.enderborg@sony.com> wrote:  
>>>> On 8/14/20 6:51 PM, Stephen Smalley wrote:  
>>>>> On Fri, Aug 14, 2020 at 9:05 AM Thiébaud Weksteen <tweek@google.com> wrote:  
>>>>>> On Thu, Aug 13, 2020 at 5:41 PM Stephen Smalley
>>>>>> <stephen.smalley.work@gmail.com> wrote:  
>>>>>>> An explanation here of how one might go about decoding audited and
>>>>>>> tclass would be helpful to users (even better would be a script to do it
>>>>>>> for them).  Again, I know how to do that but not everyone using
>>>>>>> perf/ftrace will.  
>>>>>> What about something along those lines:
>>>>>>
>>>>>> The tclass value can be mapped to a class by searching
>>>>>> security/selinux/flask.h. The audited value is a bit field of the
>>>>>> permissions described in security/selinux/av_permissions.h for the
>>>>>> corresponding class.  
>>>>> Sure, I guess that works.  Would be nice if we just included the class
>>>>> and permission name(s) in the event itself but I guess you viewed that
>>>>> as too heavyweight?  
>>>> The class name is added in part 2. Im not sure how a proper format for permission
>>>> would look like in trace terms. It is a list, right?  
>>> Yes.  See avc_audit_pre_callback() for example code to log the permission names.  
>> I wrote about that on some of the previous sets. The problem is that trace format is quite fixed. So it is lists are not
>> that easy to handle if you want to filter in them. You can have a trace event for each of them. You can also add
>> additional trace event "selinux_audied_permission" for each permission. With that you can filter out tclass or permissions.
>>
>> But the basic thing we would like at the moment is a event that we can debug in user space.
> We have a trace_seq p helper, that lets you create strings in
> TP_printk(). I should document this more. Thus you can do:
>
> extern const char *audit_perm_to_name(struct trace_seq *p, u16 class, u32 audited);
> #define __perm_to_name(p, class, audited) audit_perm_to_name(p, class, audited)
>
> 	TP_printk("tclass=%u audited=%x (%s)",
> 		__entry->tclass,
> 		__entry->audited,
> 		__perm_to_name(__entry->tclass, __entry->audited))
>
>
> const char *audit_perm_to_name(struct trace_seq *p, u16 tclass, u32 av)
> {
> 	const char *ret = trace_seq_buffer_ptr(p);
> 	int i, perm;
>
> 	( some check for tclass integrity here)
>
> 	perms = secclass_map[tclass-1].perms;
>
> 	i = 0;
> 	perm = 1;
> 	while (i < (sizeof(av) * 8)) {
> 		if ((perm & av) && perms[i]) {
> 			trace_seq_printf(p, " %s", perms[i]);
> 			av &= ~perm;
> 		}
> 		i++;
> 		perm <<= 1;
> 	}
>
> 	return ret;
> }
>
> Note, this wont work for perf and trace-cmd as it wouldn't know how to
> parse it, but if the tclass perms are stable, you could create a plugin
> to libtraceevent that can do the above as well.
>
> -- Steve

Im find with that, but then you  can not do filtering? I would be pretty neat with a filter saying tclass=file permission=write.

Re: [PATCH v2 1/2] selinux: add tracepoint on denials

[Steven Rostedt]

On Fri, 14 Aug 2020 20:06:34 +0200
peter enderborg <peter.enderborg@sony.com> wrote:

> Im find with that, but then you  can not do filtering? I would be
> pretty neat with a filter saying tclass=file permission=write.
> 

Well, if the mapping is stable, you could do:

	(tclass == 6) && (audited & 0x4)

-- Steve

Re: [PATCH v2 1/2] selinux: add tracepoint on denials

[peter enderborg]

On 8/14/20 8:30 PM, Steven Rostedt wrote:
> On Fri, 14 Aug 2020 20:06:34 +0200
> peter enderborg <peter.enderborg@sony.com> wrote:
>
>> Im find with that, but then you  can not do filtering? I would be
>> pretty neat with a filter saying tclass=file permission=write.
>>
> Well, if the mapping is stable, you could do:
>
> 	(tclass == 6) && (audited & 0x4)

It does not happen to exist a hook for translate strings to numeric values when inserting filter?


> -- Steve

Re: [PATCH v2 1/2] selinux: add tracepoint on denials

[Steven Rostedt]

On Fri, 14 Aug 2020 20:50:47 +0200
peter enderborg <peter.enderborg@sony.com> wrote:

> On 8/14/20 8:30 PM, Steven Rostedt wrote:
> > On Fri, 14 Aug 2020 20:06:34 +0200
> > peter enderborg <peter.enderborg@sony.com> wrote:
> >  
> >> Im find with that, but then you  can not do filtering? I would be
> >> pretty neat with a filter saying tclass=file permission=write.
> >>  
> > Well, if the mapping is stable, you could do:
> >
> > 	(tclass == 6) && (audited & 0x4)  
> 
> It does not happen to exist a hook for translate strings to numeric values when inserting filter?
> 

How would you imagine such a hook existing?

Something that would be specific to each trace event class, where you
can register at boot up a mapping of names to values? Or a function
that would translate it?

-- Steve

Re: [PATCH v2 1/2] selinux: add tracepoint on denials

[peter enderborg]

On 8/14/20 7:46 PM, Steven Rostedt wrote:
> On Fri, 14 Aug 2020 19:22:13 +0200
> peter enderborg <peter.enderborg@sony.com> wrote:
>
>> On 8/14/20 7:08 PM, Stephen Smalley wrote:
>>> On Fri, Aug 14, 2020 at 1:07 PM peter enderborg
>>> <peter.enderborg@sony.com> wrote:  
>>>> On 8/14/20 6:51 PM, Stephen Smalley wrote:  
>>>>> On Fri, Aug 14, 2020 at 9:05 AM Thiébaud Weksteen <tweek@google.com> wrote:  
>>>>>> On Thu, Aug 13, 2020 at 5:41 PM Stephen Smalley
>>>>>> <stephen.smalley.work@gmail.com> wrote:  
>>>>>>> An explanation here of how one might go about decoding audited and
>>>>>>> tclass would be helpful to users (even better would be a script to do it
>>>>>>> for them).  Again, I know how to do that but not everyone using
>>>>>>> perf/ftrace will.  
>>>>>> What about something along those lines:
>>>>>>
>>>>>> The tclass value can be mapped to a class by searching
>>>>>> security/selinux/flask.h. The audited value is a bit field of the
>>>>>> permissions described in security/selinux/av_permissions.h for the
>>>>>> corresponding class.  
>>>>> Sure, I guess that works.  Would be nice if we just included the class
>>>>> and permission name(s) in the event itself but I guess you viewed that
>>>>> as too heavyweight?  
>>>> The class name is added in part 2. Im not sure how a proper format for permission
>>>> would look like in trace terms. It is a list, right?  
>>> Yes.  See avc_audit_pre_callback() for example code to log the permission names.  
>> I wrote about that on some of the previous sets. The problem is that trace format is quite fixed. So it is lists are not
>> that easy to handle if you want to filter in them. You can have a trace event for each of them. You can also add
>> additional trace event "selinux_audied_permission" for each permission. With that you can filter out tclass or permissions.
>>
>> But the basic thing we would like at the moment is a event that we can debug in user space.
> We have a trace_seq p helper, that lets you create strings in
> TP_printk(). I should document this more. Thus you can do:
>
> extern const char *audit_perm_to_name(struct trace_seq *p, u16 class, u32 audited);
> #define __perm_to_name(p, class, audited) audit_perm_to_name(p, class, audited)
>
> 	TP_printk("tclass=%u audited=%x (%s)",
> 		__entry->tclass,
> 		__entry->audited,
> 		__perm_to_name(__entry->tclass, __entry->audited))
>
>
> const char *audit_perm_to_name(struct trace_seq *p, u16 tclass, u32 av)
> {
> 	const char *ret = trace_seq_buffer_ptr(p);
> 	int i, perm;
>
> 	( some check for tclass integrity here)
>
> 	perms = secclass_map[tclass-1].perms;
>
> 	i = 0;
> 	perm = 1;
> 	while (i < (sizeof(av) * 8)) {
> 		if ((perm & av) && perms[i]) {
> 			trace_seq_printf(p, " %s", perms[i]);
> 			av &= ~perm;
> 		}
> 		i++;
> 		perm <<= 1;
> 	}
>
> 	return ret;
> }
>
> Note, this wont work for perf and trace-cmd as it wouldn't know how to
> parse it, but if the tclass perms are stable, you could create a plugin
> to libtraceevent that can do the above as well.
>
> -- Steve

That works fine. I will do this as third patch in our patch-set.  But I think we also should export the permission-map
somewhere. I don’t think there is any good place for it in tracefs. So selinuxfs or debugfs might do? And I think it is
more useful to print what is denied than what is audited but that does not match the trace event name.

Re: [PATCH v2 1/2] selinux: add tracepoint on denials

[peter enderborg]

On 8/14/20 7:46 PM, Steven Rostedt wrote:
> On Fri, 14 Aug 2020 19:22:13 +0200
> peter enderborg <peter.enderborg@sony.com> wrote:
>
>> On 8/14/20 7:08 PM, Stephen Smalley wrote:
>>> On Fri, Aug 14, 2020 at 1:07 PM peter enderborg
>>> <peter.enderborg@sony.com> wrote:  
>>>> On 8/14/20 6:51 PM, Stephen Smalley wrote:  
>>>>> On Fri, Aug 14, 2020 at 9:05 AM Thiébaud Weksteen <tweek@google.com> wrote:  
>>>>>> On Thu, Aug 13, 2020 at 5:41 PM Stephen Smalley
>>>>>> <stephen.smalley.work@gmail.com> wrote:  
>>>>>>> An explanation here of how one might go about decoding audited and
>>>>>>> tclass would be helpful to users (even better would be a script to do it
>>>>>>> for them).  Again, I know how to do that but not everyone using
>>>>>>> perf/ftrace will.  
>>>>>> What about something along those lines:
>>>>>>
>>>>>> The tclass value can be mapped to a class by searching
>>>>>> security/selinux/flask.h. The audited value is a bit field of the
>>>>>> permissions described in security/selinux/av_permissions.h for the
>>>>>> corresponding class.  
>>>>> Sure, I guess that works.  Would be nice if we just included the class
>>>>> and permission name(s) in the event itself but I guess you viewed that
>>>>> as too heavyweight?  
>>>> The class name is added in part 2. Im not sure how a proper format for permission
>>>> would look like in trace terms. It is a list, right?  
>>> Yes.  See avc_audit_pre_callback() for example code to log the permission names.  
>> I wrote about that on some of the previous sets. The problem is that trace format is quite fixed. So it is lists are not
>> that easy to handle if you want to filter in them. You can have a trace event for each of them. You can also add
>> additional trace event "selinux_audied_permission" for each permission. With that you can filter out tclass or permissions.
>>
>> But the basic thing we would like at the moment is a event that we can debug in user space.
> We have a trace_seq p helper, that lets you create strings in
> TP_printk(). I should document this more. Thus you can do:
>
> extern const char *audit_perm_to_name(struct trace_seq *p, u16 class, u32 audited);
> #define __perm_to_name(p, class, audited) audit_perm_to_name(p, class, audited)
>
> 	TP_printk("tclass=%u audited=%x (%s)",
> 		__entry->tclass,
> 		__entry->audited,
> 		__perm_to_name(__entry->tclass, __entry->audited))
>
>
> const char *audit_perm_to_name(struct trace_seq *p, u16 tclass, u32 av)
> {
> 	const char *ret = trace_seq_buffer_ptr(p);
> 	int i, perm;
>
> 	( some check for tclass integrity here)
>
> 	perms = secclass_map[tclass-1].perms;
>
> 	i = 0;
> 	perm = 1;
> 	while (i < (sizeof(av) * 8)) {
> 		if ((perm & av) && perms[i]) {
> 			trace_seq_printf(p, " %s", perms[i]);
> 			av &= ~perm;
> 		}
> 		i++;
> 		perm <<= 1;
> 	}
>
> 	return ret;
> }
>
> Note, this wont work for perf and trace-cmd as it wouldn't know how to
> parse it, but if the tclass perms are stable, you could create a plugin
> to libtraceevent that can do the above as well.
>
> -- Steve

Something like:

    while (i < (sizeof(av) * 8)) {
        if ((perm & av)  && perms[i]) {
            if (!(perm & avdenied))
                trace_seq_printf(p, " %s", perms[i]);
            else
                trace_seq_printf(p, " !%s", perms[i]);
            av &= ~perm;

And you get information about denied too.