Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200724091520.880211-1-tweek@google.com> <CAEjxPJ45ij3obT37ywn_edb9xb89z-SdwzejfN6+jrvAtghXfA@mail.gmail.com>
In-Reply-To: <CAEjxPJ45ij3obT37ywn_edb9xb89z-SdwzejfN6+jrvAtghXfA@mail.gmail.com>
From:   Paul Moore <paul@paul-moore.com>
Date:   Fri, 24 Jul 2020 09:54:59 -0400
Message-ID: <CAHC9VhS4aXD8kcXnQ2MsYvjc--xXSUpsM1xtgq3X5DBT59ohhw@mail.gmail.com>
Subject: Re: [PATCH] selinux: add tracepoint on denials
To:     Stephen Smalley <stephen.smalley.work@gmail.com>
Cc:     =?UTF-8?Q?Thi=C3=A9baud_Weksteen?= <tweek@google.com>,
        Nick Kralevich <nnk@google.com>,
        Joel Fernandes <joelaf@google.com>,
        Eric Paris <eparis@parisplace.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Rob Herring <robh@kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        SElinux list <selinux@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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=C3=A9baud 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=C3=A9baud 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/seli=
nux.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 =3D cls;
> > +               __entry->av =3D 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.

--=20
paul moore
www.paul-moore.com