[PATCH v3 0/3] selinux: add detailed tracepoint on audited events

[PATCH v3 0/3] selinux: add detailed tracepoint on audited events

[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 adding a
trace point when an event is audited.

This series of patch defines this new trace point and extra attributes
to easily match the tracepoint event with the audit event. It is also
possible to filter the events based on these attributes.

Changes since v2
================
- Add patch to include decoded permissions.
- Remove ssid and tsid from attributes list.
- Update commit log with more context.

Peter Enderborg (2):
  selinux: add basic filtering for audit trace events
  selinux: add permission names to trace event

Thiébaud Weksteen (1):
  selinux: add tracepoint on denials

 MAINTAINERS                |  1 +
 include/trace/events/avc.h | 60 ++++++++++++++++++++++++++++++++++++++
 security/selinux/avc.c     | 59 ++++++++++++++++++++++++++++++++-----
 3 files changed, 113 insertions(+), 7 deletions(-)
 create mode 100644 include/trace/events/avc.h

-- 
2.28.0.220.ged08abb693-goog

[PATCH v3 1/3] selinux: add tracepoint on audited events

[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

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.

[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>
---
 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.220.ged08abb693-goog

[PATCH v3 2/3] 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
       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\" " > events/avc/selinux_audited/filter

This adds that we only get tclass=file.

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
     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>

Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
Reviewed-by: Thiébaud Weksteen <tweek@google.com>
---
 include/trace/events/avc.h | 36 ++++++++++++++++++++++++++----------
 security/selinux/avc.c     | 22 +++++++++++++---------
 2 files changed, 39 insertions(+), 19 deletions(-)

diff --git a/include/trace/events/avc.h b/include/trace/events/avc.h
index 07c058a9bbcd..b55fda2e0773 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,38 @@
 
 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)
 	),
 
 	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;
+		__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 scontext=%s tcontext=%s tclass=%s",
+		__entry->requested, __entry->denied, __entry->audited, __entry->result,
+		__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.220.ged08abb693-goog

[PATCH v3 3/3] selinux: add permission names to trace event

[Thiébaud Weksteen]

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

In the print out add permissions, it will look like:
    <...>-1042  [007] ....   201.965142: selinux_audited:
    requested=0x4000000 denied=0x4000000 audited=0x4000000
    result=-13
    scontext=system_u:system_r:cupsd_t:s0-s0:c0.c1023
    tcontext=system_u:object_r:bin_t:s0
    tclass=file permissions={ !entrypoint }

This patch is adding the "permissions={ !entrypoint }".
The permissions preceded by "!" have been denied and the permissions
without have been accepted.

Note that permission filtering is done on the audited, denied or
requested attributes.

Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Reviewed-by: Thiébaud Weksteen <tweek@google.com>
Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
---
 include/trace/events/avc.h | 11 +++++++++--
 security/selinux/avc.c     | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/include/trace/events/avc.h b/include/trace/events/avc.h
index b55fda2e0773..94bca8bef8d2 100644
--- a/include/trace/events/avc.h
+++ b/include/trace/events/avc.h
@@ -10,6 +10,10 @@
 #define _TRACE_SELINUX_H
 
 #include <linux/tracepoint.h>
+#include <linux/trace_seq.h>
+
+extern const char *avc_trace_perm_to_name(struct trace_seq *p, u16 class, u32 audited, u32 denied);
+#define __perm_to_name(class, audited, denied) avc_trace_perm_to_name(p, class, audited, denied)
 
 TRACE_EVENT(selinux_audited,
 
@@ -29,6 +33,7 @@ TRACE_EVENT(selinux_audited,
 		__string(scontext, scontext)
 		__string(tcontext, tcontext)
 		__string(tclass, tclass)
+		__field(u16, utclass)
 	),
 
 	TP_fast_assign(
@@ -36,14 +41,16 @@ TRACE_EVENT(selinux_audited,
 		__entry->denied		= sad->denied;
 		__entry->audited	= sad->audited;
 		__entry->result		= sad->result;
+		__entry->utclass	= sad->tclass;
 		__assign_str(tcontext, tcontext);
 		__assign_str(scontext, scontext);
 		__assign_str(tclass, tclass);
 	),
 
-	TP_printk("requested=0x%x denied=0x%x audited=0x%x result=%d scontext=%s tcontext=%s tclass=%s",
+	TP_printk("requested=0x%x denied=0x%x audited=0x%x result=%d scontext=%s tcontext=%s tclass=%s permissions={%s }",
 		__entry->requested, __entry->denied, __entry->audited, __entry->result,
-		__get_str(scontext), __get_str(tcontext), __get_str(tclass)
+		__get_str(scontext), __get_str(tcontext), __get_str(tclass),
+		__perm_to_name(__entry->utclass, __entry->audited, __entry->denied)
 	)
 );
 
diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 7de5cc5169af..d585b68c2a50 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -695,6 +695,7 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a)
 	audit_log_format(ab, " } for ");
 }
 
+
 /**
  * avc_audit_post_callback - SELinux specific information
  * will be called by generic audit code
@@ -991,6 +992,41 @@ int avc_ss_reset(struct selinux_avc *avc, u32 seqno)
 	return rc;
 }
 
+/**
+ * avc_trace_perm_to_name - SELinux help function for trace
+ * @p pointer to output storage
+ * @tclass tclass for the event
+ * @av access vector
+ * @avdenied denied permissions in av format
+ */
+const char *avc_trace_perm_to_name(struct trace_seq *p, u16 tclass, u32 av, u32 avdenied)
+{
+	const char *ret = trace_seq_buffer_ptr(p);
+	int i, perm;
+	const char **perms;
+
+	if (WARN_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map)))
+		return NULL;
+
+	perms = secclass_map[tclass-1].perms;
+
+	i = 0;
+	perm = 1;
+	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;
+		}
+		i++;
+		perm <<= 1;
+	}
+
+	return ret;
+}
+
 /*
  * Slow-path helper function for avc_has_perm_noaudit,
  * when the avc_node lookup fails. We get called with
-- 
2.28.0.220.ged08abb693-goog

Re: [PATCH v3 3/3] selinux: add permission names to trace event

[Stephen Smalley]

On 8/17/20 1:07 PM, Thiébaud Weksteen wrote:

> From: Peter Enderborg <peter.enderborg@sony.com>
>
> In the print out add permissions, it will look like:
>      <...>-1042  [007] ....   201.965142: selinux_audited:
>      requested=0x4000000 denied=0x4000000 audited=0x4000000
>      result=-13
>      scontext=system_u:system_r:cupsd_t:s0-s0:c0.c1023
>      tcontext=system_u:object_r:bin_t:s0
>      tclass=file permissions={ !entrypoint }
>
> This patch is adding the "permissions={ !entrypoint }".
> The permissions preceded by "!" have been denied and the permissions
> without have been accepted.
>
> Note that permission filtering is done on the audited, denied or
> requested attributes.
>
> Suggested-by: Steven Rostedt <rostedt@goodmis.org>
> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> Reviewed-by: Thiébaud Weksteen <tweek@google.com>
> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
> ---

Does this require a corresponding patch to userspace?  Otherwise, I get 
the following:

libtraceevent: No such file or directory
   [avc:selinux_audited] function avc_trace_perm_to_name not defined

Re: [PATCH v3 3/3] selinux: add permission names to trace event

[Stephen Smalley]

On 8/17/20 1:07 PM, Thiébaud Weksteen wrote:

> From: Peter Enderborg <peter.enderborg@sony.com>
>
> In the print out add permissions, it will look like:
>      <...>-1042  [007] ....   201.965142: selinux_audited:
>      requested=0x4000000 denied=0x4000000 audited=0x4000000
>      result=-13
>      scontext=system_u:system_r:cupsd_t:s0-s0:c0.c1023
>      tcontext=system_u:object_r:bin_t:s0
>      tclass=file permissions={ !entrypoint }
>
> This patch is adding the "permissions={ !entrypoint }".
> The permissions preceded by "!" have been denied and the permissions
> without have been accepted.
>
> Note that permission filtering is done on the audited, denied or
> requested attributes.
>
> Suggested-by: Steven Rostedt <rostedt@goodmis.org>
> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> Reviewed-by: Thiébaud Weksteen <tweek@google.com>
> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
> ---
>   include/trace/events/avc.h | 11 +++++++++--
>   security/selinux/avc.c     | 36 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 45 insertions(+), 2 deletions(-)
>
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 7de5cc5169af..d585b68c2a50 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -695,6 +695,7 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a)
>   	audit_log_format(ab, " } for ");
>   }
>   
> +
>   /**
>    * avc_audit_post_callback - SELinux specific information
>    * will be called by generic audit code

Also, drop the spurious whitespace change above.

Re: [PATCH v3 3/3] selinux: add permission names to trace event

[Steven Rostedt]

On Mon, 17 Aug 2020 16:13:29 -0400
Stephen Smalley <stephen.smalley.work@gmail.com> wrote:

> Does this require a corresponding patch to userspace?  Otherwise, I get 
> the following:
> 
> libtraceevent: No such file or directory
>    [avc:selinux_audited] function avc_trace_perm_to_name not defined

Yes, we need to add a plugin to libtraceevent that will add that
function.

I could possibly write one up real quick.

-- Steve

Re: [PATCH v3 3/3] selinux: add permission names to trace event

[peter enderborg]

On 8/17/20 10:16 PM, Stephen Smalley wrote:
> On 8/17/20 1:07 PM, Thiébaud Weksteen wrote:
>
>> From: Peter Enderborg <peter.enderborg@sony.com>
>>
>> In the print out add permissions, it will look like:
>>      <...>-1042  [007] ....   201.965142: selinux_audited:
>>      requested=0x4000000 denied=0x4000000 audited=0x4000000
>>      result=-13
>>      scontext=system_u:system_r:cupsd_t:s0-s0:c0.c1023
>>      tcontext=system_u:object_r:bin_t:s0
>>      tclass=file permissions={ !entrypoint }
>>
>> This patch is adding the "permissions={ !entrypoint }".
>> The permissions preceded by "!" have been denied and the permissions
>> without have been accepted.
>>
>> Note that permission filtering is done on the audited, denied or
>> requested attributes.
>>
>> Suggested-by: Steven Rostedt <rostedt@goodmis.org>
>> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
>> Reviewed-by: Thiébaud Weksteen <tweek@google.com>
>> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
>> ---
>>   include/trace/events/avc.h | 11 +++++++++--
>>   security/selinux/avc.c     | 36 ++++++++++++++++++++++++++++++++++++
>>   2 files changed, 45 insertions(+), 2 deletions(-)
>>
>> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
>> index 7de5cc5169af..d585b68c2a50 100644
>> --- a/security/selinux/avc.c
>> +++ b/security/selinux/avc.c
>> @@ -695,6 +695,7 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a)
>>       audit_log_format(ab, " } for ");
>>   }
>>   +
>>   /**
>>    * avc_audit_post_callback - SELinux specific information
>>    * will be called by generic audit code
>
> Also, drop the spurious whitespace change above.
>
>
Is there any other things we need to fix? A part 1&2 now OK?

Re: [PATCH v3 3/3] selinux: add permission names to trace event

[Stephen Smalley]

On Tue, Aug 18, 2020 at 4:11 AM peter enderborg
<peter.enderborg@sony.com> wrote:
>
> On 8/17/20 10:16 PM, Stephen Smalley wrote:
> > On 8/17/20 1:07 PM, Thiébaud Weksteen wrote:
> >
> >> From: Peter Enderborg <peter.enderborg@sony.com>
> >>
> >> In the print out add permissions, it will look like:
> >>      <...>-1042  [007] ....   201.965142: selinux_audited:
> >>      requested=0x4000000 denied=0x4000000 audited=0x4000000
> >>      result=-13
> >>      scontext=system_u:system_r:cupsd_t:s0-s0:c0.c1023
> >>      tcontext=system_u:object_r:bin_t:s0
> >>      tclass=file permissions={ !entrypoint }
> >>
> >> This patch is adding the "permissions={ !entrypoint }".
> >> The permissions preceded by "!" have been denied and the permissions
> >> without have been accepted.
> >>
> >> Note that permission filtering is done on the audited, denied or
> >> requested attributes.
> >>
> >> Suggested-by: Steven Rostedt <rostedt@goodmis.org>
> >> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> >> Reviewed-by: Thiébaud Weksteen <tweek@google.com>
> >> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
> >> ---
> >>   include/trace/events/avc.h | 11 +++++++++--
> >>   security/selinux/avc.c     | 36 ++++++++++++++++++++++++++++++++++++
> >>   2 files changed, 45 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> >> index 7de5cc5169af..d585b68c2a50 100644
> >> --- a/security/selinux/avc.c
> >> +++ b/security/selinux/avc.c
> >> @@ -695,6 +695,7 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a)
> >>       audit_log_format(ab, " } for ");
> >>   }
> >>   +
> >>   /**
> >>    * avc_audit_post_callback - SELinux specific information
> >>    * will be called by generic audit code
> >
> > Also, drop the spurious whitespace change above.
> >
> >
> Is there any other things we need to fix? A part 1&2 now OK?

They looked ok to me, but Paul should review them.

Re: [PATCH v3 1/3] selinux: add tracepoint on audited events

[Stephen Smalley]

On 8/17/20 1:07 PM, 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
>
> 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.
>
> [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>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>

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

[Stephen Smalley]

On 8/17/20 1:07 PM, 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
>         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\" " > events/avc/selinux_audited/filter
>
> This adds that we only get tclass=file.
>
> 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
>       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>
>
> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
> Reviewed-by: Thiébaud Weksteen <tweek@google.com>
> ---
>   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);
>   	}
I guess technically you should drop the { } above since it is reduced to 
a single statement body.
>   
> -	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);
>   	}
Ditto.

Re: [PATCH v3 3/3] selinux: add permission names to trace event

[Steven Rostedt]

On Mon, 17 Aug 2020 16:29:33 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 17 Aug 2020 16:13:29 -0400
> Stephen Smalley <stephen.smalley.work@gmail.com> wrote:
> 
> > Does this require a corresponding patch to userspace?  Otherwise, I get 
> > the following:
> > 
> > libtraceevent: No such file or directory
> >    [avc:selinux_audited] function avc_trace_perm_to_name not defined  
> 
> Yes, we need to add a plugin to libtraceevent that will add that
> function.
> 
> I could possibly write one up real quick.

Something like this (this is patched on top of trace-cmd, but will work
for tools/lib/traceevent too).

With CONFIG_TRACE_EVENT_INJECT enabled (to test events), I did the following:

 # echo 'utclass=1 audited=1 denied=0' > /sys/kernel/tracing/events/avc/selinux_audited/inject
 # trace-cmd extract
 # trace-cmd report
cpus=8
           <...>-1685  [005]  1607.612032: selinux_audited:      requested=0x0 denied=0x0 audited=0x1 result=0 scontext= tcontext= tclass= permissions={ compute_av }

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

---
diff --git a/lib/traceevent/plugins/Makefile b/lib/traceevent/plugins/Makefile
index 21e933af..13cbcb92 100644
--- a/lib/traceevent/plugins/Makefile
+++ b/lib/traceevent/plugins/Makefile
@@ -16,6 +16,7 @@ PLUGIN_OBJS += plugin_scsi.o
 PLUGIN_OBJS += plugin_cfg80211.o
 PLUGIN_OBJS += plugin_blk.o
 PLUGIN_OBJS += plugin_tlb.o
+PLUGIN_OBJS += plugin_avc.o
 
 PLUGIN_OBJS := $(PLUGIN_OBJS:%.o=$(bdir)/%.o)
 PLUGIN_BUILD := $(PLUGIN_OBJS:$(bdir)/%.o=$(bdir)/%.so)
diff --git a/lib/traceevent/plugins/plugin_avc.c b/lib/traceevent/plugins/plugin_avc.c
new file mode 100644
index 00000000..76af23b9
--- /dev/null
+++ b/lib/traceevent/plugins/plugin_avc.c
@@ -0,0 +1,312 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <stdio.h>
+#include <string.h>
+#include "event-parse.h"
+
+#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
+
+typedef unsigned short u16;
+
+/* Class/perm mapping support */
+struct security_class_mapping {
+	const char *name;
+	const char *perms[sizeof(unsigned) * 8 + 1];
+};
+
+#define COMMON_FILE_SOCK_PERMS "ioctl", "read", "write", "create", \
+    "getattr", "setattr", "lock", "relabelfrom", "relabelto", "append", "map"
+
+#define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \
+    "rename", "execute", "quotaon", "mounton", "audit_access", \
+	"open", "execmod", "watch", "watch_mount", "watch_sb", \
+	"watch_with_perm", "watch_reads"
+
+#define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
+    "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom",  \
+    "sendto", "name_bind"
+
+#define COMMON_IPC_PERMS "create", "destroy", "getattr", "setattr", "read", \
+	    "write", "associate", "unix_read", "unix_write"
+
+#define COMMON_CAP_PERMS  "chown", "dac_override", "dac_read_search", \
+	    "fowner", "fsetid", "kill", "setgid", "setuid", "setpcap", \
+	    "linux_immutable", "net_bind_service", "net_broadcast", \
+	    "net_admin", "net_raw", "ipc_lock", "ipc_owner", "sys_module", \
+	    "sys_rawio", "sys_chroot", "sys_ptrace", "sys_pacct", "sys_admin", \
+	    "sys_boot", "sys_nice", "sys_resource", "sys_time", \
+	    "sys_tty_config", "mknod", "lease", "audit_write", \
+	    "audit_control", "setfcap"
+
+#define COMMON_CAP2_PERMS  "mac_override", "mac_admin", "syslog", \
+		"wake_alarm", "block_suspend", "audit_read", "perfmon", "bpf"
+
+/*
+ * Note: The name for any socket class should be suffixed by "socket",
+ *	 and doesn't contain more than one substr of "socket".
+ */
+struct security_class_mapping secclass_map[] = {
+	{ "security",
+	  { "compute_av", "compute_create", "compute_member",
+	    "check_context", "load_policy", "compute_relabel",
+	    "compute_user", "setenforce", "setbool", "setsecparam",
+	    "setcheckreqprot", "read_policy", "validate_trans", NULL } },
+	{ "process",
+	  { "fork", "transition", "sigchld", "sigkill",
+	    "sigstop", "signull", "signal", "ptrace", "getsched", "setsched",
+	    "getsession", "getpgid", "setpgid", "getcap", "setcap", "share",
+	    "getattr", "setexec", "setfscreate", "noatsecure", "siginh",
+	    "setrlimit", "rlimitinh", "dyntransition", "setcurrent",
+	    "execmem", "execstack", "execheap", "setkeycreate",
+	    "setsockcreate", "getrlimit", NULL } },
+	{ "process2",
+	  { "nnp_transition", "nosuid_transition", NULL } },
+	{ "system",
+	  { "ipc_info", "syslog_read", "syslog_mod",
+	    "syslog_console", "module_request", "module_load", NULL } },
+	{ "capability",
+	  { COMMON_CAP_PERMS, NULL } },
+	{ "filesystem",
+	  { "mount", "remount", "unmount", "getattr",
+	    "relabelfrom", "relabelto", "associate", "quotamod",
+	    "quotaget", "watch", NULL } },
+	{ "file",
+	  { COMMON_FILE_PERMS,
+	    "execute_no_trans", "entrypoint", NULL } },
+	{ "dir",
+	  { COMMON_FILE_PERMS, "add_name", "remove_name",
+	    "reparent", "search", "rmdir", NULL } },
+	{ "fd", { "use", NULL } },
+	{ "lnk_file",
+	  { COMMON_FILE_PERMS, NULL } },
+	{ "chr_file",
+	  { COMMON_FILE_PERMS, NULL } },
+	{ "blk_file",
+	  { COMMON_FILE_PERMS, NULL } },
+	{ "sock_file",
+	  { COMMON_FILE_PERMS, NULL } },
+	{ "fifo_file",
+	  { COMMON_FILE_PERMS, NULL } },
+	{ "socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "tcp_socket",
+	  { COMMON_SOCK_PERMS,
+	    "node_bind", "name_connect",
+	    NULL } },
+	{ "udp_socket",
+	  { COMMON_SOCK_PERMS,
+	    "node_bind", NULL } },
+	{ "rawip_socket",
+	  { COMMON_SOCK_PERMS,
+	    "node_bind", NULL } },
+	{ "node",
+	  { "recvfrom", "sendto", NULL } },
+	{ "netif",
+	  { "ingress", "egress", NULL } },
+	{ "netlink_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "packet_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "key_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "unix_stream_socket",
+	  { COMMON_SOCK_PERMS, "connectto", NULL } },
+	{ "unix_dgram_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "sem",
+	  { COMMON_IPC_PERMS, NULL } },
+	{ "msg", { "send", "receive", NULL } },
+	{ "msgq",
+	  { COMMON_IPC_PERMS, "enqueue", NULL } },
+	{ "shm",
+	  { COMMON_IPC_PERMS, "lock", NULL } },
+	{ "ipc",
+	  { COMMON_IPC_PERMS, NULL } },
+	{ "netlink_route_socket",
+	  { COMMON_SOCK_PERMS,
+	    "nlmsg_read", "nlmsg_write", NULL } },
+	{ "netlink_tcpdiag_socket",
+	  { COMMON_SOCK_PERMS,
+	    "nlmsg_read", "nlmsg_write", NULL } },
+	{ "netlink_nflog_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "netlink_xfrm_socket",
+	  { COMMON_SOCK_PERMS,
+	    "nlmsg_read", "nlmsg_write", NULL } },
+	{ "netlink_selinux_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "netlink_iscsi_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "netlink_audit_socket",
+	  { COMMON_SOCK_PERMS,
+	    "nlmsg_read", "nlmsg_write", "nlmsg_relay", "nlmsg_readpriv",
+	    "nlmsg_tty_audit", NULL } },
+	{ "netlink_fib_lookup_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "netlink_connector_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "netlink_netfilter_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "netlink_dnrt_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "association",
+	  { "sendto", "recvfrom", "setcontext", "polmatch", NULL } },
+	{ "netlink_kobject_uevent_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "netlink_generic_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "netlink_scsitransport_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "netlink_rdma_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "netlink_crypto_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "appletalk_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "packet",
+	  { "send", "recv", "relabelto", "forward_in", "forward_out", NULL } },
+	{ "key",
+	  { "view", "read", "write", "search", "link", "setattr", "create",
+	    NULL } },
+	{ "dccp_socket",
+	  { COMMON_SOCK_PERMS,
+	    "node_bind", "name_connect", NULL } },
+	{ "memprotect", { "mmap_zero", NULL } },
+	{ "peer", { "recv", NULL } },
+	{ "capability2",
+	  { COMMON_CAP2_PERMS, NULL } },
+	{ "kernel_service", { "use_as_override", "create_files_as", NULL } },
+	{ "tun_socket",
+	  { COMMON_SOCK_PERMS, "attach_queue", NULL } },
+	{ "binder", { "impersonate", "call", "set_context_mgr", "transfer",
+		      NULL } },
+	{ "cap_userns",
+	  { COMMON_CAP_PERMS, NULL } },
+	{ "cap2_userns",
+	  { COMMON_CAP2_PERMS, NULL } },
+	{ "sctp_socket",
+	  { COMMON_SOCK_PERMS,
+	    "node_bind", "name_connect", "association", NULL } },
+	{ "icmp_socket",
+	  { COMMON_SOCK_PERMS,
+	    "node_bind", NULL } },
+	{ "ax25_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "ipx_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "netrom_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "atmpvc_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "x25_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "rose_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "decnet_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "atmsvc_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "rds_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "irda_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "pppox_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "llc_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "can_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "tipc_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "bluetooth_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "iucv_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "rxrpc_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "isdn_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "phonet_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "ieee802154_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "caif_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "alg_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "nfc_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "vsock_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "kcm_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "qipcrtr_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "smc_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "infiniband_pkey",
+	  { "access", NULL } },
+	{ "infiniband_endport",
+	  { "manage_subnet", NULL } },
+	{ "bpf",
+	  {"map_create", "map_read", "map_write", "prog_load", "prog_run"} },
+	{ "xdp_socket",
+	  { COMMON_SOCK_PERMS, NULL } },
+	{ "perf_event",
+	  {"open", "cpu", "kernel", "tracepoint", "read", "write"} },
+	{ "lockdown",
+	  { "integrity", "confidentiality", NULL } },
+	{ NULL }
+  };
+
+
+static unsigned long long
+avc_trace_perm_to_name(struct trace_seq *p, unsigned long long *args)
+{
+	u16 tclass = args[1];
+	int av = args[2];
+	int avdenied = args[3];
+	int i, perm;
+	const char **perms;
+
+	if (!tclass || tclass >= ARRAY_SIZE(secclass_map))
+		return 0;
+
+	perms = secclass_map[tclass-1].perms;
+
+	i = 0;
+	perm = 1;
+	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;
+		}
+		i++;
+		perm <<= 1;
+	}
+
+	return 0;
+}
+
+int TEP_PLUGIN_LOADER(struct tep_handle *tep)
+{
+	tep_register_print_function(tep,
+				    avc_trace_perm_to_name,
+				    TEP_FUNC_ARG_STRING,
+				    "avc_trace_perm_to_name",
+				    TEP_FUNC_ARG_PTR,
+				    TEP_FUNC_ARG_INT,
+				    TEP_FUNC_ARG_INT,
+				    TEP_FUNC_ARG_INT,
+				    TEP_FUNC_ARG_VOID);
+
+	return 0;
+}
+
+void TEP_PLUGIN_UNLOADER(struct tep_handle *tep)
+{
+	tep_unregister_print_function(tep, avc_trace_perm_to_name,
+				    "avc_trace_perm_to_name");
+}

