On Fri, Jul 24, 2020 at 9:32 AM Stephen Smalley <stephen.smalley.work@xxxxxxxxx> wrote: > On Fri, Jul 24, 2020 at 5:15 AM Thiébaud Weksteen <tweek@xxxxxxxxxx> 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. > > > > [1] https://source.android.com/devices/tech/debug/native_stack_dump > > > > Signed-off-by: Thiébaud Weksteen <tweek@xxxxxxxxxx> > > Signed-off-by: Joel Fernandes <joelaf@xxxxxxxxxx> > > --- > > MAINTAINERS | 1 + > > include/trace/events/selinux.h | 35 ++++++++++++++++++++++++++++++++++ > > security/selinux/avc.c | 6 ++++++ > > 3 files changed, 42 insertions(+) > > create mode 100644 include/trace/events/selinux.h > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index e64cdde81851..6b6cd5e13537 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -15358,6 +15358,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/selinux.h > > F: include/uapi/linux/selinux_netlink.h > > F: scripts/selinux/ > > F: security/selinux/ > > diff --git a/include/trace/events/selinux.h b/include/trace/events/selinux.h > > new file mode 100644 > > index 000000000000..e247187a8135 > > --- /dev/null > > +++ b/include/trace/events/selinux.h > > @@ -0,0 +1,35 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +#undef TRACE_SYSTEM > > +#define TRACE_SYSTEM selinux > > + > > +#if !defined(_TRACE_SELINUX_H) || defined(TRACE_HEADER_MULTI_READ) > > +#define _TRACE_SELINUX_H > > + > > +#include <linux/ktime.h> > > +#include <linux/tracepoint.h> > > + > > +TRACE_EVENT(selinux_denied, > > + > > + TP_PROTO(int cls, int av), > > + > > + TP_ARGS(cls, av), > > + > > + TP_STRUCT__entry( > > + __field(int, cls) > > + __field(int, av) > > + ), > > + > > + TP_fast_assign( > > + __entry->cls = cls; > > + __entry->av = av; > > + ), > > + > > + TP_printk("denied %d %d", > > + __entry->cls, > > + __entry->av) > > +); > > I would think you would want to log av as %x for easier interpretation > especially when there are multiple permissions being checked at once > (which can happen). Also both cls and av would properly be unsigned > values. Only other question I have is whether it would be beneficial > to include other information here to help uniquely identify/correlate > the denial with the avc: message and whether any decoding of the > class, av, or other information could/should be done here versus in > some userland helper. It does seem like at the very least it would be nice to see the av as hex values instead of integers, e.g. "%x" in the TP_printk() call. Considering this patch is about making dev's lives easier, I tend to agree with Stephen questioning if you should go a step further and convert both the class and av values into string representations. -- paul moore www.paul-moore.com
