diff mbox series

[RFC] selinux: Add denied trace with permssion filter

Message ID 20200824132252.31261-2-peter.enderborg@sony.com
State Rejected
Delegated to: Paul Moore
Headers show
Series [RFC] selinux: Add denied trace with permssion filter | expand

Commit Message

peter enderborg Aug. 24, 2020, 1:22 p.m. UTC
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(-)

Comments

Paul Moore Aug. 26, 2020, 1:42 p.m. UTC | #1
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
peter enderborg Aug. 26, 2020, 2:34 p.m. UTC | #2
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
Paul Moore Aug. 26, 2020, 2:45 p.m. UTC | #3
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.
peter enderborg Aug. 26, 2020, 3:06 p.m. UTC | #4
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.
>
Paul Moore Aug. 27, 2020, 1:30 p.m. UTC | #5
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 :)
peter enderborg Aug. 27, 2020, 2:04 p.m. UTC | #6
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.
Paul Moore Aug. 31, 2020, 2:16 p.m. UTC | #7
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.
Robert Judy Aug. 31, 2020, 2:19 p.m. UTC | #8
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
Paul Moore Aug. 31, 2020, 2:24 p.m. UTC | #9
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
peter enderborg Aug. 31, 2020, 3:34 p.m. UTC | #10
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.
Paul Moore Sept. 1, 2020, 3:31 p.m. UTC | #11
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 ...
peter enderborg Sept. 1, 2020, 5:18 p.m. UTC | #12
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.
Steven Rostedt Sept. 18, 2020, 1:47 a.m. UTC | #13
[ 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 mbox series

Patch

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);