Re: [PATCH v3 3/3] selinux: add permission names to trace event

[Stephen Smalley]

On 8/18/20 12:09 PM, Steven Rostedt wrote:

> On Mon, 17 Aug 2020 16:29:33 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
>> On Mon, 17 Aug 2020 16:13:29 -0400
>> Stephen Smalley <stephen.smalley.work@gmail.com> wrote:
>>
>>> Does this require a corresponding patch to userspace?  Otherwise, I get
>>> the following:
>>>
>>> libtraceevent: No such file or directory
>>>     [avc:selinux_audited] function avc_trace_perm_to_name not defined
>> Yes, we need to add a plugin to libtraceevent that will add that
>> function.
>>
>> I could possibly write one up real quick.
> Something like this (this is patched on top of trace-cmd, but will work
> for tools/lib/traceevent too).
>
> With CONFIG_TRACE_EVENT_INJECT enabled (to test events), I did the following:
>
>   # echo 'utclass=1 audited=1 denied=0' > /sys/kernel/tracing/events/avc/selinux_audited/inject
>   # trace-cmd extract
>   # trace-cmd report
> cpus=8
>             <...>-1685  [005]  1607.612032: selinux_audited:      requested=0x0 denied=0x0 audited=0x1 result=0 scontext= tcontext= tclass= permissions={ compute_av }
>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>
> ---
> diff --git a/lib/traceevent/plugins/Makefile b/lib/traceevent/plugins/Makefile
> index 21e933af..13cbcb92 100644
> --- a/lib/traceevent/plugins/Makefile
> +++ b/lib/traceevent/plugins/Makefile
> @@ -16,6 +16,7 @@ PLUGIN_OBJS += plugin_scsi.o
>   PLUGIN_OBJS += plugin_cfg80211.o
>   PLUGIN_OBJS += plugin_blk.o
>   PLUGIN_OBJS += plugin_tlb.o
> +PLUGIN_OBJS += plugin_avc.o
>   
>   PLUGIN_OBJS := $(PLUGIN_OBJS:%.o=$(bdir)/%.o)
>   PLUGIN_BUILD := $(PLUGIN_OBJS:$(bdir)/%.o=$(bdir)/%.so)
> diff --git a/lib/traceevent/plugins/plugin_avc.c b/lib/traceevent/plugins/plugin_avc.c
> new file mode 100644
> index 00000000..76af23b9
> --- /dev/null
> +++ b/lib/traceevent/plugins/plugin_avc.c
> @@ -0,0 +1,312 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <stdio.h>
> +#include <string.h>
> +#include "event-parse.h"
> +
> +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> +
> +typedef unsigned short u16;
> +
> +/* Class/perm mapping support */
> +struct security_class_mapping {
> +	const char *name;
> +	const char *perms[sizeof(unsigned) * 8 + 1];
> +};
> +
> +#define COMMON_FILE_SOCK_PERMS "ioctl", "read", "write", "create", \
> +    "getattr", "setattr", "lock", "relabelfrom", "relabelto", "append", "map"
> +
> +#define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \
> +    "rename", "execute", "quotaon", "mounton", "audit_access", \
> +	"open", "execmod", "watch", "watch_mount", "watch_sb", \
> +	"watch_with_perm", "watch_reads"
> +
> +#define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
> +    "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom",  \
> +    "sendto", "name_bind"
> +
> +#define COMMON_IPC_PERMS "create", "destroy", "getattr", "setattr", "read", \
> +	    "write", "associate", "unix_read", "unix_write"
> +
> +#define COMMON_CAP_PERMS  "chown", "dac_override", "dac_read_search", \
> +	    "fowner", "fsetid", "kill", "setgid", "setuid", "setpcap", \
> +	    "linux_immutable", "net_bind_service", "net_broadcast", \
> +	    "net_admin", "net_raw", "ipc_lock", "ipc_owner", "sys_module", \
> +	    "sys_rawio", "sys_chroot", "sys_ptrace", "sys_pacct", "sys_admin", \
> +	    "sys_boot", "sys_nice", "sys_resource", "sys_time", \
> +	    "sys_tty_config", "mknod", "lease", "audit_write", \
> +	    "audit_control", "setfcap"
> +
> +#define COMMON_CAP2_PERMS  "mac_override", "mac_admin", "syslog", \
> +		"wake_alarm", "block_suspend", "audit_read", "perfmon", "bpf"
> +
> +/*
> + * Note: The name for any socket class should be suffixed by "socket",
> + *	 and doesn't contain more than one substr of "socket".
> + */
> +struct security_class_mapping secclass_map[] = {
> +	{ "security",
> +	  { "compute_av", "compute_create", "compute_member",
> +	    "check_context", "load_policy", "compute_relabel",
> +	    "compute_user", "setenforce", "setbool", "setsecparam",
> +	    "setcheckreqprot", "read_policy", "validate_trans", NULL } },
>
So we'll need to update this plugin whenever we modify 
security/selinux/include/classmap.h to keep them in sync.  Is that a 
concern?  I don't suppose the plugin could directly include classmap.h?  
I guess we'd have to export it as a public header. It isn't considered 
to be part of the kernel API/ABI and can change anytime (but in practice 
changes are not that frequent, and usually just additive in nature).

Re: [PATCH v3 3/3] selinux: add permission names to trace event

[Paul Moore]

On Tue, Aug 18, 2020 at 8:14 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Tue, Aug 18, 2020 at 4:11 AM peter enderborg <peter.enderborg@sony.com> wrote:

...

> > Is there any other things we need to fix? A part 1&2 now OK?
>
> They looked ok to me, but Paul should review them.

Patches 1 and 2 look fine to me with the small nits that Stephen
pointed out corrected.  I'm glad to see the information in string form
now, I think that will be a big help for people making use of this.

Unfortunately, I'm a little concerned about patch 3 for the reason
Stephen already mentioned.  While changes to the class mapping are
infrequent, they do happen, and I'm not very excited about adding it
to the userspace kAPI via a header.  Considering that the tracing
tools are going to be running on the same system that is being
inspected, perhaps the tracing tools could inspect
/sys/fs/selinux/class at runtime to query the permission mappings?
Stephen, is there a libselinux API which does this already?

-- 
paul moore
www.paul-moore.com

Re: [PATCH v3 3/3] selinux: add permission names to trace event

[Steven Rostedt]

On Wed, 19 Aug 2020 09:11:08 -0400
Stephen Smalley <stephen.smalley.work@gmail.com> wrote:

> So we'll need to update this plugin whenever we modify 
> security/selinux/include/classmap.h to keep them in sync.  Is that a 
> concern?  I don't suppose the plugin could directly include classmap.h?  
> I guess we'd have to export it as a public header. It isn't considered 
> to be part of the kernel API/ABI and can change anytime (but in practice 
> changes are not that frequent, and usually just additive in nature).

Yes, it would require some stability between userspace and the plugin.
If the value indexes don't change then that would work fine. If you add
new ones, that too should be OK, just have a way to state "unknown" in
the plugin.

-- Steve

Re: [PATCH v3 3/3] selinux: add permission names to trace event

[peter enderborg]

On 8/21/20 4:22 AM, Paul Moore wrote:
> On Tue, Aug 18, 2020 at 8:14 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
>> On Tue, Aug 18, 2020 at 4:11 AM peter enderborg <peter.enderborg@sony.com> wrote:
> ...
>
>>> Is there any other things we need to fix? A part 1&2 now OK?
>> They looked ok to me, but Paul should review them.
> Patches 1 and 2 look fine to me with the small nits that Stephen
> pointed out corrected.  I'm glad to see the information in string form
> now, I think that will be a big help for people making use of this.
>
> Unfortunately, I'm a little concerned about patch 3 for the reason
> Stephen already mentioned.  While changes to the class mapping are
> infrequent, they do happen, and I'm not very excited about adding it
> to the userspace kAPI via a header.  Considering that the tracing
> tools are going to be running on the same system that is being
> inspected, perhaps the tracing tools could inspect
> /sys/fs/selinux/class at runtime to query the permission mappings?
> Stephen, is there a libselinux API which does this already?
>
One way to use this trace is to write directly to a memory buffer over a time
period. In the case for Android and I guess in many other embedded cases too
they are moved to be some other machine to be analysed so having them
locked to where it was running also have problems.

So what is the problem we see with the plugin, that we have perms names
that are "unknown" ?

Re: [PATCH v3 3/3] selinux: add permission names to trace event

[Stephen Smalley]

On Thu, Aug 20, 2020 at 10:22 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Tue, Aug 18, 2020 at 8:14 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> > On Tue, Aug 18, 2020 at 4:11 AM peter enderborg <peter.enderborg@sony.com> wrote:
>
> ...
>
> > > Is there any other things we need to fix? A part 1&2 now OK?
> >
> > They looked ok to me, but Paul should review them.
>
> Patches 1 and 2 look fine to me with the small nits that Stephen
> pointed out corrected.  I'm glad to see the information in string form
> now, I think that will be a big help for people making use of this.
>
> Unfortunately, I'm a little concerned about patch 3 for the reason
> Stephen already mentioned.  While changes to the class mapping are
> infrequent, they do happen, and I'm not very excited about adding it
> to the userspace kAPI via a header.  Considering that the tracing
> tools are going to be running on the same system that is being
> inspected, perhaps the tracing tools could inspect
> /sys/fs/selinux/class at runtime to query the permission mappings?
> Stephen, is there a libselinux API which does this already?

There is a libselinux API but both it and the /sys/fs/selinux/class
tree is exposing the policy values for classes/permissions, not the
kernel-private indices.  The dynamic class/perm mapping support
introduced a layer of indirection between them.  The tracepoint is in
the avc and therefore dealing with the kernel-private values, not the
policy values.  The mapping occurs on entry/exit of the security
server functions. So there is no way for userspace to read the kernel
class/perm values.  We'd just need to keep them in sync manually.  And
one is allowed to insert new classes or permissions before existing
ones, thereby changing the values of existing ones, or even to remove
them.

Re: [PATCH v3 3/3] selinux: add permission names to trace event

[Stephen Smalley]

On Thu, Aug 20, 2020 at 10:31 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 19 Aug 2020 09:11:08 -0400
> Stephen Smalley <stephen.smalley.work@gmail.com> wrote:
>
> > So we'll need to update this plugin whenever we modify
> > security/selinux/include/classmap.h to keep them in sync.  Is that a
> > concern?  I don't suppose the plugin could directly include classmap.h?
> > I guess we'd have to export it as a public header. It isn't considered
> > to be part of the kernel API/ABI and can change anytime (but in practice
> > changes are not that frequent, and usually just additive in nature).
>
> Yes, it would require some stability between userspace and the plugin.
> If the value indexes don't change then that would work fine. If you add
> new ones, that too should be OK, just have a way to state "unknown" in
> the plugin.

Since we introduced the dynamic class/perm mapping support, it has
been possible for the values of existing classes/permissions to
change, and that has happened at time, e.g. when we added watch
permissions to the common file perms, that shifted the values of the
class file perms like entrypoint, when we added the process2 class
right after the process class, it shifted the values of all the
subsequent classes in the classmap.h.  So you can't rely on those
values remaining stable across kernel versions.

Re: [PATCH v3 3/3] selinux: add permission names to trace event

[Paul Moore]

On Fri, Aug 21, 2020 at 8:15 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Thu, Aug 20, 2020 at 10:22 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Tue, Aug 18, 2020 at 8:14 AM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > > On Tue, Aug 18, 2020 at 4:11 AM peter enderborg <peter.enderborg@sony.com> wrote:
> >
> > ...
> >
> > > > Is there any other things we need to fix? A part 1&2 now OK?
> > >
> > > They looked ok to me, but Paul should review them.
> >
> > Patches 1 and 2 look fine to me with the small nits that Stephen
> > pointed out corrected.  I'm glad to see the information in string form
> > now, I think that will be a big help for people making use of this.
> >
> > Unfortunately, I'm a little concerned about patch 3 for the reason
> > Stephen already mentioned.  While changes to the class mapping are
> > infrequent, they do happen, and I'm not very excited about adding it
> > to the userspace kAPI via a header.  Considering that the tracing
> > tools are going to be running on the same system that is being
> > inspected, perhaps the tracing tools could inspect
> > /sys/fs/selinux/class at runtime to query the permission mappings?
> > Stephen, is there a libselinux API which does this already?
>
> There is a libselinux API but both it and the /sys/fs/selinux/class
> tree is exposing the policy values for classes/permissions, not the
> kernel-private indices.  The dynamic class/perm mapping support
> introduced a layer of indirection between them.  The tracepoint is in
> the avc and therefore dealing with the kernel-private values, not the
> policy values.  The mapping occurs on entry/exit of the security
> server functions. So there is no way for userspace to read the kernel
> class/perm values.  We'd just need to keep them in sync manually.  And
> one is allowed to insert new classes or permissions before existing
> ones, thereby changing the values of existing ones, or even to remove
> them.

Ah, okay, thanks.  Can you tell I've never really had to look very
closely at that code ;)

