linux-kernel - Re: [PATCH] selinux: add tracepoint on denials

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEjxPJ45ij3obT37ywn_edb9xb89z-SdwzejfN6+jrvAtghXfA@mail.gmail.com>
Date:   Fri, 24 Jul 2020 09:32:46 -0400
From:   Stephen Smalley <stephen.smalley.work@...il.com>
To:     Thiébaud Weksteen <tweek@...gle.com>
Cc:     Paul Moore <paul@...l-moore.com>, Nick Kralevich <nnk@...gle.com>,
        Joel Fernandes <joelaf@...gle.com>,
        Eric Paris <eparis@...isplace.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Rob Herring <robh@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        SElinux list <selinux@...r.kernel.org>
Subject: Re: [PATCH] selinux: add tracepoint on denials

On Fri, Jul 24, 2020 at 5:15 AM Thiébaud Weksteen <tweek@...gle.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@...gle.com>
> Signed-off-by: Joel Fernandes <joelaf@...gle.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.