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: <CAHC9VhRuvK55JVyHOxckThbRQ7sCwkeZsudwCaBo2f5G4g11VA@mail.gmail.com>
 <20200824132252.31261-1-peter.enderborg@sony.com> <20200824132252.31261-2-peter.enderborg@sony.com>
 <CAHC9VhR8PscKpA5BrgTNj8cq_eQ6svqru6UXidc=v5+Ha+PM7Q@mail.gmail.com> <6cbe5d27-ebb2-70a6-bad4-31c9f310eff2@sony.com>
In-Reply-To: <6cbe5d27-ebb2-70a6-bad4-31c9f310eff2@sony.com>
From:   Paul Moore <paul@paul-moore.com>
Date:   Wed, 26 Aug 2020 10:45:43 -0400
Message-ID: <CAHC9VhRGaE4FwE8iXo_zeAPdimE9ryMR+r4Jcq=ZpF_2aTJxzQ@mail.gmail.com>
Subject: Re: [RFC PATCH] selinux: Add denied trace with permssion filter
To:     peter enderborg <peter.enderborg@sony.com>
Cc:     linux-kernel@vger.kernel.org,
        SElinux list <selinux@vger.kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Stephen Smalley <stephen.smalley.work@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Aug 26, 2020 at 10:34 AM peter enderborg
<peter.enderborg@sony.com> wrote:
> On 8/26/20 3:42 PM, Paul Moore wrote:
> > On Mon, Aug 24, 2020 at 9:23 AM Peter Enderborg
> > <peter.enderborg@sony.com> wrote:
> >> This adds tracing of all denies. They are grouped with trace_seq for
> >> each audit.
> >>
> >> A filter can be inserted with a write to it's filter section.
> >>
> >> echo "permission==\"entrypoint\"" > events/avc/selinux_denied/filter
> >>
> >> A output will be like:
> >>           runcon-1046  [002] .N..   156.351738: selinux_denied:
> >>           trace_seq=2 result=-13
> >>           scontext=system_u:system_r:cupsd_t:s0-s0:c0.
> >>           c1023 tcontext=system_u:object_r:bin_t:s0
> >>           tclass=file permission=entrypoint
> >>
> >> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
> >> ---
> >>  include/trace/events/avc.h | 37 +++++++++++++++++++++++++++++++++++++
> >>  security/selinux/avc.c     | 27 +++++++++++++++++++++++++--
> >>  2 files changed, 62 insertions(+), 2 deletions(-)
> > My most significant comment is that I don't think we want, or need,
> > two trace points in the avc_audit_post_callback() function.  Yes, I
> > understand they are triggered slightly differently, but from my
> > perspective there isn't enough difference between the two tracepoints
> > to warrant including both.  However, while the tracepoints may be
>
> We tried that but that was problematic too.

My apologies if I was on that thread, but can you remind me why it was
a problem?  Why can't we use a single tracepoint to capture the AVC
information?

> Having partly overlapping traces is not unheard off.  Check
> compaction.c where we have a     trace_mm_compaction_begin
> and a more detailed trace_mm_compaction_migratepages.
> (And a  trace_mm_compaction_end)

It may not be unique to SELinux, but that doesn't mean I like it :)

One of my concerns with adding tracepoints is that the code would get
littered with tracepoints; I accepted that it the AVC decision
codepath was an obvious place for one, so we added a tracepoint.
Having two tracepoints here is getting awfully close to my original
fears.

> > redundant in my mind, this new event does do the permission lookup in
> > the kernel so that the contexts/class/permissions are all available as
> > a string which is a good thing.
> >
> > Without going into the details, would the tracing folks be okay with
> > doing something similar with the existing selinux_audited tracepoint?
> > It's extra work in the kernel, but since it would only be triggered
> > when the tracepoint was active it seems bearable to me.
>
> I think the method for expanding lists is what we tried first on
> suggestion from Steven Rostedt.  Maybe we can do a trace_event
> from a TP_prink but that would be recursive.

Wait, why would you be adding a trace event to a trace event, or am I
misunderstanding you?

All I was talking about was adding the permission resolution code to
the already existing SELinux AVC tracepoint.

-- 
paul moore
www.paul-moore.com