-- 
paul moore
www.paul-moore.com

Re: [PATCH v3 3/3] selinux: add permission names to trace event

[Paul Moore]

On Fri, Aug 21, 2020 at 8:29 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Thu, Aug 20, 2020 at 10:31 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Wed, 19 Aug 2020 09:11:08 -0400
> > Stephen Smalley <stephen.smalley.work@gmail.com> wrote:
> >
> > > So we'll need to update this plugin whenever we modify
> > > security/selinux/include/classmap.h to keep them in sync.  Is that a
> > > concern?  I don't suppose the plugin could directly include classmap.h?
> > > I guess we'd have to export it as a public header. It isn't considered
> > > to be part of the kernel API/ABI and can change anytime (but in practice
> > > changes are not that frequent, and usually just additive in nature).
> >
> > Yes, it would require some stability between userspace and the plugin.
> > If the value indexes don't change then that would work fine. If you add
> > new ones, that too should be OK, just have a way to state "unknown" in
> > the plugin.
>
> Since we introduced the dynamic class/perm mapping support, it has
> been possible for the values of existing classes/permissions to
> change, and that has happened at time, e.g. when we added watch
> permissions to the common file perms, that shifted the values of the
> class file perms like entrypoint, when we added the process2 class
> right after the process class, it shifted the values of all the
> subsequent classes in the classmap.h.  So you can't rely on those
> values remaining stable across kernel versions.

