Re: [PATCH] selinux: add tracepoint on denials

[Paul Moore <paul@paul-moore.com>]

On Fri, Jul 24, 2020 at 9:32 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Fri, Jul 24, 2020 at 5:15 AM Thiébaud Weksteen <tweek@google.com> 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@google.com>
> > Signed-off-by: Joel Fernandes <joelaf@google.com>
> > ---
> >  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