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