I think it is becoming increasingly clear that generating the
permission set string in userspace isn't really workable without
breaking the dynamic class/permission mapping to some degree.
Unfortunately I don't see these perf changes as a big enough "win" to
offset the loss of the dynamic mapping loss.

I'm okay with merging patches 1/3 and 2/3 wth the changes Stephen
suggested, but I think we will need to leave patch 3/3 out of this for
now.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v3 3/3] selinux: add permission names to trace event

[peter enderborg]

On 8/21/20 3:19 PM, Paul Moore wrote:
> On Fri, Aug 21, 2020 at 8:29 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
>> On Thu, Aug 20, 2020 at 10:31 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>>> On Wed, 19 Aug 2020 09:11:08 -0400
>>> Stephen Smalley <stephen.smalley.work@gmail.com> wrote:
>>>
>>>> So we'll need to update this plugin whenever we modify
>>>> security/selinux/include/classmap.h to keep them in sync.  Is that a
>>>> concern?  I don't suppose the plugin could directly include classmap.h?
>>>> I guess we'd have to export it as a public header. It isn't considered
>>>> to be part of the kernel API/ABI and can change anytime (but in practice
>>>> changes are not that frequent, and usually just additive in nature).
>>> Yes, it would require some stability between userspace and the plugin.
>>> If the value indexes don't change then that would work fine. If you add
>>> new ones, that too should be OK, just have a way to state "unknown" in
>>> the plugin.
>> Since we introduced the dynamic class/perm mapping support, it has
>> been possible for the values of existing classes/permissions to
>> change, and that has happened at time, e.g. when we added watch
>> permissions to the common file perms, that shifted the values of the
>> class file perms like entrypoint, when we added the process2 class
>> right after the process class, it shifted the values of all the
>> subsequent classes in the classmap.h.  So you can't rely on those
>> values remaining stable across kernel versions.
> I think it is becoming increasingly clear that generating the
> permission set string in userspace isn't really workable without
> breaking the dynamic class/permission mapping to some degree.
> Unfortunately I don't see these perf changes as a big enough "win" to
> offset the loss of the dynamic mapping loss.
>
> I'm okay with merging patches 1/3 and 2/3 wth the changes Stephen
> suggested, but I think we will need to leave patch 3/3 out of this for
> now.
>
Ok.

Re: [PATCH v3 3/3] selinux: add permission names to trace event

[Paul Moore]

On Fri, Aug 21, 2020 at 9:21 AM Thiébaud Weksteen <tweek@google.com> wrote:
>>
>> I'm okay with merging patches 1/3 and 2/3 wth the changes Stephen
>> suggested, but I think we will need to leave patch 3/3 out of this for
>> now.
>
> That works for me.

Can you respin patches 1 and two with those changes and repost?  I try
to refrain from making non-merge-fuzz changes when merging patches.

-- 
paul moore
www.paul-moore.com

[RFC PATCH] selinux: Add denied trace with permssion filter

[Peter Enderborg]

This adds tracing of all denies. They are grouped with trace_seq for
each audit.

A filter can be inserted with a write to it's filter section.

echo "permission==\"entrypoint\"" > events/avc/selinux_denied/filter

A output will be like:
          runcon-1046  [002] .N..   156.351738: selinux_denied:
	  trace_seq=2 result=-13
	  scontext=system_u:system_r:cupsd_t:s0-s0:c0.
	  c1023 tcontext=system_u:object_r:bin_t:s0
	  tclass=file permission=entrypoint

Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
---
 include/trace/events/avc.h | 37 +++++++++++++++++++++++++++++++++++++
 security/selinux/avc.c     | 27 +++++++++++++++++++++++++--
 2 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/include/trace/events/avc.h b/include/trace/events/avc.h
index 94bca8bef8d2..9a28559956de 100644
--- a/include/trace/events/avc.h
+++ b/include/trace/events/avc.h
@@ -54,6 +54,43 @@ TRACE_EVENT(selinux_audited,
 	)
 );
 
+TRACE_EVENT(selinux_denied,
+
+	TP_PROTO(struct selinux_audit_data *sad,
+		char *scontext,
+		char *tcontext,
+		const char *tclass,
+		const char *permission,
+		int64_t seq
+	),
+
+	TP_ARGS(sad, scontext, tcontext, tclass, permission, seq),
+
+	TP_STRUCT__entry(
+		__field(int, result)
+		__string(scontext, scontext)
+		__string(tcontext, tcontext)
+		__string(permission, permission)
+		__string(tclass, tclass)
+		__field(u64, seq)
+	),
+
+	TP_fast_assign(
+		__entry->result	= sad->result;
+		__entry->seq	= seq;
+		__assign_str(tcontext, tcontext);
+		__assign_str(scontext, scontext);
+		__assign_str(permission, permission);
+		__assign_str(tclass, tclass);
+	),
+
+	TP_printk("trace_seq=%lld result=%d scontext=%s tcontext=%s tclass=%s permission=%s",
+		 __entry->seq, __entry->result, __get_str(scontext), __get_str(tcontext),
+		 __get_str(tclass), __get_str(permission)
+
+	)
+);
+
 #endif
 
 /* This part must be outside protection */
diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 1debddfb5af9..ca53baca15e1 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -92,6 +92,7 @@ struct selinux_avc {
 };
 
 static struct selinux_avc selinux_avc;
+static atomic64_t trace_seqno;
 
 void selinux_avc_init(struct selinux_avc **avc)
 {
@@ -731,9 +732,31 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
 	tclass = secclass_map[sad->tclass-1].name;
 	audit_log_format(ab, " tclass=%s", tclass);
 
-	if (sad->denied)
+	if (sad->denied) {
 		audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1);
-
+		if (trace_selinux_denied_enabled()) {
+			int i, perm;
+			const char **perms;
+			u32 denied = sad->denied;
+			int64_t seq;
+
+			seq = atomic_long_inc_return(&trace_seqno);
+			perms = secclass_map[sad->tclass-1].perms;
+			i = 0;
+			perm = 1;
+			while (i < (sizeof(denied) * 8)) {
+				if ((perm & denied & sad->requested) && perms[i]) {
+					trace_selinux_denied(sad, scontext, tcontext,
+							     tclass, perms[i], seq);
+					denied &= ~perm;
+					if (!denied)
+						break;
+				}
+				i++;
+				perm <<= 1;
+			}
+		}
+	}
 	trace_selinux_audited(sad, scontext, tcontext, tclass);
 	kfree(tcontext);
 	kfree(scontext);
-- 
2.17.1

Re: [RFC PATCH] selinux: Add denied trace with permssion filter

[Paul Moore]

On Mon, Aug 24, 2020 at 9:23 AM Peter Enderborg
<peter.enderborg@sony.com> wrote:
>
> This adds tracing of all denies. They are grouped with trace_seq for
> each audit.
>
> A filter can be inserted with a write to it's filter section.
>
> echo "permission==\"entrypoint\"" > events/avc/selinux_denied/filter
>
> A output will be like:
>           runcon-1046  [002] .N..   156.351738: selinux_denied:
>           trace_seq=2 result=-13
>           scontext=system_u:system_r:cupsd_t:s0-s0:c0.
>           c1023 tcontext=system_u:object_r:bin_t:s0
>           tclass=file permission=entrypoint
>
> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
> ---
>  include/trace/events/avc.h | 37 +++++++++++++++++++++++++++++++++++++
>  security/selinux/avc.c     | 27 +++++++++++++++++++++++++--
>  2 files changed, 62 insertions(+), 2 deletions(-)

My most significant comment is that I don't think we want, or need,
two trace points in the avc_audit_post_callback() function.  Yes, I
understand they are triggered slightly differently, but from my
perspective there isn't enough difference between the two tracepoints
to warrant including both.  However, while the tracepoints may be
redundant in my mind, this new event does do the permission lookup in
the kernel so that the contexts/class/permissions are all available as
a string which is a good thing.

Without going into the details, would the tracing folks be okay with
doing something similar with the existing selinux_audited tracepoint?
It's extra work in the kernel, but since it would only be triggered
when the tracepoint was active it seems bearable to me.

