Re: [PATCH 2/2] selinux: add attributes to avc tracepoint

[peter enderborg]

On 8/6/20 5:03 PM, Stephen Smalley wrote:
> On Thu, Aug 6, 2020 at 10:51 AM peter enderborg
> <peter.enderborg@xxxxxxxx> wrote:
>> On 8/6/20 3:49 PM, Stephen Smalley wrote:
>>> On Thu, Aug 6, 2020 at 9:45 AM Stephen Smalley
>>> <stephen.smalley.work@xxxxxxxxx> wrote:
>>>> On 8/6/20 8:32 AM, Stephen Smalley wrote:
>>>>
>>>>> On 8/6/20 8:24 AM, peter enderborg wrote:
>>>>>
>>>>>> On 8/6/20 2:11 PM, Stephen Smalley wrote:
>>>>>>> On 8/6/20 4:03 AM, Thiébaud Weksteen wrote:
>>>>>>>
>>>>>>>> From: Peter Enderborg <peter.enderborg@xxxxxxxx>
>>>>>>>>
>>>>>>>> Add further attributes to filter the trace events from AVC.
>>>>>>> Please include sample usage and output in the description.
>>>>>>>
>>>>>>>
>>>>>> Im not sure where you want it to be.
>>>>>>
>>>>>> In the commit message or in a Documentation/trace/events-avc.rst ?
>>>>> I was just asking for it in the commit message / patch description.  I
>>>>> don't know what is typical for Documentation/trace.
>>>> For example, I just took the patches for a spin, running the
>>>> selinux-testsuite under perf like so:
>>>>
>>>> sudo perf record -e avc:selinux_audited -g make test
>>>>
>>>> and then ran:
>>>>
>>>> sudo perf report -g
>>>>
>>>> and a snippet of sample output included:
>>>>
>>>>       6.40%     6.40%  requested=0x800000 denied=0x800000
>>>> audited=0x800000 result=-13 ssid=922 tsid=922
>>>> scontext=unconfined_u:unconfined_r:test_binder_mgr_t:s0-s0:c0.c1023
>>>> tcontext=unconfined_u:unconfined_r:test_binder_mgr_t:s0-s0:c0.c1023
>>>> tclass=capability
>>> So then the question becomes how do you use the above information,
>>> e.g. is that sufficient to correlate it to an actual avc: denied
>>> message, how do you decode the requested/denied/audited fields (or
>>> should the code do that for you and just report the string name(s) of
>>> the permission(s), do you need all three of those fields separately,
>>> is it useful to log the ssid/tsid at all given that you have the
>>> contexts and sids are dynamically assigned, etc.
>>>
>>>>              |
>>>>              ---0x495641000028933d
>>>>                 __libc_start_main
>>>>                 |
>>>>                 |--4.60%--__GI___ioctl
>>>>                 |          entry_SYSCALL_64
>>>>                 |          do_syscall_64
>>>>                 |          __x64_sys_ioctl
>>>>                 |          ksys_ioctl
>>>>                 |          binder_ioctl
>>>>                 |          binder_set_nice
>>>>                 |          can_nice
>>>>                 |          capable
>>>>                 |          security_capable
>>>>                 |          cred_has_capability.isra.0
>>>>                 |          slow_avc_audit
>>>>                 |          common_lsm_audit
>>>>                 |          avc_audit_post_callback
>>>>                 |          avc_audit_post_callback
>> The real cool thing happen when you enable "user-stack-trace" too.
>>
>>            <...>-4820  [007] .... 85878.897553: selinux_audited: requested=0x4000000 denied=0x4000000 audited=0x4000000 result=-13 ssid=341 tsid=61 scontext=system_u:system_r:ntpd_t:s0 tcontext=system_u:object_r:bin_t:s0 tclass=file
>>            <...>-4820  [007] .... 85878.897572: <user stack trace>
>>  =>  <00007f07d99bb45b>
>>  =>  <0000555ecd89ca57>
>>
>> The fields are useful for filter what you what to see and what you can ignore.  Having the ssid and text was from the part where it is called.
>> The numeric can be used for two things. When you dont have any context. Same same reason as in post_callback. We need to be static
>> in format so it need  be there if it ever can happen. And it is also useful for faster filtering.
>>
>> You can do "ssid!=42 && ssid!=43 && tsid==666".  From my view it would be good to have all fields there. But they need to right typed to be able
>> to use the filter mechanism. There must me some trade-off too where the argument filtering get bigger than the processing, but I think we can
>> add a lot more before we reach that threshold.
> I don't think the SIDs are useful because they are dynamically
> assigned (aside from the initial SIDs for bootstrapping before policy
> load) and are not exported to userspace (userspace has no way to look
> them up).  It is probably a mistake that we even fall back to logging
> them in the existing code and may just be a legacy of when SIDs were
> exported to userspace (ancient history, before mainline merge of
> SELinux).
>
> In any event, if you were to include the sample usage and output I
> provided as part of the commit message / patch description (or replace
> with your own example), that would be helpful I think.  Even better
> would be to also provide some hint as to how people are expected to
> decode the requested/denied/audited fields (I know how to do that but
> not everyone will, and ideally one would have a script or something
> for doing so).

But they are always the same until load off a new policy? And they can be mapped while debugging.

You see one event and then you "know" it. This is for developers so it is a different mindset than for the
netlink type that is for administrators.  Features for speeding things up are useful.

Im doing a update on commit message, will send it to Weksteen for a review round first.