Re: [PATCH] selinux: add tracepoint on denials

["Thiébaud Weksteen" <tweek@google.com>]

Thanks for the review! I'll send a new revision of the patch with the
%x formatter and using the TP_CONDITION macro.

On adding further information to the trace event, I would prefer
adding the strict minimum to be able to correlate the event with the
avc message. The reason is that tracevents have a fixed size (see
https://www.kernel.org/doc/Documentation/trace/events.txt). For
instance, we would need to decide on a maximum size for the string
representation of the list of permissions. This would also duplicate
the reporting done in the avc audit event. I'll simply add the pid as
part of the printk, which should be sufficient for the correlation.


On Fri, Jul 24, 2020 at 3:55 PM Paul Moore <paul@paul-moore.com> wrote:
>
> 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