> diff --git a/include/trace/events/avc.h b/include/trace/events/avc.h
> index 94bca8bef8d2..9a28559956de 100644
> --- a/include/trace/events/avc.h
> +++ b/include/trace/events/avc.h
> @@ -54,6 +54,43 @@ TRACE_EVENT(selinux_audited,
>         )
>  );
>
> +TRACE_EVENT(selinux_denied,
> +
> +       TP_PROTO(struct selinux_audit_data *sad,
> +               char *scontext,
> +               char *tcontext,
> +               const char *tclass,
> +               const char *permission,
> +               int64_t seq
> +       ),
> +
> +       TP_ARGS(sad, scontext, tcontext, tclass, permission, seq),
> +
> +       TP_STRUCT__entry(
> +               __field(int, result)
> +               __string(scontext, scontext)
> +               __string(tcontext, tcontext)
> +               __string(permission, permission)
> +               __string(tclass, tclass)
> +               __field(u64, seq)
> +       ),
> +
> +       TP_fast_assign(
> +               __entry->result = sad->result;
> +               __entry->seq    = seq;
> +               __assign_str(tcontext, tcontext);
> +               __assign_str(scontext, scontext);
> +               __assign_str(permission, permission);
> +               __assign_str(tclass, tclass);
> +       ),
> +
> +       TP_printk("trace_seq=%lld result=%d scontext=%s tcontext=%s tclass=%s permission=%s",
> +                __entry->seq, __entry->result, __get_str(scontext), __get_str(tcontext),
> +                __get_str(tclass), __get_str(permission)
> +
> +       )
> +);
> +
>  #endif
>
>  /* This part must be outside protection */
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 1debddfb5af9..ca53baca15e1 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -92,6 +92,7 @@ struct selinux_avc {
>  };
>
>  static struct selinux_avc selinux_avc;
> +static atomic64_t trace_seqno;
>
>  void selinux_avc_init(struct selinux_avc **avc)
>  {
> @@ -731,9 +732,31 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
>         tclass = secclass_map[sad->tclass-1].name;
>         audit_log_format(ab, " tclass=%s", tclass);
>
> -       if (sad->denied)
> +       if (sad->denied) {
>                 audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1);
> -
> +               if (trace_selinux_denied_enabled()) {
> +                       int i, perm;
> +                       const char **perms;
> +                       u32 denied = sad->denied;
> +                       int64_t seq;
> +
> +                       seq = atomic_long_inc_return(&trace_seqno);
> +                       perms = secclass_map[sad->tclass-1].perms;
> +                       i = 0;
> +                       perm = 1;
> +                       while (i < (sizeof(denied) * 8)) {
> +                               if ((perm & denied & sad->requested) && perms[i]) {
> +                                       trace_selinux_denied(sad, scontext, tcontext,
> +                                                            tclass, perms[i], seq);
> +                                       denied &= ~perm;
> +                                       if (!denied)
> +                                               break;
> +                               }
> +                               i++;
> +                               perm <<= 1;
> +                       }
> +               }
> +       }
>         trace_selinux_audited(sad, scontext, tcontext, tclass);
>         kfree(tcontext);
>         kfree(scontext);
> --
> 2.17.1

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH] selinux: Add denied trace with permssion filter

[peter enderborg]

On 8/26/20 3:42 PM, Paul Moore wrote:
> On Mon, Aug 24, 2020 at 9:23 AM Peter Enderborg
> <peter.enderborg@sony.com> wrote:
>> This adds tracing of all denies. They are grouped with trace_seq for
>> each audit.
>>
>> A filter can be inserted with a write to it's filter section.
>>
>> echo "permission==\"entrypoint\"" > events/avc/selinux_denied/filter
>>
>> A output will be like:
>>           runcon-1046  [002] .N..   156.351738: selinux_denied:
>>           trace_seq=2 result=-13
>>           scontext=system_u:system_r:cupsd_t:s0-s0:c0.
>>           c1023 tcontext=system_u:object_r:bin_t:s0
>>           tclass=file permission=entrypoint
>>
>> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
>> ---
>>  include/trace/events/avc.h | 37 +++++++++++++++++++++++++++++++++++++
>>  security/selinux/avc.c     | 27 +++++++++++++++++++++++++--
>>  2 files changed, 62 insertions(+), 2 deletions(-)
> My most significant comment is that I don't think we want, or need,
> two trace points in the avc_audit_post_callback() function.  Yes, I
> understand they are triggered slightly differently, but from my
> perspective there isn't enough difference between the two tracepoints
> to warrant including both.  However, while the tracepoints may be

We tried that but that was problematic too.

Having partly overlapping traces is not unheard off.  Check
compaction.c where we have a     trace_mm_compaction_begin
and a more detailed trace_mm_compaction_migratepages.
(And a  trace_mm_compaction_end)


> redundant in my mind, this new event does do the permission lookup in
> the kernel so that the contexts/class/permissions are all available as
> a string which is a good thing.
>
> Without going into the details, would the tracing folks be okay with
> doing something similar with the existing selinux_audited tracepoint?
> It's extra work in the kernel, but since it would only be triggered
> when the tracepoint was active it seems bearable to me.

I think the method for expanding lists is what we tried first on
suggestion from Steven Rostedt.  Maybe we can do a trace_event
from a TP_prink but that would be recursive.

 

>> diff --git a/include/trace/events/avc.h b/include/trace/events/avc.h
>> index 94bca8bef8d2..9a28559956de 100644
>> --- a/include/trace/events/avc.h
>> +++ b/include/trace/events/avc.h
>> @@ -54,6 +54,43 @@ TRACE_EVENT(selinux_audited,
>>         )
>>  );
>>
>> +TRACE_EVENT(selinux_denied,
>> +
>> +       TP_PROTO(struct selinux_audit_data *sad,
>> +               char *scontext,
>> +               char *tcontext,
>> +               const char *tclass,
>> +               const char *permission,
>> +               int64_t seq
>> +       ),
>> +
>> +       TP_ARGS(sad, scontext, tcontext, tclass, permission, seq),
>> +
>> +       TP_STRUCT__entry(
>> +               __field(int, result)
>> +               __string(scontext, scontext)
>> +               __string(tcontext, tcontext)
>> +               __string(permission, permission)
>> +               __string(tclass, tclass)
>> +               __field(u64, seq)
>> +       ),
>> +
>> +       TP_fast_assign(
>> +               __entry->result = sad->result;
>> +               __entry->seq    = seq;
>> +               __assign_str(tcontext, tcontext);
>> +               __assign_str(scontext, scontext);
>> +               __assign_str(permission, permission);
>> +               __assign_str(tclass, tclass);
>> +       ),
>> +
>> +       TP_printk("trace_seq=%lld result=%d scontext=%s tcontext=%s tclass=%s permission=%s",
>> +                __entry->seq, __entry->result, __get_str(scontext), __get_str(tcontext),
>> +                __get_str(tclass), __get_str(permission)
>> +
>> +       )
>> +);
>> +
>>  #endif
>>
>>  /* This part must be outside protection */
>> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
>> index 1debddfb5af9..ca53baca15e1 100644
>> --- a/security/selinux/avc.c
>> +++ b/security/selinux/avc.c
>> @@ -92,6 +92,7 @@ struct selinux_avc {
>>  };
>>
>>  static struct selinux_avc selinux_avc;
>> +static atomic64_t trace_seqno;
>>
>>  void selinux_avc_init(struct selinux_avc **avc)
>>  {
>> @@ -731,9 +732,31 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
>>         tclass = secclass_map[sad->tclass-1].name;
>>         audit_log_format(ab, " tclass=%s", tclass);
>>
>> -       if (sad->denied)
>> +       if (sad->denied) {
>>                 audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1);
>> -
>> +               if (trace_selinux_denied_enabled()) {
>> +                       int i, perm;
>> +                       const char **perms;
>> +                       u32 denied = sad->denied;
>> +                       int64_t seq;
>> +
>> +                       seq = atomic_long_inc_return(&trace_seqno);
>> +                       perms = secclass_map[sad->tclass-1].perms;
>> +                       i = 0;
>> +                       perm = 1;
>> +                       while (i < (sizeof(denied) * 8)) {
>> +                               if ((perm & denied & sad->requested) && perms[i]) {
>> +                                       trace_selinux_denied(sad, scontext, tcontext,
>> +                                                            tclass, perms[i], seq);
>> +                                       denied &= ~perm;
>> +                                       if (!denied)
>> +                                               break;
>> +                               }
>> +                               i++;
>> +                               perm <<= 1;
>> +                       }
>> +               }
>> +       }
>>         trace_selinux_audited(sad, scontext, tcontext, tclass);
>>         kfree(tcontext);
>>         kfree(scontext);
>> --
>> 2.17.1

Re: [RFC PATCH] selinux: Add denied trace with permssion filter

[Paul Moore]

On Wed, Aug 26, 2020 at 10:34 AM peter enderborg
<peter.enderborg@sony.com> wrote:
> On 8/26/20 3:42 PM, Paul Moore wrote:
> > On Mon, Aug 24, 2020 at 9:23 AM Peter Enderborg
> > <peter.enderborg@sony.com> wrote:
> >> This adds tracing of all denies. They are grouped with trace_seq for
> >> each audit.
> >>
> >> A filter can be inserted with a write to it's filter section.
> >>
> >> echo "permission==\"entrypoint\"" > events/avc/selinux_denied/filter
> >>
> >> A output will be like:
> >>           runcon-1046  [002] .N..   156.351738: selinux_denied:
> >>           trace_seq=2 result=-13
> >>           scontext=system_u:system_r:cupsd_t:s0-s0:c0.
> >>           c1023 tcontext=system_u:object_r:bin_t:s0
> >>           tclass=file permission=entrypoint
> >>
> >> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
> >> ---
> >>  include/trace/events/avc.h | 37 +++++++++++++++++++++++++++++++++++++
> >>  security/selinux/avc.c     | 27 +++++++++++++++++++++++++--
> >>  2 files changed, 62 insertions(+), 2 deletions(-)
> > My most significant comment is that I don't think we want, or need,
> > two trace points in the avc_audit_post_callback() function.  Yes, I
> > understand they are triggered slightly differently, but from my
> > perspective there isn't enough difference between the two tracepoints
> > to warrant including both.  However, while the tracepoints may be
>
> We tried that but that was problematic too.

My apologies if I was on that thread, but can you remind me why it was
a problem?  Why can't we use a single tracepoint to capture the AVC
information?

> Having partly overlapping traces is not unheard off.  Check
> compaction.c where we have a     trace_mm_compaction_begin
> and a more detailed trace_mm_compaction_migratepages.
> (And a  trace_mm_compaction_end)

It may not be unique to SELinux, but that doesn't mean I like it :)

One of my concerns with adding tracepoints is that the code would get
littered with tracepoints; I accepted that it the AVC decision
codepath was an obvious place for one, so we added a tracepoint.
Having two tracepoints here is getting awfully close to my original
fears.

> > redundant in my mind, this new event does do the permission lookup in
> > the kernel so that the contexts/class/permissions are all available as
> > a string which is a good thing.
> >
> > Without going into the details, would the tracing folks be okay with
> > doing something similar with the existing selinux_audited tracepoint?
> > It's extra work in the kernel, but since it would only be triggered
> > when the tracepoint was active it seems bearable to me.
>
> I think the method for expanding lists is what we tried first on
> suggestion from Steven Rostedt.  Maybe we can do a trace_event
> from a TP_prink but that would be recursive.

Wait, why would you be adding a trace event to a trace event, or am I
misunderstanding you?

All I was talking about was adding the permission resolution code to
the already existing SELinux AVC tracepoint.

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH] selinux: Add denied trace with permssion filter

[peter enderborg]

On 8/26/20 4:45 PM, Paul Moore wrote:
> On Wed, Aug 26, 2020 at 10:34 AM peter enderborg
> <peter.enderborg@sony.com> wrote:
>> On 8/26/20 3:42 PM, Paul Moore wrote:
>>> On Mon, Aug 24, 2020 at 9:23 AM Peter Enderborg
>>> <peter.enderborg@sony.com> wrote:
>>>> This adds tracing of all denies. They are grouped with trace_seq for
>>>> each audit.
>>>>
>>>> A filter can be inserted with a write to it's filter section.
>>>>
>>>> echo "permission==\"entrypoint\"" > events/avc/selinux_denied/filter
>>>>
>>>> A output will be like:
>>>>           runcon-1046  [002] .N..   156.351738: selinux_denied:
>>>>           trace_seq=2 result=-13
>>>>           scontext=system_u:system_r:cupsd_t:s0-s0:c0.
>>>>           c1023 tcontext=system_u:object_r:bin_t:s0
>>>>           tclass=file permission=entrypoint
>>>>
>>>> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
>>>> ---
>>>>  include/trace/events/avc.h | 37 +++++++++++++++++++++++++++++++++++++
>>>>  security/selinux/avc.c     | 27 +++++++++++++++++++++++++--
>>>>  2 files changed, 62 insertions(+), 2 deletions(-)
>>> My most significant comment is that I don't think we want, or need,
>>> two trace points in the avc_audit_post_callback() function.  Yes, I
>>> understand they are triggered slightly differently, but from my
>>> perspective there isn't enough difference between the two tracepoints
>>> to warrant including both.  However, while the tracepoints may be
>> We tried that but that was problematic too.
> My apologies if I was on that thread, but can you remind me why it was
> a problem?  Why can't we use a single tracepoint to capture the AVC
> information?

