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