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> <20200724095232.5f9d3f17@oasis.local.home>
 <80a23580-5067-93b0-53fa-3bd53253c056@sony.com>
In-Reply-To: <80a23580-5067-93b0-53fa-3bd53253c056@sony.com>
From:   =?UTF-8?Q?Thi=C3=A9baud_Weksteen?= <tweek@google.com>
Date:   Fri, 31 Jul 2020 13:07:50 +0200
Message-ID: <CA+zpnLd+bTbhiVutj=DpfTHkJFsXqodu+PekqTPDcBB+UKsoaw@mail.gmail.com>
Subject: Re: [PATCH] RFC: selinux avc trace
To:     peter enderborg <peter.enderborg@sony.com>
Cc:     Steven Rostedt <rostedt@goodmis.org>,
        Paul Moore <paul@paul-moore.com>,
        Nick Kralevich <nnk@google.com>,
        Joel Fernandes <joelaf@google.com>,
        Stephen Smalley <stephen.smalley.work@gmail.com>,
        Eric Paris <eparis@parisplace.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"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Thanks Peter, this looks like a great start.

> Perhaps the two of you could work together to come up with a common
tracepoint that addresses both needs.

Agreed.

> 1 Filtering. Types goes to trace so we can put up a filter for contexts or type etc.

That's right. I think this is the main reason why we would need
further attributes in the trace event.

> 2 It tries also to cover non denies.  And upon that you should be able to do coverage tools.
> I think many systems have a lot more rules that what is needed, but there is good way
> to find out what.  A other way us to make a stat page for the rules, but this way connect to
> userspace and can be used for test cases.

This is a great idea too.

>> On the one hand, we don't need/want to duplicate the avc message
>> itself; we just need enough to be able to correlate them.
>> With respect to non-denials, SELinux auditallow statements can be used
>> to generate avc: granted messages that can be used to support coverage
>> tools although you can easily flood the logs that way.  One other
> That is one reason to use trace.

Yes, that's right. I don't have any concern about the flooding here.
As Peter mentioned, trace is specially designed for this purpose.

On the patch, few things to note:

> ---
> +#include <linux/tracepoint.h>
> +TRACE_EVENT(avc_data,
> +        TP_PROTO(u32 requested,
> +             u32 denied,
> +             u32 audited,
> +             int result,
> +             const char *msg
> +             ),

I would not store the raw msg from avc. As we discussed, it is useful
to be able to match against the values we are seeing in the avc denial
message but these attributes should be simple (as opposed to
composite) so the filtering can easily be setup (see section 5.1 in
https://www.kernel.org/doc/Documentation/trace/events.txt). It makes
more sense extracting scontext and tcontext (for instance) which
allows for a precise filtering setup.

Here, I would also pass down the "struct selinux_audit_data" to avoid
a large list of arguments.

> +TRACE_EVENT(avc_req,
> +        TP_PROTO(u32 requested,
> +             u32 denied,
> +             u32 audited,
> +             int result,
> +             const char *msg,
> +             u32 ssid,
> +             struct selinux_state *state
> +             ),

I don't see that event being used later on. What was the intention here?

> +static int avc_dump_querys(struct selinux_state *state, char *ab, u32 ssid, u32 tsid, u16 tclass)
> +{
> +    int rc;
> +    char *scontext;
> +    u32 scontext_len;
> +    int rp;
> +
> +    rc = security_sid_to_context(state,ssid, &scontext, &scontext_len);
> +    if (rc)
> +        rp = sprintf(ab, "ssid=%d", ssid);
> +    else {
> +        rp = sprintf(ab, "scontext=%s", scontext);
> +        kfree(scontext);
> +    }
> +
> +    rc = security_sid_to_context(state, tsid, &scontext, &scontext_len);
> +    if (rc)
> +        rp +=sprintf(ab+rp, " tsid=%d", tsid);
> +    else {
> +        rp +=sprintf(ab+rp, " tcontext=%s", scontext);
> +        kfree(scontext);
> +    }
> +
> +    BUG_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map));
> +    rp += sprintf(ab+rp, " tclass=%s", secclass_map[tclass-1].name);
> +    return rp;
> +}

As I mentioned before, this is literally repeating the avc audit
message. We are better off storing the exact fields we are interested
in, so that the filtering is precise.

Thanks