Re: [RFC PATCH] selinux: Add denied trace with permssion filter

[peter enderborg]

On 8/26/20 4:45 PM, Paul Moore wrote:
> On Wed, Aug 26, 2020 at 10:34 AM peter enderborg
> <peter.enderborg@xxxxxxxx> wrote:
>> On 8/26/20 3:42 PM, Paul Moore wrote:
>>> On Mon, Aug 24, 2020 at 9:23 AM Peter Enderborg
>>> <peter.enderborg@xxxxxxxx> 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@xxxxxxxx>
>>>> ---
>>>>  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?

The problem is parsing the event.

https://lkml.org/lkml/2020/8/18/842

https://lkml.org/lkml/2020/8/21/526

and the "single list" version

https://lkml.org/lkml/2020/8/17/1346

With this patch we follow standard message format so no plugin should be needed.


>> 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.
>