The problem is parsing the event.

https://lkml.org/lkml/2020/8/18/842

https://lkml.org/lkml/2020/8/21/526

and the "single list" version

https://lkml.org/lkml/2020/8/17/1346

With this patch we follow standard message format so no plugin should be needed.


>> Having partly overlapping traces is not unheard off.  Check
>> compaction.c where we have a     trace_mm_compaction_begin
>> and a more detailed trace_mm_compaction_migratepages.
>> (And a  trace_mm_compaction_end)
> It may not be unique to SELinux, but that doesn't mean I like it :)
>
> One of my concerns with adding tracepoints is that the code would get
> littered with tracepoints; I accepted that it the AVC decision
> codepath was an obvious place for one, so we added a tracepoint.
> Having two tracepoints here is getting awfully close to my original
> fears.
>
>>> redundant in my mind, this new event does do the permission lookup in
>>> the kernel so that the contexts/class/permissions are all available as
>>> a string which is a good thing.
>>>
>>> Without going into the details, would the tracing folks be okay with
>>> doing something similar with the existing selinux_audited tracepoint?
>>> It's extra work in the kernel, but since it would only be triggered
>>> when the tracepoint was active it seems bearable to me.
>> I think the method for expanding lists is what we tried first on
>> suggestion from Steven Rostedt.  Maybe we can do a trace_event
>> from a TP_prink but that would be recursive.
> Wait, why would you be adding a trace event to a trace event, or am I
> misunderstanding you?
>
> All I was talking about was adding the permission resolution code to
> the already existing SELinux AVC tracepoint.
>

Re: [RFC PATCH] selinux: Add denied trace with permssion filter

[Paul Moore]

On Wed, Aug 26, 2020 at 11:06 AM peter enderborg
<peter.enderborg@sony.com> wrote:
> On 8/26/20 4:45 PM, Paul Moore wrote:
> > On Wed, Aug 26, 2020 at 10:34 AM peter enderborg
> > <peter.enderborg@sony.com> wrote:
> >> On 8/26/20 3:42 PM, Paul Moore wrote:
> >>> On Mon, Aug 24, 2020 at 9:23 AM Peter Enderborg
> >>> <peter.enderborg@sony.com> wrote:
> >>>> This adds tracing of all denies. They are grouped with trace_seq for
> >>>> each audit.
> >>>>
> >>>> A filter can be inserted with a write to it's filter section.
> >>>>
> >>>> echo "permission==\"entrypoint\"" > events/avc/selinux_denied/filter
> >>>>
> >>>> A output will be like:
> >>>>           runcon-1046  [002] .N..   156.351738: selinux_denied:
> >>>>           trace_seq=2 result=-13
> >>>>           scontext=system_u:system_r:cupsd_t:s0-s0:c0.
> >>>>           c1023 tcontext=system_u:object_r:bin_t:s0
> >>>>           tclass=file permission=entrypoint
> >>>>
> >>>> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
> >>>> ---
> >>>>  include/trace/events/avc.h | 37 +++++++++++++++++++++++++++++++++++++
> >>>>  security/selinux/avc.c     | 27 +++++++++++++++++++++++++--
> >>>>  2 files changed, 62 insertions(+), 2 deletions(-)
> >>> My most significant comment is that I don't think we want, or need,
> >>> two trace points in the avc_audit_post_callback() function.  Yes, I
> >>> understand they are triggered slightly differently, but from my
> >>> perspective there isn't enough difference between the two tracepoints
> >>> to warrant including both.  However, while the tracepoints may be
> >> We tried that but that was problematic too.
> > My apologies if I was on that thread, but can you remind me why it was
> > a problem?  Why can't we use a single tracepoint to capture the AVC
> > information?
>
> The problem is parsing the event.
>
> https://lkml.org/lkml/2020/8/18/842
>
> https://lkml.org/lkml/2020/8/21/526
>
> and the "single list" version
>
> https://lkml.org/lkml/2020/8/17/1346
>
> With this patch we follow standard message format so no plugin should be needed.

I'm evidently missing something very fundamental (likely), and/or I'm
just not communicating very clearly (also likely), because the above
links don't appear to make any sense with respect to my question.

Let me try a reset ... Why can't we basically take the
"selinux_denied" TRACE_EVENT implementation in your patch and use it
to replace the "selinux_audited" TRACE_EVENT in the selinux/next tree
(of course with the necessary changes to the AVC callback code)?

If the "selinux_denied" implementation is valid from a tracing point
of view, why can we not do this?  Of course if the "selinux_denied"
implementation is not a valid TRACE_EVENT then I'm not sure why this
was suggested for SELinux :)

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH] selinux: Add denied trace with permssion filter

[peter enderborg]

On 8/27/20 3:30 PM, Paul Moore wrote:
> On Wed, Aug 26, 2020 at 11:06 AM peter enderborg
> <peter.enderborg@sony.com> wrote:
>> On 8/26/20 4:45 PM, Paul Moore wrote:
>>> On Wed, Aug 26, 2020 at 10:34 AM peter enderborg
>>> <peter.enderborg@sony.com> wrote:
>>>> On 8/26/20 3:42 PM, Paul Moore wrote:
>>>>> On Mon, Aug 24, 2020 at 9:23 AM Peter Enderborg
>>>>> <peter.enderborg@sony.com> wrote:
>>>>>> This adds tracing of all denies. They are grouped with trace_seq for
>>>>>> each audit.
>>>>>>
>>>>>> A filter can be inserted with a write to it's filter section.
>>>>>>
>>>>>> echo "permission==\"entrypoint\"" > events/avc/selinux_denied/filter
>>>>>>
>>>>>> A output will be like:
>>>>>>           runcon-1046  [002] .N..   156.351738: selinux_denied:
>>>>>>           trace_seq=2 result=-13
>>>>>>           scontext=system_u:system_r:cupsd_t:s0-s0:c0.
>>>>>>           c1023 tcontext=system_u:object_r:bin_t:s0
>>>>>>           tclass=file permission=entrypoint
>>>>>>
>>>>>> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
>>>>>> ---
>>>>>>  include/trace/events/avc.h | 37 +++++++++++++++++++++++++++++++++++++
>>>>>>  security/selinux/avc.c     | 27 +++++++++++++++++++++++++--
>>>>>>  2 files changed, 62 insertions(+), 2 deletions(-)
>>>>> My most significant comment is that I don't think we want, or need,
>>>>> two trace points in the avc_audit_post_callback() function.  Yes, I
>>>>> understand they are triggered slightly differently, but from my
>>>>> perspective there isn't enough difference between the two tracepoints
>>>>> to warrant including both.  However, while the tracepoints may be
>>>> We tried that but that was problematic too.
>>> My apologies if I was on that thread, but can you remind me why it was
>>> a problem?  Why can't we use a single tracepoint to capture the AVC
>>> information?
>> The problem is parsing the event.
>>
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_18_842&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=iS3eZr3TFrN5I7BbnvPFYOKd6DfW1FHTFcwI7joS_fk&e= 
>>
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_21_526&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=9OsLN0Y5mUWxEAAqUE6K4PS57Pn1XyZz7GXak6uc_Ls&e= 
>>
>> and the "single list" version
>>
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_17_1346&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=tWSY2ry2IT6RcT5BIUwMuqBL_yPObDE1VljbLqI1zrA&e= 
>>
>> With this patch we follow standard message format so no plugin should be needed.
> I'm evidently missing something very fundamental (likely), and/or I'm
> just not communicating very clearly (also likely), because the above
> links don't appear to make any sense with respect to my question.
>
> Let me try a reset ... Why can't we basically take the
> "selinux_denied" TRACE_EVENT implementation in your patch and use it
> to replace the "selinux_audited" TRACE_EVENT in the selinux/next tree
> (of course with the necessary changes to the AVC callback code)?
>
> If the "selinux_denied" implementation is valid from a tracing point
> of view, why can we not do this?  Of course if the "selinux_denied"
> implementation is not a valid TRACE_EVENT then I'm not sure why this
> was suggested for SELinux :)
>
Im happly fine with replacing the selinux_audited with selinux_denied.  However it is the case where there are more than one denial at the same time. Im not sure how and when it might happen.
When that happen we got more than one event. I have no problems with that, but im not sure if the debug tools and perf can make sense of that.

A other feature with the selinux_audited event it might be inserted on other places in the code too.  A denial is sort of final.

Re: [RFC PATCH] selinux: Add denied trace with permssion filter

[Paul Moore]

On Thu, Aug 27, 2020 at 10:04 AM peter enderborg
<peter.enderborg@sony.com> wrote:
>
> On 8/27/20 3:30 PM, Paul Moore wrote:
> > On Wed, Aug 26, 2020 at 11:06 AM peter enderborg
> > <peter.enderborg@sony.com> wrote:
> >> On 8/26/20 4:45 PM, Paul Moore wrote:
> >>> On Wed, Aug 26, 2020 at 10:34 AM peter enderborg
> >>> <peter.enderborg@sony.com> wrote:
> >>>> On 8/26/20 3:42 PM, Paul Moore wrote:
> >>>>> On Mon, Aug 24, 2020 at 9:23 AM Peter Enderborg
> >>>>> <peter.enderborg@sony.com> wrote:
> >>>>>> This adds tracing of all denies. They are grouped with trace_seq for
> >>>>>> each audit.
> >>>>>>
> >>>>>> A filter can be inserted with a write to it's filter section.
> >>>>>>
> >>>>>> echo "permission==\"entrypoint\"" > events/avc/selinux_denied/filter
> >>>>>>
> >>>>>> A output will be like:
> >>>>>>           runcon-1046  [002] .N..   156.351738: selinux_denied:
> >>>>>>           trace_seq=2 result=-13
> >>>>>>           scontext=system_u:system_r:cupsd_t:s0-s0:c0.
> >>>>>>           c1023 tcontext=system_u:object_r:bin_t:s0
> >>>>>>           tclass=file permission=entrypoint
> >>>>>>
> >>>>>> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
> >>>>>> ---
> >>>>>>  include/trace/events/avc.h | 37 +++++++++++++++++++++++++++++++++++++
> >>>>>>  security/selinux/avc.c     | 27 +++++++++++++++++++++++++--
> >>>>>>  2 files changed, 62 insertions(+), 2 deletions(-)
> >>>>> My most significant comment is that I don't think we want, or need,
> >>>>> two trace points in the avc_audit_post_callback() function.  Yes, I
> >>>>> understand they are triggered slightly differently, but from my
> >>>>> perspective there isn't enough difference between the two tracepoints
> >>>>> to warrant including both.  However, while the tracepoints may be
> >>>> We tried that but that was problematic too.
> >>> My apologies if I was on that thread, but can you remind me why it was
> >>> a problem?  Why can't we use a single tracepoint to capture the AVC
> >>> information?
> >> The problem is parsing the event.
> >>
> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_18_842&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=iS3eZr3TFrN5I7BbnvPFYOKd6DfW1FHTFcwI7joS_fk&e=
> >>
> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_21_526&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=9OsLN0Y5mUWxEAAqUE6K4PS57Pn1XyZz7GXak6uc_Ls&e=
> >>
> >> and the "single list" version
> >>
> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_17_1346&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=tWSY2ry2IT6RcT5BIUwMuqBL_yPObDE1VljbLqI1zrA&e=
> >>
> >> With this patch we follow standard message format so no plugin should be needed.
> > I'm evidently missing something very fundamental (likely), and/or I'm
> > just not communicating very clearly (also likely), because the above
> > links don't appear to make any sense with respect to my question.
> >
> > Let me try a reset ... Why can't we basically take the
> > "selinux_denied" TRACE_EVENT implementation in your patch and use it
> > to replace the "selinux_audited" TRACE_EVENT in the selinux/next tree
> > (of course with the necessary changes to the AVC callback code)?
> >
> > If the "selinux_denied" implementation is valid from a tracing point
> > of view, why can we not do this?  Of course if the "selinux_denied"
> > implementation is not a valid TRACE_EVENT then I'm not sure why this
> > was suggested for SELinux :)
>
> Im happly fine with replacing the selinux_audited with selinux_denied.  However it is the case where there are more than one denial at the same time. Im not sure how and when it might happen.

One thing I wondered about was why not build up a single string with
all of the permissions instead of generating multiple trace events?
In the previous discussion it was implied that this was due to
limitations in the tracing subsystem's filtering, and based on the
discussion thus far I'm guessing there is little desire for this
information if it can't be filtered on?

