| Message ID | 20200824132252.31261-2-peter.enderborg@sony.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [RFC] selinux: Add denied trace with permssion filter | expand |
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 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. > diff --git a/include/trace/events/avc.h b/include/trace/events/avc.h > index 94bca8bef8d2..9a28559956de 100644 > --- a/include/trace/events/avc.h > +++ b/include/trace/events/avc.h > @@ -54,6 +54,43 @@ TRACE_EVENT(selinux_audited, > ) > ); > > +TRACE_EVENT(selinux_denied, > + > + TP_PROTO(struct selinux_audit_data *sad, > + char *scontext, > + char *tcontext, > + const char *tclass, > + const char *permission, > + int64_t seq > + ), > + > + TP_ARGS(sad, scontext, tcontext, tclass, permission, seq), > + > + TP_STRUCT__entry( > + __field(int, result) > + __string(scontext, scontext) > + __string(tcontext, tcontext) > + __string(permission, permission) > + __string(tclass, tclass) > + __field(u64, seq) > + ), > + > + TP_fast_assign( > + __entry->result = sad->result; > + __entry->seq = seq; > + __assign_str(tcontext, tcontext); > + __assign_str(scontext, scontext); > + __assign_str(permission, permission); > + __assign_str(tclass, tclass); > + ), > + > + TP_printk("trace_seq=%lld result=%d scontext=%s tcontext=%s tclass=%s permission=%s", > + __entry->seq, __entry->result, __get_str(scontext), __get_str(tcontext), > + __get_str(tclass), __get_str(permission) > + > + ) > +); > + > #endif > > /* This part must be outside protection */ > diff --git a/security/selinux/avc.c b/security/selinux/avc.c > index 1debddfb5af9..ca53baca15e1 100644 > --- a/security/selinux/avc.c > +++ b/security/selinux/avc.c > @@ -92,6 +92,7 @@ struct selinux_avc { > }; > > static struct selinux_avc selinux_avc; > +static atomic64_t trace_seqno; > > void selinux_avc_init(struct selinux_avc **avc) > { > @@ -731,9 +732,31 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a) > tclass = secclass_map[sad->tclass-1].name; > audit_log_format(ab, " tclass=%s", tclass); > > - if (sad->denied) > + if (sad->denied) { > audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1); > - > + if (trace_selinux_denied_enabled()) { > + int i, perm; > + const char **perms; > + u32 denied = sad->denied; > + int64_t seq; > + > + seq = atomic_long_inc_return(&trace_seqno); > + perms = secclass_map[sad->tclass-1].perms; > + i = 0; > + perm = 1; > + while (i < (sizeof(denied) * 8)) { > + if ((perm & denied & sad->requested) && perms[i]) { > + trace_selinux_denied(sad, scontext, tcontext, > + tclass, perms[i], seq); > + denied &= ~perm; > + if (!denied) > + break; > + } > + i++; > + perm <<= 1; > + } > + } > + } > trace_selinux_audited(sad, scontext, tcontext, tclass); > kfree(tcontext); > kfree(scontext); > -- > 2.17.1
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. 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) > 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. >> diff --git a/include/trace/events/avc.h b/include/trace/events/avc.h >> index 94bca8bef8d2..9a28559956de 100644 >> --- a/include/trace/events/avc.h >> +++ b/include/trace/events/avc.h >> @@ -54,6 +54,43 @@ TRACE_EVENT(selinux_audited, >> ) >> ); >> >> +TRACE_EVENT(selinux_denied, >> + >> + TP_PROTO(struct selinux_audit_data *sad, >> + char *scontext, >> + char *tcontext, >> + const char *tclass, >> + const char *permission, >> + int64_t seq >> + ), >> + >> + TP_ARGS(sad, scontext, tcontext, tclass, permission, seq), >> + >> + TP_STRUCT__entry( >> + __field(int, result) >> + __string(scontext, scontext) >> + __string(tcontext, tcontext) >> + __string(permission, permission) >> + __string(tclass, tclass) >> + __field(u64, seq) >> + ), >> + >> + TP_fast_assign( >> + __entry->result = sad->result; >> + __entry->seq = seq; >> + __assign_str(tcontext, tcontext); >> + __assign_str(scontext, scontext); >> + __assign_str(permission, permission); >> + __assign_str(tclass, tclass); >> + ), >> + >> + TP_printk("trace_seq=%lld result=%d scontext=%s tcontext=%s tclass=%s permission=%s", >> + __entry->seq, __entry->result, __get_str(scontext), __get_str(tcontext), >> + __get_str(tclass), __get_str(permission) >> + >> + ) >> +); >> + >> #endif >> >> /* This part must be outside protection */ >> diff --git a/security/selinux/avc.c b/security/selinux/avc.c >> index 1debddfb5af9..ca53baca15e1 100644 >> --- a/security/selinux/avc.c >> +++ b/security/selinux/avc.c >> @@ -92,6 +92,7 @@ struct selinux_avc { >> }; >> >> static struct selinux_avc selinux_avc; >> +static atomic64_t trace_seqno; >> >> void selinux_avc_init(struct selinux_avc **avc) >> { >> @@ -731,9 +732,31 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a) >> tclass = secclass_map[sad->tclass-1].name; >> audit_log_format(ab, " tclass=%s", tclass); >> >> - if (sad->denied) >> + if (sad->denied) { >> audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1); >> - >> + if (trace_selinux_denied_enabled()) { >> + int i, perm; >> + const char **perms; >> + u32 denied = sad->denied; >> + int64_t seq; >> + >> + seq = atomic_long_inc_return(&trace_seqno); >> + perms = secclass_map[sad->tclass-1].perms; >> + i = 0; >> + perm = 1; >> + while (i < (sizeof(denied) * 8)) { >> + if ((perm & denied & sad->requested) && perms[i]) { >> + trace_selinux_denied(sad, scontext, tcontext, >> + tclass, perms[i], seq); >> + denied &= ~perm; >> + if (!denied) >> + break; >> + } >> + i++; >> + perm <<= 1; >> + } >> + } >> + } >> trace_selinux_audited(sad, scontext, tcontext, tclass); >> kfree(tcontext); >> kfree(scontext); >> -- >> 2.17.1
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.
On 8/26/20 4:45 PM, Paul Moore wrote: > 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? 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. >
On Wed, Aug 26, 2020 at 11:06 AM peter enderborg <peter.enderborg@sony.com> wrote: > On 8/26/20 4:45 PM, Paul Moore wrote: > > 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? > > 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. I'm evidently missing something very fundamental (likely), and/or I'm just not communicating very clearly (also likely), because the above links don't appear to make any sense with respect to my question. Let me try a reset ... Why can't we basically take the "selinux_denied" TRACE_EVENT implementation in your patch and use it to replace the "selinux_audited" TRACE_EVENT in the selinux/next tree (of course with the necessary changes to the AVC callback code)? If the "selinux_denied" implementation is valid from a tracing point of view, why can we not do this? Of course if the "selinux_denied" implementation is not a valid TRACE_EVENT then I'm not sure why this was suggested for SELinux :)
On 8/27/20 3:30 PM, Paul Moore wrote: > On Wed, Aug 26, 2020 at 11:06 AM peter enderborg > <peter.enderborg@sony.com> wrote: >> On 8/26/20 4:45 PM, Paul Moore wrote: >>> 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? >> The problem is parsing the event. >> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_18_842&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=iS3eZr3TFrN5I7BbnvPFYOKd6DfW1FHTFcwI7joS_fk&e= >> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_21_526&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=9OsLN0Y5mUWxEAAqUE6K4PS57Pn1XyZz7GXak6uc_Ls&e= >> >> and the "single list" version >> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_17_1346&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=tWSY2ry2IT6RcT5BIUwMuqBL_yPObDE1VljbLqI1zrA&e= >> >> With this patch we follow standard message format so no plugin should be needed. > I'm evidently missing something very fundamental (likely), and/or I'm > just not communicating very clearly (also likely), because the above > links don't appear to make any sense with respect to my question. > > Let me try a reset ... Why can't we basically take the > "selinux_denied" TRACE_EVENT implementation in your patch and use it > to replace the "selinux_audited" TRACE_EVENT in the selinux/next tree > (of course with the necessary changes to the AVC callback code)? > > If the "selinux_denied" implementation is valid from a tracing point > of view, why can we not do this? Of course if the "selinux_denied" > implementation is not a valid TRACE_EVENT then I'm not sure why this > was suggested for SELinux :) > Im happly fine with replacing the selinux_audited with selinux_denied. However it is the case where there are more than one denial at the same time. Im not sure how and when it might happen. When that happen we got more than one event. I have no problems with that, but im not sure if the debug tools and perf can make sense of that. A other feature with the selinux_audited event it might be inserted on other places in the code too. A denial is sort of final.
On Thu, Aug 27, 2020 at 10:04 AM peter enderborg <peter.enderborg@sony.com> wrote: > > On 8/27/20 3:30 PM, Paul Moore wrote: > > On Wed, Aug 26, 2020 at 11:06 AM peter enderborg > > <peter.enderborg@sony.com> wrote: > >> On 8/26/20 4:45 PM, Paul Moore wrote: > >>> 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? > >> The problem is parsing the event. > >> > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_18_842&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=iS3eZr3TFrN5I7BbnvPFYOKd6DfW1FHTFcwI7joS_fk&e= > >> > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_21_526&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=9OsLN0Y5mUWxEAAqUE6K4PS57Pn1XyZz7GXak6uc_Ls&e= > >> > >> and the "single list" version > >> > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_17_1346&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=tWSY2ry2IT6RcT5BIUwMuqBL_yPObDE1VljbLqI1zrA&e= > >> > >> With this patch we follow standard message format so no plugin should be needed. > > I'm evidently missing something very fundamental (likely), and/or I'm > > just not communicating very clearly (also likely), because the above > > links don't appear to make any sense with respect to my question. > > > > Let me try a reset ... Why can't we basically take the > > "selinux_denied" TRACE_EVENT implementation in your patch and use it > > to replace the "selinux_audited" TRACE_EVENT in the selinux/next tree > > (of course with the necessary changes to the AVC callback code)? > > > > If the "selinux_denied" implementation is valid from a tracing point > > of view, why can we not do this? Of course if the "selinux_denied" > > implementation is not a valid TRACE_EVENT then I'm not sure why this > > was suggested for SELinux :) > > Im happly fine with replacing the selinux_audited with selinux_denied. However it is the case where there are more than one denial at the same time. Im not sure how and when it might happen. One thing I wondered about was why not build up a single string with all of the permissions instead of generating multiple trace events? In the previous discussion it was implied that this was due to limitations in the tracing subsystem's filtering, and based on the discussion thus far I'm guessing there is little desire for this information if it can't be filtered on? If that's the case then I think we are stuck with the tracing code that currently lives in selinux/next, as I currently have little desire to add more than one tracepoint in the SELinux permission checking codepath. > When that happen we got more than one event. I have no problems with that, but im not sure if the debug tools and perf can make sense of that. > > A other feature with the selinux_audited event it might be inserted on other places in the code too. A denial is sort of final.
I would like to unsubscribe from this group. I have sent "unsubscribe" requests to selinux-owner@vger.kernel.org as subject and in the body of the e-mail but that has not worked. Please advise me how to unsubscribe. Thank you and respectfully, rmj -----Original Message----- From: selinux-owner@vger.kernel.org <selinux-owner@vger.kernel.org> On Behalf Of Paul Moore Sent: Monday, August 31, 2020 9:16 AM 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> Subject: Re: [RFC PATCH] selinux: Add denied trace with permssion filter On Thu, Aug 27, 2020 at 10:04 AM peter enderborg <peter.enderborg@sony.com> wrote: > > On 8/27/20 3:30 PM, Paul Moore wrote: > > On Wed, Aug 26, 2020 at 11:06 AM peter enderborg > > <peter.enderborg@sony.com> wrote: > >> On 8/26/20 4:45 PM, Paul Moore wrote: > >>> 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? > >> The problem is parsing the event. > >> > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_ > >> 2020_8_18_842&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4c > >> c&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhH > >> kpb71j1YoicydLh-7l4cOsLYcY&s=iS3eZr3TFrN5I7BbnvPFYOKd6DfW1FHTFcwI7j > >> oS_fk&e= > >> > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_ > >> 2020_8_21_526&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4c > >> c&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhH > >> kpb71j1YoicydLh-7l4cOsLYcY&s=9OsLN0Y5mUWxEAAqUE6K4PS57Pn1XyZz7GXak6 > >> uc_Ls&e= > >> > >> and the "single list" version > >> > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_ > >> 2020_8_17_1346&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4 > >> cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLh > >> Hkpb71j1YoicydLh-7l4cOsLYcY&s=tWSY2ry2IT6RcT5BIUwMuqBL_yPObDE1VljbL > >> qI1zrA&e= > >> > >> With this patch we follow standard message format so no plugin should be needed. > > I'm evidently missing something very fundamental (likely), and/or > > I'm just not communicating very clearly (also likely), because the > > above links don't appear to make any sense with respect to my question. > > > > Let me try a reset ... Why can't we basically take the > > "selinux_denied" TRACE_EVENT implementation in your patch and use it > > to replace the "selinux_audited" TRACE_EVENT in the selinux/next > > tree (of course with the necessary changes to the AVC callback code)? > > > > If the "selinux_denied" implementation is valid from a tracing point > > of view, why can we not do this? Of course if the "selinux_denied" > > implementation is not a valid TRACE_EVENT then I'm not sure why this > > was suggested for SELinux :) > > Im happly fine with replacing the selinux_audited with selinux_denied. However it is the case where there are more than one denial at the same time. Im not sure how and when it might happen. One thing I wondered about was why not build up a single string with all of the permissions instead of generating multiple trace events? In the previous discussion it was implied that this was due to limitations in the tracing subsystem's filtering, and based on the discussion thus far I'm guessing there is little desire for this information if it can't be filtered on? If that's the case then I think we are stuck with the tracing code that currently lives in selinux/next, as I currently have little desire to add more than one tracepoint in the SELinux permission checking codepath. > When that happen we got more than one event. I have no problems with that, but im not sure if the debug tools and perf can make sense of that. > > A other feature with the selinux_audited event it might be inserted on other places in the code too. A denial is sort of final. -- paul moore www.paul-moore.com
On Mon, Aug 31, 2020 at 10:19 AM Robert Judy <rjudy@sfasu.edu> wrote: > > I would like to unsubscribe from this group. I have sent "unsubscribe" requests to selinux-owner@vger.kernel.org as subject and in the body of the e-mail but that has not worked. Please advise me how to unsubscribe. > > Thank you and respectfully, When you subscribed to the mailing list you were sent instructions on how to unsubscribe from the mailing list. The SELinux developer's mailing list is managed via majordomo@vger.kernel.org, in order to unsubscribe you need to send an email to that address with "unsubscribe selinux" in the body of the email. > -----Original Message----- > From: selinux-owner@vger.kernel.org <selinux-owner@vger.kernel.org> On Behalf Of Paul Moore > Sent: Monday, August 31, 2020 9:16 AM > 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> > Subject: Re: [RFC PATCH] selinux: Add denied trace with permssion filter > > On Thu, Aug 27, 2020 at 10:04 AM peter enderborg <peter.enderborg@sony.com> wrote: > > > > On 8/27/20 3:30 PM, Paul Moore wrote: > > > On Wed, Aug 26, 2020 at 11:06 AM peter enderborg > > > <peter.enderborg@sony.com> wrote: > > >> On 8/26/20 4:45 PM, Paul Moore wrote: > > >>> 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? > > >> The problem is parsing the event. > > >> > > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_ > > >> 2020_8_18_842&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4c > > >> c&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhH > > >> kpb71j1YoicydLh-7l4cOsLYcY&s=iS3eZr3TFrN5I7BbnvPFYOKd6DfW1FHTFcwI7j > > >> oS_fk&e= > > >> > > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_ > > >> 2020_8_21_526&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4c > > >> c&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhH > > >> kpb71j1YoicydLh-7l4cOsLYcY&s=9OsLN0Y5mUWxEAAqUE6K4PS57Pn1XyZz7GXak6 > > >> uc_Ls&e= > > >> > > >> and the "single list" version > > >> > > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_ > > >> 2020_8_17_1346&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4 > > >> cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLh > > >> Hkpb71j1YoicydLh-7l4cOsLYcY&s=tWSY2ry2IT6RcT5BIUwMuqBL_yPObDE1VljbL > > >> qI1zrA&e= > > >> > > >> With this patch we follow standard message format so no plugin should be needed. > > > I'm evidently missing something very fundamental (likely), and/or > > > I'm just not communicating very clearly (also likely), because the > > > above links don't appear to make any sense with respect to my question. > > > > > > Let me try a reset ... Why can't we basically take the > > > "selinux_denied" TRACE_EVENT implementation in your patch and use it > > > to replace the "selinux_audited" TRACE_EVENT in the selinux/next > > > tree (of course with the necessary changes to the AVC callback code)? > > > > > > If the "selinux_denied" implementation is valid from a tracing point > > > of view, why can we not do this? Of course if the "selinux_denied" > > > implementation is not a valid TRACE_EVENT then I'm not sure why this > > > was suggested for SELinux :) > > > > Im happly fine with replacing the selinux_audited with selinux_denied. However it is the case where there are more than one denial at the same time. Im not sure how and when it might happen. > > One thing I wondered about was why not build up a single string with all of the permissions instead of generating multiple trace events? > In the previous discussion it was implied that this was due to limitations in the tracing subsystem's filtering, and based on the discussion thus far I'm guessing there is little desire for this information if it can't be filtered on? > > If that's the case then I think we are stuck with the tracing code that currently lives in selinux/next, as I currently have little desire to add more than one tracepoint in the SELinux permission checking codepath. > > > When that happen we got more than one event. I have no problems with that, but im not sure if the debug tools and perf can make sense of that. > > > > A other feature with the selinux_audited event it might be inserted on other places in the code too. A denial is sort of final. > > -- > paul moore > www.paul-moore.com
On 8/31/20 4:16 PM, Paul Moore wrote: > On Thu, Aug 27, 2020 at 10:04 AM peter enderborg > <peter.enderborg@sony.com> wrote: >> On 8/27/20 3:30 PM, Paul Moore wrote: >>> On Wed, Aug 26, 2020 at 11:06 AM peter enderborg >>> <peter.enderborg@sony.com> wrote: >>>> On 8/26/20 4:45 PM, Paul Moore wrote: >>>>> 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? >>>> The problem is parsing the event. >>>> >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_18_842&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=iS3eZr3TFrN5I7BbnvPFYOKd6DfW1FHTFcwI7joS_fk&e= >>>> >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_21_526&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=9OsLN0Y5mUWxEAAqUE6K4PS57Pn1XyZz7GXak6uc_Ls&e= >>>> >>>> and the "single list" version >>>> >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_17_1346&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=tWSY2ry2IT6RcT5BIUwMuqBL_yPObDE1VljbLqI1zrA&e= >>>> >>>> With this patch we follow standard message format so no plugin should be needed. >>> I'm evidently missing something very fundamental (likely), and/or I'm >>> just not communicating very clearly (also likely), because the above >>> links don't appear to make any sense with respect to my question. >>> >>> Let me try a reset ... Why can't we basically take the >>> "selinux_denied" TRACE_EVENT implementation in your patch and use it >>> to replace the "selinux_audited" TRACE_EVENT in the selinux/next tree >>> (of course with the necessary changes to the AVC callback code)? >>> >>> If the "selinux_denied" implementation is valid from a tracing point >>> of view, why can we not do this? Of course if the "selinux_denied" >>> implementation is not a valid TRACE_EVENT then I'm not sure why this >>> was suggested for SELinux :) >> Im happly fine with replacing the selinux_audited with selinux_denied. However it is the case where there are more than one denial at the same time. Im not sure how and when it might happen. > One thing I wondered about was why not build up a single string with > all of the permissions instead of generating multiple trace events? > In the previous discussion it was implied that this was due to > limitations in the tracing subsystem's filtering, and based on the > discussion thus far I'm guessing there is little desire for this > information if it can't be filtered on? The information is of course as essential as for audit messages. I dont see much of the problem with having as the first suggestion with a list. It works fine for trace_pipe. It is not failing due to that we can not filter with that. It is cause in other tools in user-space that needs a plugin to parse it. It need static mapping for something that is not really static. Not in runtime, and it will change over time. A other idea based on the first one is to have multiple pairs like class=file permission=read permission=write permission=open but then you need to filter on numeric values that are not static and I don't know if library can make anything useful from that. I don't see why it should be a issue with a event for each denial, all of the trace system is opt-in. It is usually only a NOP instruction, but here it is a conditional branch and it is in the end of long process where of a very tiny percent a ending up as denial. From my view it is more annoying that we do similar things for audit_log but not equal enough to be shared. > > If that's the case then I think we are stuck with the tracing code > that currently lives in selinux/next, as I currently have little > desire to add more than one tracepoint in the SELinux permission > checking codepath. > >> When that happen we got more than one event. I have no problems with that, but im not sure if the debug tools and perf can make sense of that. >> >> A other feature with the selinux_audited event it might be inserted on other places in the code too. A denial is sort of final.
On Mon, Aug 31, 2020 at 11:34 AM peter enderborg wrote: > On 8/31/20 4:16 PM, Paul Moore wrote: > > On Thu, Aug 27, 2020 at 10:04 AM peter enderborg wrote: ... > >> Im happly fine with replacing the selinux_audited with selinux_denied. However it is the case where there are more than one denial at the same time. Im not sure how and when it might happen. > > One thing I wondered about was why not build up a single string with > > all of the permissions instead of generating multiple trace events? > > In the previous discussion it was implied that this was due to > > limitations in the tracing subsystem's filtering, and based on the > > discussion thus far I'm guessing there is little desire for this > > information if it can't be filtered on? > > The information is of course as essential as for audit messages. > I dont see much of the problem with having as the first suggestion with > a list. It works fine for trace_pipe. It is not failing due to that we can not > filter with that. I don't really have much personal experience with the kernel tracing tools, so an example would be helpful as I'm not really following what you are saying. Are you talking about something like "permission=foo,bar,baz"? > It is cause in other tools in user-space > that needs a plugin to parse it. It need static > mapping for something that is not really static. Not in runtime, and it will > change over time. I think we've all come to the conclusion that doing the permission bitmap-to-string translation in a plugin is not really desirable. > A other idea based on the first one is to have multiple pairs like > > class=file permission=read permission=write permission=open > > but then you need to filter on numeric values that are not static and > I don't know if library can make anything useful from that. Oh, wait, is the issue that the tracing subsystem can't filter on strings? That doesn't seem right, but I can understand why one might want to avoid that for performance reasons. If the tracing subsystem *can* filter on strings, why did you say that in the "perm=foo perm=bar" format one would need to filter on numeric values? I still think I'm missing something here ...
On 9/1/20 5:31 PM, Paul Moore wrote: > On Mon, Aug 31, 2020 at 11:34 AM peter enderborg wrote: >> On 8/31/20 4:16 PM, Paul Moore wrote: >>> On Thu, Aug 27, 2020 at 10:04 AM peter enderborg wrote: > ... > >>>> Im happly fine with replacing the selinux_audited with selinux_denied. However it is the case where there are more than one denial at the same time. Im not sure how and when it might happen. >>> One thing I wondered about was why not build up a single string with >>> all of the permissions instead of generating multiple trace events? >>> In the previous discussion it was implied that this was due to >>> limitations in the tracing subsystem's filtering, and based on the >>> discussion thus far I'm guessing there is little desire for this >>> information if it can't be filtered on? >> The information is of course as essential as for audit messages. >> I dont see much of the problem with having as the first suggestion with >> a list. It works fine for trace_pipe. It is not failing due to that we can not >> filter with that. > I don't really have much personal experience with the kernel tracing > tools, so an example would be helpful as I'm not really following what > you are saying. Are you talking about something like > "permission=foo,bar,baz"? In the first patch we hade premission that was in the format: permission={read !write open} That would have work fine if it was not for that the permission can be created dynamically. It would be very easy to change that to permission=read permission=!write permission=open But I think that will cause problems too. The create a format specifiaction in the trace tree, and I dont think we can describe this format. > >> It is cause in other tools in user-space >> that needs a plugin to parse it. It need static >> mapping for something that is not really static. Not in runtime, and it will >> change over time. > I think we've all come to the conclusion that doing the permission > bitmap-to-string translation in a plugin is not really desirable. Yes. But Im arguing that we still have the same information, but it will "cost" that we have more events. It is a fair trade-off, it is usually a lot better to have the trace simple than flexible and let it cost events when needed. >> A other idea based on the first one is to have multiple pairs like >> >> class=file permission=read permission=write permission=open >> >> but then you need to filter on numeric values that are not static and >> I don't know if library can make anything useful from that. > Oh, wait, is the issue that the tracing subsystem can't filter on > strings? That doesn't seem right, but I can understand why one might > want to avoid that for performance reasons. If the tracing subsystem > *can* filter on strings, why did you say that in the "perm=foo > perm=bar" format one would need to filter on numeric values? I still > think I'm missing something here ... > No. It can filter on strings. But it can not do any fuzzy matching. They are equal not not equal. So if you have a parameter value that is { open read !write } you need to specify a exact match. With multiple events you match for "open" or "write" not "{ open read !write }". So you get one event for "open" and a other event for "write". The numeric values is not matching perm=, it is a match for the bits in the denied= parameter that is present within selinux_audited event.
[ Late reply due to long vacation followed by drowning in the email built up from said vacation! ] On Tue, 1 Sep 2020 19:18:46 +0200 peter enderborg <peter.enderborg@sony.com> wrote: > No. It can filter on strings. But it can not do any fuzzy matching. > They are equal not not equal. So if you have a parameter value > that is { open read !write } you need to specify a exact match. That is not actually true. It allows globing in filters. # trace-cmd start -e sched_switch -f 'next_comm ~ "c*"' # cat /etc/passwd # trace-cmd show # tracer: nop # # entries-in-buffer/entries-written: 3/3 #P:8 # # _-----=> irqs-off # / _----=> need-resched # | / _---=> hardirq/softirq # || / _--=> preempt-depth # ||| / delay # TASK-PID CPU# |||| TIMESTAMP FUNCTION # | | | |||| | | kworker/2:1-2137 [002] d..2 9263.286132: sched_switch: prev_comm=kworker/2:1 prev_pid=2137 prev_prio=120 prev_state=I ==> next_comm=cat next_pid=2146 next_prio=120 <idle>-0 [002] d..2 9264.390089: sched_switch: prev_comm=swapper/2 prev_pid=0 prev_prio=120 prev_state=R ==> next_comm=cat next_pid=2146 next_prio=120 kworker/2:1-2137 [002] d..2 9264.390440: sched_switch: prev_comm=kworker/2:1 prev_pid=2137 prev_prio=120 prev_state=I ==> next_comm=cat next_pid=2146 next_prio=120 Thus you can filter: "foo*" - everything that starts with foo "*foo" - everything that ends with foo "*foo*" - everything that has foo in it. -- Steve
diff --git a/include/trace/events/avc.h b/include/trace/events/avc.h index 94bca8bef8d2..9a28559956de 100644 --- a/include/trace/events/avc.h +++ b/include/trace/events/avc.h @@ -54,6 +54,43 @@ TRACE_EVENT(selinux_audited, ) ); +TRACE_EVENT(selinux_denied, + + TP_PROTO(struct selinux_audit_data *sad, + char *scontext, + char *tcontext, + const char *tclass, + const char *permission, + int64_t seq + ), + + TP_ARGS(sad, scontext, tcontext, tclass, permission, seq), + + TP_STRUCT__entry( + __field(int, result) + __string(scontext, scontext) + __string(tcontext, tcontext) + __string(permission, permission) + __string(tclass, tclass) + __field(u64, seq) + ), + + TP_fast_assign( + __entry->result = sad->result; + __entry->seq = seq; + __assign_str(tcontext, tcontext); + __assign_str(scontext, scontext); + __assign_str(permission, permission); + __assign_str(tclass, tclass); + ), + + TP_printk("trace_seq=%lld result=%d scontext=%s tcontext=%s tclass=%s permission=%s", + __entry->seq, __entry->result, __get_str(scontext), __get_str(tcontext), + __get_str(tclass), __get_str(permission) + + ) +); + #endif /* This part must be outside protection */ diff --git a/security/selinux/avc.c b/security/selinux/avc.c index 1debddfb5af9..ca53baca15e1 100644 --- a/security/selinux/avc.c +++ b/security/selinux/avc.c @@ -92,6 +92,7 @@ struct selinux_avc { }; static struct selinux_avc selinux_avc; +static atomic64_t trace_seqno; void selinux_avc_init(struct selinux_avc **avc) { @@ -731,9 +732,31 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a) tclass = secclass_map[sad->tclass-1].name; audit_log_format(ab, " tclass=%s", tclass); - if (sad->denied) + if (sad->denied) { audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1); - + if (trace_selinux_denied_enabled()) { + int i, perm; + const char **perms; + u32 denied = sad->denied; + int64_t seq; + + seq = atomic_long_inc_return(&trace_seqno); + perms = secclass_map[sad->tclass-1].perms; + i = 0; + perm = 1; + while (i < (sizeof(denied) * 8)) { + if ((perm & denied & sad->requested) && perms[i]) { + trace_selinux_denied(sad, scontext, tcontext, + tclass, perms[i], seq); + denied &= ~perm; + if (!denied) + break; + } + i++; + perm <<= 1; + } + } + } trace_selinux_audited(sad, scontext, tcontext, tclass); kfree(tcontext); kfree(scontext);
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(-)