If that's the case then I think we are stuck with the tracing code
that currently lives in selinux/next, as I currently have little
desire to add more than one tracepoint in the SELinux permission
checking codepath.

> When that happen we got more than one event. I have no problems with that, but im not sure if the debug tools and perf can make sense of that.
>
> A other feature with the selinux_audited event it might be inserted on other places in the code too.  A denial is sort of final.

-- 
paul moore
www.paul-moore.com

RE: [RFC PATCH] selinux: Add denied trace with permssion filter

[Robert Judy]

I would like to unsubscribe from this group. I have sent "unsubscribe" requests to selinux-owner@vger.kernel.org as subject and in the body of the e-mail but that has not worked. Please advise me how to unsubscribe.

Thank you and respectfully,

rmj

-----Original Message-----
From: selinux-owner@vger.kernel.org <selinux-owner@vger.kernel.org> On Behalf Of Paul Moore
Sent: Monday, August 31, 2020 9:16 AM
To: peter enderborg <peter.enderborg@sony.com>
Cc: linux-kernel@vger.kernel.org; SElinux list <selinux@vger.kernel.org>; Steven Rostedt <rostedt@goodmis.org>; Stephen Smalley <stephen.smalley.work@gmail.com>
Subject: Re: [RFC PATCH] selinux: Add denied trace with permssion filter

On Thu, Aug 27, 2020 at 10:04 AM peter enderborg <peter.enderborg@sony.com> wrote:
>
> On 8/27/20 3:30 PM, Paul Moore wrote:
> > On Wed, Aug 26, 2020 at 11:06 AM peter enderborg 
> > <peter.enderborg@sony.com> wrote:
> >> On 8/26/20 4:45 PM, Paul Moore wrote:
> >>> On Wed, Aug 26, 2020 at 10:34 AM peter enderborg 
> >>> <peter.enderborg@sony.com> wrote:
> >>>> On 8/26/20 3:42 PM, Paul Moore wrote:
> >>>>> On Mon, Aug 24, 2020 at 9:23 AM Peter Enderborg 
> >>>>> <peter.enderborg@sony.com> wrote:
> >>>>>> This adds tracing of all denies. They are grouped with 
> >>>>>> trace_seq for each audit.
> >>>>>>
> >>>>>> A filter can be inserted with a write to it's filter section.
> >>>>>>
> >>>>>> echo "permission==\"entrypoint\"" > 
> >>>>>> events/avc/selinux_denied/filter
> >>>>>>
> >>>>>> A output will be like:
> >>>>>>           runcon-1046  [002] .N..   156.351738: selinux_denied:
> >>>>>>           trace_seq=2 result=-13
> >>>>>>           scontext=system_u:system_r:cupsd_t:s0-s0:c0.
> >>>>>>           c1023 tcontext=system_u:object_r:bin_t:s0
> >>>>>>           tclass=file permission=entrypoint
> >>>>>>
> >>>>>> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
> >>>>>> ---
> >>>>>>  include/trace/events/avc.h | 37 +++++++++++++++++++++++++++++++++++++
> >>>>>>  security/selinux/avc.c     | 27 +++++++++++++++++++++++++--
> >>>>>>  2 files changed, 62 insertions(+), 2 deletions(-)
> >>>>> My most significant comment is that I don't think we want, or 
> >>>>> need, two trace points in the avc_audit_post_callback() 
> >>>>> function.  Yes, I understand they are triggered slightly 
> >>>>> differently, but from my perspective there isn't enough 
> >>>>> difference between the two tracepoints to warrant including 
> >>>>> both.  However, while the tracepoints may be
> >>>> We tried that but that was problematic too.
> >>> My apologies if I was on that thread, but can you remind me why it 
> >>> was a problem?  Why can't we use a single tracepoint to capture 
> >>> the AVC information?
> >> The problem is parsing the event.
> >>
> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_
> >> 2020_8_18_842&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4c
> >> c&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhH
> >> kpb71j1YoicydLh-7l4cOsLYcY&s=iS3eZr3TFrN5I7BbnvPFYOKd6DfW1FHTFcwI7j
> >> oS_fk&e=
> >>
> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_
> >> 2020_8_21_526&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4c
> >> c&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhH
> >> kpb71j1YoicydLh-7l4cOsLYcY&s=9OsLN0Y5mUWxEAAqUE6K4PS57Pn1XyZz7GXak6
> >> uc_Ls&e=
> >>
> >> and the "single list" version
> >>
> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_
> >> 2020_8_17_1346&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4
> >> cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLh
> >> Hkpb71j1YoicydLh-7l4cOsLYcY&s=tWSY2ry2IT6RcT5BIUwMuqBL_yPObDE1VljbL
> >> qI1zrA&e=
> >>
> >> With this patch we follow standard message format so no plugin should be needed.
> > I'm evidently missing something very fundamental (likely), and/or 
> > I'm just not communicating very clearly (also likely), because the 
> > above links don't appear to make any sense with respect to my question.
> >
> > Let me try a reset ... Why can't we basically take the 
> > "selinux_denied" TRACE_EVENT implementation in your patch and use it 
> > to replace the "selinux_audited" TRACE_EVENT in the selinux/next 
> > tree (of course with the necessary changes to the AVC callback code)?
> >
> > If the "selinux_denied" implementation is valid from a tracing point 
> > of view, why can we not do this?  Of course if the "selinux_denied"
> > implementation is not a valid TRACE_EVENT then I'm not sure why this 
> > was suggested for SELinux :)
>
> Im happly fine with replacing the selinux_audited with selinux_denied.  However it is the case where there are more than one denial at the same time. Im not sure how and when it might happen.

One thing I wondered about was why not build up a single string with all of the permissions instead of generating multiple trace events?
In the previous discussion it was implied that this was due to limitations in the tracing subsystem's filtering, and based on the discussion thus far I'm guessing there is little desire for this information if it can't be filtered on?

If that's the case then I think we are stuck with the tracing code that currently lives in selinux/next, as I currently have little desire to add more than one tracepoint in the SELinux permission checking codepath.

> When that happen we got more than one event. I have no problems with that, but im not sure if the debug tools and perf can make sense of that.
>
> A other feature with the selinux_audited event it might be inserted on other places in the code too.  A denial is sort of final.

--
paul moore
www.paul-moore.com

Re: [RFC PATCH] selinux: Add denied trace with permssion filter

[Paul Moore]

On Mon, Aug 31, 2020 at 10:19 AM Robert Judy <rjudy@sfasu.edu> wrote:
>
> I would like to unsubscribe from this group. I have sent "unsubscribe" requests to selinux-owner@vger.kernel.org as subject and in the body of the e-mail but that has not worked. Please advise me how to unsubscribe.
>
> Thank you and respectfully,

When you subscribed to the mailing list you were sent instructions on
how to unsubscribe from the mailing list.

The SELinux developer's mailing list is managed via
majordomo@vger.kernel.org, in order to unsubscribe you need to send an
email to that address with "unsubscribe selinux" in the body of the
email.

> -----Original Message-----
> From: selinux-owner@vger.kernel.org <selinux-owner@vger.kernel.org> On Behalf Of Paul Moore
> Sent: Monday, August 31, 2020 9:16 AM
> To: peter enderborg <peter.enderborg@sony.com>
> Cc: linux-kernel@vger.kernel.org; SElinux list <selinux@vger.kernel.org>; Steven Rostedt <rostedt@goodmis.org>; Stephen Smalley <stephen.smalley.work@gmail.com>
> Subject: Re: [RFC PATCH] selinux: Add denied trace with permssion filter
>
> On Thu, Aug 27, 2020 at 10:04 AM peter enderborg <peter.enderborg@sony.com> wrote:
> >
> > On 8/27/20 3:30 PM, Paul Moore wrote:
> > > On Wed, Aug 26, 2020 at 11:06 AM peter enderborg
> > > <peter.enderborg@sony.com> wrote:
> > >> On 8/26/20 4:45 PM, Paul Moore wrote:
> > >>> On Wed, Aug 26, 2020 at 10:34 AM peter enderborg
> > >>> <peter.enderborg@sony.com> wrote:
> > >>>> On 8/26/20 3:42 PM, Paul Moore wrote:
> > >>>>> On Mon, Aug 24, 2020 at 9:23 AM Peter Enderborg
> > >>>>> <peter.enderborg@sony.com> wrote:
> > >>>>>> This adds tracing of all denies. They are grouped with
> > >>>>>> trace_seq for each audit.
> > >>>>>>
> > >>>>>> A filter can be inserted with a write to it's filter section.
> > >>>>>>
> > >>>>>> echo "permission==\"entrypoint\"" >
> > >>>>>> events/avc/selinux_denied/filter
> > >>>>>>
> > >>>>>> A output will be like:
> > >>>>>>           runcon-1046  [002] .N..   156.351738: selinux_denied:
> > >>>>>>           trace_seq=2 result=-13
> > >>>>>>           scontext=system_u:system_r:cupsd_t:s0-s0:c0.
> > >>>>>>           c1023 tcontext=system_u:object_r:bin_t:s0
> > >>>>>>           tclass=file permission=entrypoint
> > >>>>>>
> > >>>>>> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
> > >>>>>> ---
> > >>>>>>  include/trace/events/avc.h | 37 +++++++++++++++++++++++++++++++++++++
> > >>>>>>  security/selinux/avc.c     | 27 +++++++++++++++++++++++++--
> > >>>>>>  2 files changed, 62 insertions(+), 2 deletions(-)
> > >>>>> My most significant comment is that I don't think we want, or
> > >>>>> need, two trace points in the avc_audit_post_callback()
> > >>>>> function.  Yes, I understand they are triggered slightly
> > >>>>> differently, but from my perspective there isn't enough
> > >>>>> difference between the two tracepoints to warrant including
> > >>>>> both.  However, while the tracepoints may be
> > >>>> We tried that but that was problematic too.
> > >>> My apologies if I was on that thread, but can you remind me why it
> > >>> was a problem?  Why can't we use a single tracepoint to capture
> > >>> the AVC information?
> > >> The problem is parsing the event.
> > >>
> > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_
> > >> 2020_8_18_842&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4c
> > >> c&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhH
> > >> kpb71j1YoicydLh-7l4cOsLYcY&s=iS3eZr3TFrN5I7BbnvPFYOKd6DfW1FHTFcwI7j
> > >> oS_fk&e=
> > >>
> > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_
> > >> 2020_8_21_526&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4c
> > >> c&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhH
> > >> kpb71j1YoicydLh-7l4cOsLYcY&s=9OsLN0Y5mUWxEAAqUE6K4PS57Pn1XyZz7GXak6
> > >> uc_Ls&e=
> > >>
> > >> and the "single list" version
> > >>
> > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_
> > >> 2020_8_17_1346&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4
> > >> cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLh
> > >> Hkpb71j1YoicydLh-7l4cOsLYcY&s=tWSY2ry2IT6RcT5BIUwMuqBL_yPObDE1VljbL
> > >> qI1zrA&e=
> > >>
> > >> With this patch we follow standard message format so no plugin should be needed.
> > > I'm evidently missing something very fundamental (likely), and/or
> > > I'm just not communicating very clearly (also likely), because the
> > > above links don't appear to make any sense with respect to my question.
> > >
> > > Let me try a reset ... Why can't we basically take the
> > > "selinux_denied" TRACE_EVENT implementation in your patch and use it
> > > to replace the "selinux_audited" TRACE_EVENT in the selinux/next
> > > tree (of course with the necessary changes to the AVC callback code)?
> > >
> > > If the "selinux_denied" implementation is valid from a tracing point
> > > of view, why can we not do this?  Of course if the "selinux_denied"
> > > implementation is not a valid TRACE_EVENT then I'm not sure why this
> > > was suggested for SELinux :)
> >
> > Im happly fine with replacing the selinux_audited with selinux_denied.  However it is the case where there are more than one denial at the same time. Im not sure how and when it might happen.
>
> One thing I wondered about was why not build up a single string with all of the permissions instead of generating multiple trace events?
> In the previous discussion it was implied that this was due to limitations in the tracing subsystem's filtering, and based on the discussion thus far I'm guessing there is little desire for this information if it can't be filtered on?
>
> If that's the case then I think we are stuck with the tracing code that currently lives in selinux/next, as I currently have little desire to add more than one tracepoint in the SELinux permission checking codepath.
>
> > When that happen we got more than one event. I have no problems with that, but im not sure if the debug tools and perf can make sense of that.
> >
> > A other feature with the selinux_audited event it might be inserted on other places in the code too.  A denial is sort of final.
>
> --
> paul moore
> www.paul-moore.com



-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH] selinux: Add denied trace with permssion filter

[peter enderborg]

On 8/31/20 4:16 PM, Paul Moore wrote:
> On Thu, Aug 27, 2020 at 10:04 AM peter enderborg
> <peter.enderborg@sony.com> wrote:
>> On 8/27/20 3:30 PM, Paul Moore wrote:
>>> On Wed, Aug 26, 2020 at 11:06 AM peter enderborg
>>> <peter.enderborg@sony.com> wrote:
>>>> On 8/26/20 4:45 PM, Paul Moore wrote:
>>>>> On Wed, Aug 26, 2020 at 10:34 AM peter enderborg
>>>>> <peter.enderborg@sony.com> wrote:
>>>>>> On 8/26/20 3:42 PM, Paul Moore wrote:
>>>>>>> On Mon, Aug 24, 2020 at 9:23 AM Peter Enderborg
>>>>>>> <peter.enderborg@sony.com> wrote:
>>>>>>>> This adds tracing of all denies. They are grouped with trace_seq for
>>>>>>>> each audit.
>>>>>>>>
>>>>>>>> A filter can be inserted with a write to it's filter section.
>>>>>>>>
>>>>>>>> echo "permission==\"entrypoint\"" > events/avc/selinux_denied/filter
>>>>>>>>
>>>>>>>> A output will be like:
>>>>>>>>           runcon-1046  [002] .N..   156.351738: selinux_denied:
>>>>>>>>           trace_seq=2 result=-13
>>>>>>>>           scontext=system_u:system_r:cupsd_t:s0-s0:c0.
>>>>>>>>           c1023 tcontext=system_u:object_r:bin_t:s0
>>>>>>>>           tclass=file permission=entrypoint
>>>>>>>>
>>>>>>>> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
>>>>>>>> ---
>>>>>>>>  include/trace/events/avc.h | 37 +++++++++++++++++++++++++++++++++++++
>>>>>>>>  security/selinux/avc.c     | 27 +++++++++++++++++++++++++--
>>>>>>>>  2 files changed, 62 insertions(+), 2 deletions(-)
>>>>>>> My most significant comment is that I don't think we want, or need,
>>>>>>> two trace points in the avc_audit_post_callback() function.  Yes, I
>>>>>>> understand they are triggered slightly differently, but from my
>>>>>>> perspective there isn't enough difference between the two tracepoints
>>>>>>> to warrant including both.  However, while the tracepoints may be
>>>>>> We tried that but that was problematic too.
>>>>> My apologies if I was on that thread, but can you remind me why it was
>>>>> a problem?  Why can't we use a single tracepoint to capture the AVC
>>>>> information?
>>>> The problem is parsing the event.
>>>>
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_18_842&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=iS3eZr3TFrN5I7BbnvPFYOKd6DfW1FHTFcwI7joS_fk&e=
>>>>
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_21_526&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=9OsLN0Y5mUWxEAAqUE6K4PS57Pn1XyZz7GXak6uc_Ls&e=
>>>>
>>>> and the "single list" version
>>>>
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_17_1346&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=tWSY2ry2IT6RcT5BIUwMuqBL_yPObDE1VljbLqI1zrA&e=
>>>>
>>>> With this patch we follow standard message format so no plugin should be needed.
>>> I'm evidently missing something very fundamental (likely), and/or I'm
>>> just not communicating very clearly (also likely), because the above
>>> links don't appear to make any sense with respect to my question.
>>>
>>> Let me try a reset ... Why can't we basically take the
>>> "selinux_denied" TRACE_EVENT implementation in your patch and use it
>>> to replace the "selinux_audited" TRACE_EVENT in the selinux/next tree
>>> (of course with the necessary changes to the AVC callback code)?
>>>
>>> If the "selinux_denied" implementation is valid from a tracing point
>>> of view, why can we not do this?  Of course if the "selinux_denied"
>>> implementation is not a valid TRACE_EVENT then I'm not sure why this
>>> was suggested for SELinux :)
>> Im happly fine with replacing the selinux_audited with selinux_denied.  However it is the case where there are more than one denial at the same time. Im not sure how and when it might happen.
> One thing I wondered about was why not build up a single string with
> all of the permissions instead of generating multiple trace events?
> In the previous discussion it was implied that this was due to
> limitations in the tracing subsystem's filtering, and based on the
> discussion thus far I'm guessing there is little desire for this
> information if it can't be filtered on?

The information is of course as essential as for audit messages.
I dont see much of the problem with having as the first suggestion with
a list. It works fine for trace_pipe. It is not failing due to that we can not
filter with that. It is cause in other tools in user-space
that needs a plugin to parse it. It need static
mapping for something that is not really static. Not in runtime, and it will
change over time.

A other idea based on the first one is to have multiple pairs like

class=file permission=read permission=write permission=open

but then you need to filter on numeric values that are not static and
I don't know if library can make anything useful from that.

I don't see why it should be a issue with a event for each denial, all
of the trace system is opt-in. It is usually only a NOP instruction,
but here  it is a conditional branch and it is in the end of long
process where of a very tiny percent a ending up as denial.

From my view it is more annoying that we do similar things for
audit_log but not equal enough to be shared.


>
> If that's the case then I think we are stuck with the tracing code
> that currently lives in selinux/next, as I currently have little
> desire to add more than one tracepoint in the SELinux permission
> checking codepath.
>
>> When that happen we got more than one event. I have no problems with that, but im not sure if the debug tools and perf can make sense of that.
>>
>> A other feature with the selinux_audited event it might be inserted on other places in the code too.  A denial is sort of final.

Re: [RFC PATCH] selinux: Add denied trace with permssion filter

[Paul Moore]

On Mon, Aug 31, 2020 at 11:34 AM peter enderborg wrote:
> On 8/31/20 4:16 PM, Paul Moore wrote:
> > On Thu, Aug 27, 2020 at 10:04 AM peter enderborg wrote:

...

> >> Im happly fine with replacing the selinux_audited with selinux_denied.  However it is the case where there are more than one denial at the same time. Im not sure how and when it might happen.
> > One thing I wondered about was why not build up a single string with
> > all of the permissions instead of generating multiple trace events?
> > In the previous discussion it was implied that this was due to
> > limitations in the tracing subsystem's filtering, and based on the
> > discussion thus far I'm guessing there is little desire for this
> > information if it can't be filtered on?
>
> The information is of course as essential as for audit messages.
> I dont see much of the problem with having as the first suggestion with
> a list. It works fine for trace_pipe. It is not failing due to that we can not
> filter with that.

I don't really have much personal experience with the kernel tracing
tools, so an example would be helpful as I'm not really following what
you are saying.  Are you talking about something like
"permission=foo,bar,baz"?

> It is cause in other tools in user-space
> that needs a plugin to parse it. It need static
> mapping for something that is not really static. Not in runtime, and it will
> change over time.

I think we've all come to the conclusion that doing the permission
bitmap-to-string translation in a plugin is not really desirable.

> A other idea based on the first one is to have multiple pairs like
>
> class=file permission=read permission=write permission=open
>
> but then you need to filter on numeric values that are not static and
> I don't know if library can make anything useful from that.

Oh, wait, is the issue that the tracing subsystem can't filter on
strings?  That doesn't seem right, but I can understand why one might
want to avoid that for performance reasons.  If the tracing subsystem
*can* filter on strings, why did you say that in the "perm=foo
perm=bar" format one would need to filter on numeric values?  I still
think I'm missing something here ...

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH] selinux: Add denied trace with permssion filter

[peter enderborg]

On 9/1/20 5:31 PM, Paul Moore wrote:
> On Mon, Aug 31, 2020 at 11:34 AM peter enderborg wrote:
>> On 8/31/20 4:16 PM, Paul Moore wrote:
>>> On Thu, Aug 27, 2020 at 10:04 AM peter enderborg wrote:
> ...
>
>>>> Im happly fine with replacing the selinux_audited with selinux_denied.  However it is the case where there are more than one denial at the same time. Im not sure how and when it might happen.
>>> One thing I wondered about was why not build up a single string with
>>> all of the permissions instead of generating multiple trace events?
>>> In the previous discussion it was implied that this was due to
>>> limitations in the tracing subsystem's filtering, and based on the
>>> discussion thus far I'm guessing there is little desire for this
>>> information if it can't be filtered on?
>> The information is of course as essential as for audit messages.
>> I dont see much of the problem with having as the first suggestion with
>> a list. It works fine for trace_pipe. It is not failing due to that we can not
>> filter with that.
> I don't really have much personal experience with the kernel tracing
> tools, so an example would be helpful as I'm not really following what
> you are saying.  Are you talking about something like
> "permission=foo,bar,baz"?

In the first patch we hade premission that was in the format:

permission={read !write open}

That would have work fine if it was not for that the permission can be created
dynamically.  It would be very easy to change that to

permission=read permission=!write permission=open

But I think that will cause problems too. The create a format specifiaction
in the trace tree, and I dont think we can describe this format.


>
>> It is cause in other tools in user-space
>> that needs a plugin to parse it. It need static
>> mapping for something that is not really static. Not in runtime, and it will
>> change over time.
> I think we've all come to the conclusion that doing the permission
> bitmap-to-string translation in a plugin is not really desirable.

Yes. But Im arguing that we still have the same information,
but it will "cost" that we have more events. It is a fair trade-off,
it is usually a lot better to have the trace simple than
flexible and let it cost events when needed.


>> A other idea based on the first one is to have multiple pairs like
>>
>> class=file permission=read permission=write permission=open
>>
>> but then you need to filter on numeric values that are not static and
>> I don't know if library can make anything useful from that.
> Oh, wait, is the issue that the tracing subsystem can't filter on
> strings?  That doesn't seem right, but I can understand why one might
> want to avoid that for performance reasons.  If the tracing subsystem
> *can* filter on strings, why did you say that in the "perm=foo
> perm=bar" format one would need to filter on numeric values?  I still
> think I'm missing something here ...
>
No. It can filter on strings. But it can not do any fuzzy matching.
They are equal not not equal. So if you have a parameter value
that is { open read !write } you need to specify a exact match.

With multiple events you match for "open" or "write" not "{ open read !write }".

So you get one event for "open" and a other event for "write".

The numeric values is not matching perm=, it is a match for the
bits in the denied= parameter that is present within selinux_audited event.

Re: [RFC PATCH] selinux: Add denied trace with permssion filter

[Steven Rostedt]

 [ Late reply due to long vacation followed by drowning in the email
   built up from said vacation! ]

On Tue, 1 Sep 2020 19:18:46 +0200
peter enderborg <peter.enderborg@sony.com> wrote:

> No. It can filter on strings. But it can not do any fuzzy matching.
> They are equal not not equal. So if you have a parameter value
> that is { open read !write } you need to specify a exact match.

That is not actually true.

It allows globing in filters.

 # trace-cmd start -e sched_switch -f 'next_comm ~ "c*"'
 # cat /etc/passwd
 # trace-cmd show
# tracer: nop
#
# entries-in-buffer/entries-written: 3/3   #P:8
#
#                              _-----=> irqs-off
#                             / _----=> need-resched
#                            | / _---=> hardirq/softirq
#                            || / _--=> preempt-depth
#                            ||| /     delay
#           TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
#              | |       |   ||||       |         |
     kworker/2:1-2137  [002] d..2  9263.286132: sched_switch: prev_comm=kworker/2:1 prev_pid=2137 prev_prio=120 prev_state=I ==> next_comm=cat next_pid=2146 next_prio=120
          <idle>-0     [002] d..2  9264.390089: sched_switch: prev_comm=swapper/2 prev_pid=0 prev_prio=120 prev_state=R ==> next_comm=cat next_pid=2146 next_prio=120
     kworker/2:1-2137  [002] d..2  9264.390440: sched_switch: prev_comm=kworker/2:1 prev_pid=2137 prev_prio=120 prev_state=I ==> next_comm=cat next_pid=2146 next_prio=120


Thus you can filter:

 "foo*" - everything that starts with foo
 "*foo" - everything that ends with foo
 "*foo*" - everything that has foo in it.

-- Steve